Re: [PATCH v2 2/2] firmware: arm_scmi: request modules for discovered protocols

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free in `scmi_protocol_device_unrequest` during concurrent driver unregistration.
- [High] IDA and SystemPower protocol state leak when the SCMI platform device is unbound.
--

commit 55b65f66dfc5c3002d324389dda6dc66790201cc
Author: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

firmware: arm_scmi: request modules for discovered protocols

This commit enables autoloading for scmi driver modules by emitting a
protocol-level alias for scmi device ID table entries. It requests the
alias when discovering implemented protocols so that driver modules can
be loaded before the concrete client name is available.

> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 0fd6a947499e7..7d33fab94e281 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but can concurrent driver unregistration
lead to a use-after-free in drivers/firmware/arm_scmi/bus.c?

Looking at scmi_protocol_device_unrequest(), it drops the
scmi_requested_devices_mtx lock to call blocking_notifier_call_chain().

If two threads execute concurrent module unloads for different scmi
drivers on the same protocol:

Thread 1 completes the notifier, re-acquires the lock, and frees phead:
    if (list_empty(phead)) {
        idr_remove(&scmi_requested_devices, id_table->protocol_id);
        kfree(phead);
    }

Would Thread 2 then evaluate list_empty(phead) using the just-freed
phead pointer when it finishes its own notifier and re-acquires the lock?


[Severity: High]
This isn't a bug introduced by this patch, but does the scmi platform
device unbinding leak the scmi_bus_id ida and system power protocol state?

When scmi_remove() calls scmi_destroy_protocol_devices() with a NULL
name, it eventually reaches scmi_child_dev_find() in
drivers/firmware/arm_scmi/bus.c:

    strscpy(id_table[0].name, name, sizeof(id_table[0].name));

Since strscpy() does not accept a NULL source pointer, doesn't this
trigger a warning and leave the target name empty?

If this empty name causes scmi_dev_match_by_id_table() to fail, the
device won't be found and the manual cleanup in __scmi_device_destroy()
is bypassed.

The device core will then automatically unregister the child devices
using:

    static void scmi_device_release(struct device *dev)
    {
        struct scmi_device *scmi_dev = to_scmi_dev(dev);

        kfree_const(scmi_dev->name);
        kfree(scmi_dev);
    }

Since this release callback skips explicit ida and state cleanup, does
this permanently prevent the system power protocol device from being
recreated if the driver is rebound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-scmi-modalias-v2-0-8c7547c1be21@oss.qualcomm.com?part=2