Re: [PATCH V3 03/14] i3c: master: Do not treat master device as a duplicate target

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

Pre-existing issues:
- [High] Unregistering an I3C master causes a use-after-free if its sysfs attributes (e.g., `pid`, `bcr`, `dcr`) are read concurrently.
--

--- Patch [3]: [PATCH V3 03/14] i3c: master: Do not treat master device as a duplicate target ---
commit a98a06e576b08be2cd4e71df3fa63ba688a254ca
Author: Adrian Hunter <[email protected]>

i3c: master: Do not treat master device as a duplicate target

This commit updates i3c_master_search_i3c_dev_duplicate() to exclude the
master controller itself from matching when searching for duplicate target
devices. Since the controller is not a target device, it should not be
returned as a duplicate.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index c7bb52b71d884..abb582645a2ed 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2545,7 +2545,8 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  
>  	i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
>  		if (i3cdev != refdev && i3cdev->info.pid &&
> -		    i3cdev->info.pid == refdev->info.pid)
> +		    i3cdev->info.pid == refdev->info.pid &&
> +		    i3cdev != master->this)
>  			return i3cdev;
>  	}

[Severity: High]
This is a pre-existing issue, but unregistering an I3C master appears to
cause a use-after-free if its sysfs attributes are read concurrently.

Looking at i3c_master_unregister() in drivers/i3c/master.c:

i3c_master_unregister() {
    ...
    i3c_master_bus_cleanup(master);
    pm_runtime_disable(&master->dev);
    device_unregister(&master->dev);
    ...
}

The call to i3c_master_bus_cleanup() calls i3c_master_detach_free_devs(),
which frees all devices in the list including master->this. 

However, because device_unregister() hasn't been called yet, the sysfs files
remain exposed to userspace.

If userspace reads a sysfs attribute like pid during this window, could this
trigger a use-after-free?

drivers/i3c/master.c:pid_show() {
    ...
    desc = dev_to_i3cdesc(dev);
    ret = sysfs_emit(buf, "%llx\n", desc->info.pid);
    ...
}

Since master->this is already freed but the pointer is not NULL, 
desc->info.pid would dereference the freed memory.

Could device_unregister() be called earlier to prevent the sysfs files 
from being accessed while the cleanup happens?

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

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