Re: [PATCH v7 1/6] firmware: meson: sm: video firmware loading via secure monitor

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

New issues:
- [High] The `SM_LOAD_VIDEO_FW` command is added to the enum but lacks the required SMC ID mapping in the secure monitor command table, making it entirely unusable.

Pre-existing issues:
- [High] Missing synchronization in `meson_sm_call_read` and `meson_sm_call_write` leads to shared memory corruption and race conditions.
- [High] Object lifetime mismanagement in `meson_sm_get` returns a dangling pointer.
--

--- Patch [1]: [PATCH v7 1/6] firmware: meson: sm: video firmware loading via secure monitor ---
commit ee0d9a884be177be69f150b77b1f3503c328edfa
Author: Zhentao Guo <[email protected]>

firmware: meson: sm: video firmware loading via secure monitor

Add SM_LOAD_VIDEO_FW to the secure monitor command enum to allow decoder
drivers to load firmware through the meson_sm interface.

> diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
> index 3ebc2bd9a9760..3b90b49d05529 100644
> --- a/include/linux/firmware/meson/meson_sm.h
> +++ b/include/linux/firmware/meson/meson_sm.h
> @@ -15,6 +15,7 @@ enum {
>  	SM_THERMAL_CALIB_READ,
>  	SM_A1_PWRC_SET,
>  	SM_A1_PWRC_GET,
> +	SM_LOAD_VIDEO_FW,

[Severity: High]
Does this new command also need an SMC ID mapping in the gxbb_chip command
table in drivers/firmware/meson/meson_sm.c?

If drivers attempt to call meson_sm_call() with SM_LOAD_VIDEO_FW, it looks
like meson_sm_get_cmd() might return 0 because the command is not mapped,
causing meson_sm_call() to return -EINVAL:

drivers/firmware/meson/meson_sm.c:meson_sm_call() {
    ...
    cmd = meson_sm_get_cmd(fw->chip, cmd_index);
    if (!cmd)
        return -EINVAL;
    ...
}

[Severity: High]
This is a pre-existing issue, but will drivers loading video firmware
concurrently through this interface encounter data corruption?

Looking at drivers/firmware/meson/meson_sm.c, meson_sm_call_write() copies
data to the globally shared memory region and invokes the SMC without any
locking:

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, ...) < 0)
        return -EINVAL;
    ...
}

A similar lack of synchronization exists in meson_sm_call_read(). Could
multiple kernel threads executing these functions clobber each other's data
mid-operation?

[Severity: High]
This is a pre-existing issue, but is there an object lifetime mismanagement
issue for drivers obtaining the meson_sm context to make these firmware
calls?

In drivers/firmware/meson/meson_sm.c, meson_sm_get() retrieves the fw
structure but drops the device reference before returning it:

drivers/firmware/meson/meson_sm.c:meson_sm_get() {
    ...
    fw = platform_get_drvdata(pdev);
    put_device(&pdev->dev);
    return fw;
}

If the meson-sm device is unbound via sysfs, devres cleanup frees fw,
leaving consumer drivers with a dangling pointer that will be dereferenced
during subsequent operations. Does this require a fix?

>  };
>  
>  struct meson_sm_firmware;

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

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