Re: [PATCH v3 4/4] firmware: meson: pass a const buffer to meson_sm_call_write()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] `meson_sm_call()` propagates SMC failures via its output parameter `*ret` but unconditionally returns 0 on success. Callers incorrectly check the return value instead of validating the output parameter, leading to silently ignored errors and state corruption.
- [High] Missing mutual exclusion in `meson_sm_call_read` and `meson_sm_call_write` when accessing the shared IO memory buffers `fw->sm_shmem_in_base` and `fw->sm_shmem_out_base`.
--

--- Patch [4]: [PATCH v3 4/4] firmware: meson: pass a const buffer to meson_sm_call_write() ---
commit dafeddcc00e60d8baf4fc20e8f8f2f6286a33db3
Author: Link Mauve <[email protected]>

firmware: meson: pass a const buffer to meson_sm_call_write()

This commit updates the meson_sm_call_write() signature to accept a
const void *buffer instead of void *buffer. It also removes unnecessary
casts to (u8 *) in meson_efuse_read() and meson_efuse_write() when
passing the buffer to the SMC call functions.

> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index ab9751a59b552..5d4da2e3c3a94 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c
> @@ -196,7 +196,7 @@ EXPORT_SYMBOL(meson_sm_call_read);
>   *
>   * Return:	size of sent data on success, a negative value on error
>   */
> -int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
> +int meson_sm_call_write(struct meson_sm_firmware *fw, const void *buffer,
>  			unsigned int size, unsigned int cmd_index, u32 arg0,
>  			u32 arg1, u32 arg2, u32 arg3, u32 arg4)
>  {

[Severity: High]
This is a pre-existing issue, but while reviewing this function, is there a
missing lock when accessing shared IO memory?

In meson_sm_call_write(), we copy data to shared IO memory and execute
an SMC call:

	memcpy(fw->sm_shmem_in_base, buffer, size);

	if (meson_sm_call(fw, cmd_index, &written, arg0, arg1, arg2, arg3, arg4) < 0)

If multiple threads call meson_sm_call_write() or meson_sm_call_read()
concurrently, could they clobber the shared IO memory buffers before the
Secure Monitor executes the command? Access to fw->sm_shmem_in_base and
fw->sm_shmem_out_base does not appear to be serialized with a mutex.

[Severity: High]
This isn't a bug introduced by this patch, but does meson_sm_call() mask
SMC execution failures?

Looking at meson_sm_call_write(), it checks the return value:

	if (meson_sm_call(fw, cmd_index, &written, arg0, arg1, arg2, arg3, arg4) < 0)
		return -EINVAL;

However, meson_sm_call() unconditionally returns 0:

drivers/firmware/meson/meson_sm.c:meson_sm_call()
	lret = __meson_sm_call(cmd, arg0, arg1, arg2, arg3, arg4);

	if (ret)
		*ret = lret;

	return 0;

Because it always returns 0, callers checking the return value instead of
the output parameter will silently ignore errors. Could this cause drivers
like meson-efuse or amlogic_thermal to treat error codes as valid data and
corrupt state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic