Re: [PATCH v6 2/6] firmware: meson: sm: video firmware loading via secure monitor

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch series splits the addition of a new SMC command across two commits in the wrong order, breaking the build and git bisect.

Pre-existing issues:
- [High] meson_sm_call always returns 0 on success of command dispatch, masking the actual SMC return code and leading to ignored firmware errors.
- [High] Missing locking in meson_sm_call_write and meson_sm_call_read allows concurrent threads to corrupt shared memory.
- [High] meson_sm_map_shmem fails to handle SMCCC error codes, passing invalid physical addresses like 0xFFFFFFFF to ioremap_cache.
- [High] meson_sm_get returns a pointer to devres-managed memory without holding a device reference, causing a UAF if the driver is unbound.
--

--- Patch [2]: [PATCH v6 2/6] firmware: meson: sm: video firmware loading via secure monitor ---
commit 712790e34d92d775f45e1b4ef265054cbaf878df
Author: Zhentao Guo <zhentao.guo@amlogic.com>

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: Low]
Does this patch fix a build break introduced earlier in the series?

If SM_LOAD_VIDEO_FW was used in drivers/firmware/meson/meson_sm.c in a prior
commit before being declared here in the enum, it would break git bisect.

>  };
>  
>  struct meson_sm_firmware;

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

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;
}

If a caller passes NULL for ret, the function always returns 0. Should it
propagate or validate lret when ret is NULL?

[Severity: High]
This is a pre-existing issue, but does the shared memory access lack
synchronization?

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)
    ...
}

And similarly in meson_sm_call_read():

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

Can concurrent threads race here, overwriting shared memory before another
thread finishes reading its response?

[Severity: High]
This is a pre-existing issue, but could this fail to handle SMCCC error codes
correctly?

drivers/firmware/meson/meson_sm.c:meson_sm_map_shmem() {
    u32 sm_phy_base;

    sm_phy_base = __meson_sm_call(cmd_shmem, 0, 0, 0, 0, 0);
    if (!sm_phy_base)
        return NULL;

    return ioremap_cache(sm_phy_base, size);
}

If the secure monitor returns a negative error code, sm_phy_base (u32) becomes
0xFFFFFFFF. Will ioremap_cache() attempt to map this invalid physical address?

[Severity: High]
This isn't a bug introduced by this patch, but can this result in a dangling
pointer when returning devres-managed memory?

drivers/firmware/meson/meson_sm.c:meson_sm_get() {
    struct platform_device *pdev = of_find_device_by_node(sm_node);
    struct meson_sm_firmware *fw;
    ...
    fw = platform_get_drvdata(pdev);

    put_device(&pdev->dev);

    return fw;
}

Since fw is allocated with devm_kzalloc() bound to the meson-sm device, if the
driver is unbound via sysfs, devres frees the memory. Do consumer drivers
retain a dangling pointer to fw after put_device() drops the reference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-b4-s4-vdec-upstream-v6-0-6072a7ee7428@amlogic.com?part=2