Re: [PATCH v2 1/2] module: add SCMI device table alias support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dropping `scmi_requested_devices_mtx` during list iteration in `scmi_protocol_device_unrequest` leads to Use-After-Free and double-free.
- [Critical] `scmi_child_dev_find` returning an unreferenced pointer causes a Use-After-Free if the device is concurrently unregistered.
- [High] If `device_register` fails in `__scmi_device_create` for the `SCMI_PROTOCOL_SYSTEM` protocol, the atomic flag `scmi_syspower_registered` is not reset, permanently preventing future registrations.
--

commit 90162c8443181b14c6b5151402b556abe1a600f4
Author: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

module: add SCMI device table alias support

SCMI client drivers already describe their bus match data with
MODULE_DEVICE_TABLE(scmi, ...), but modpost does not know how to consume
SCMI device tables. As a result, SCMI modules do not get generated module
aliases from their id tables.

> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 793be9eabaedd..70781146fa613 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
__scmi_device_create() leak the atomic state for scmi_syspower_registered?

When device_register() fails for the SCMI_PROTOCOL_SYSTEM protocol, the code
jumps to the put_dev error path:

put_dev:
	put_device(&scmi_dev->dev);
	ida_free(&scmi_bus_id, id);
	return NULL;

This path cleans up memory and the IDA, but it doesn't seem to reset the
global scmi_syspower_registered atomic flag back to 0. Does this permanently
block subsequent attempts to register the system power protocol until reboot?

[ ... ]

> @@ -197,18 +199,18 @@ scmi_protocol_table_unregister(const struct scmi_device_id *id_table)
>  {
>  	const struct scmi_device_id *entry;
>  
> -	for (entry = id_table; entry->name; entry++)
> +	for (entry = id_table; entry->name[0]; entry++)
>  		scmi_protocol_device_unrequest(entry);

[Severity: Critical]
This is a pre-existing issue, but can the lock dropping in
scmi_protocol_device_unrequest() lead to a use-after-free or double-free?

scmi_protocol_device_unrequest() drops the scmi_requested_devices_mtx lock
during list iteration:

	mutex_lock(&scmi_requested_devices_mtx);
	phead = idr_find(&scmi_requested_devices, id_table->protocol_id);
	if (phead) {
		...
			if (!strcmp(victim->id_table->name, id_table->name)) {
				list_del(&victim->node);
				mutex_unlock(&scmi_requested_devices_mtx);
				...
				kfree(victim);
				mutex_lock(&scmi_requested_devices_mtx);
				break;
			}
		}

		if (list_empty(phead)) {
			idr_remove(&scmi_requested_devices,
				   id_table->protocol_id);
			kfree(phead);
		}
	}

If another thread concurrently removes an entry from the same phead list,
sees the list is empty, and frees phead, wouldn't the original thread
evaluate if (list_empty(phead)) with a freed pointer when it reacquires the
lock and breaks the loop?

[ ... ]

> @@ -245,7 +247,7 @@ static struct scmi_device *scmi_child_dev_find(struct device *parent,
>  	struct device *dev;
>  
>  	id_table[0].protocol_id = prot_id;
> -	id_table[0].name = name;
> +	strscpy(id_table[0].name, name, sizeof(id_table[0].name));
>  
>  	dev = device_find_child(parent, &id_table, scmi_match_by_id_table);

[Severity: Critical]
This is a pre-existing issue, but does returning an unreferenced pointer here
cause a use-after-free if the device is concurrently unregistered?

Right after the device_find_child() call above, the reference is dropped:

	if (!dev)
		return NULL;

	/* Drop the refcnt bumped implicitly by device_find_child */
	put_device(dev);

	return to_scmi_dev(dev);

device_find_child() increments the reference count of the device, but
put_device() immediately drops it. The caller will receive a raw pointer
without holding any locks or active references. Could the memory be freed
out from under the caller before they use it?

[ ... ]

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