Re: [PATCH v7 2/6] firmware: meson: sm: Add video firmware loading SMC call

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

Pre-existing issues:
- [High] Missing synchronization in `meson_sm_call_read` and `meson_sm_call_write` allows concurrent SMC calls to corrupt shared memory buffers.
--

--- Patch [2]: [PATCH v7 2/6] firmware: meson: sm: Add video firmware loading SMC call ---
commit df496e889046fc128eabc52108d2f27967c682ad
Author: Zhentao Guo <[email protected]>

firmware: meson: sm: Add video firmware loading SMC call

This commit adds the SM_LOAD_VIDEO_FW command to the command table for
loading video firmware.

> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index ab9751a59b552..887caa5c625db 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c
> @@ -48,6 +48,7 @@ static const struct meson_sm_chip gxbb_chip = {
>  		CMD(SM_THERMAL_CALIB_READ,	0x82000047),
>  		CMD(SM_A1_PWRC_SET,		0x82000093),
>  		CMD(SM_A1_PWRC_GET,		0x82000095),
> +		CMD(SM_LOAD_VIDEO_FW,		0xb200000f),

[Severity: High]
This is a pre-existing issue, but does adding new firmware loading commands
increase the risk of concurrent shared memory corruption?

Looking at how meson_sm_call_read() and meson_sm_call_write() use the shared
buffers:

drivers/firmware/meson/meson_sm.c:meson_sm_call_read() {
    ...
    if (meson_sm_call(fw, cmd_index, &size, arg0, arg1, arg2, arg3, arg4) < 0)
        return -EINVAL;
    ...
    if (buffer)
        memcpy(buffer, fw->sm_shmem_out_base, size);
    ...
}

and

drivers/firmware/meson/meson_sm.c:meson_sm_call_write() {
    ...
    memcpy(fw->sm_shmem_in_base, buffer, size);

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

Since these buffers (sm_shmem_out_base and sm_shmem_in_base) are shared
across the system, if two threads call into this API concurrently (for
example, unprivileged userspace reading the serial sysfs attribute while
another process reads efuse via the nvmem framework or loads video
firmware), can they overwrite each other's data in the shared memory region
before the memcpy completes?

Is there a locking mechanism protecting these SMC calls and shared buffers
that I am missing, or do we need to add a mutex to serialize access to
meson_sm_call_read() and meson_sm_call_write()?

>  		{ /* sentinel */ },
>  	},
>  };

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

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