Re: [PATCH V4 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] Concurrent sysfs access during controller unregistration causes Use-After-Free on master->this.
--

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

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

i3c_master_search_i3c_dev_duplicate() searches the bus for another I3C
device with the same PID as the reference device. The search can match
master->this, causing the controller itself to 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 since this patch modifies how master->this
is handled, I noticed a potential concurrent use-after-free with master->this
during unregistration.

When an I3C master controller is unregistered, i3c_master_unregister() calls
i3c_master_bus_cleanup() before device_unregister():

drivers/i3c/master.c:i3c_master_unregister() {
	...
	i3c_master_bus_cleanup(master);
	pm_runtime_disable(&master->dev);
	device_unregister(&master->dev);
	...
}

The cleanup function eventually calls i3c_master_detach_free_devs(), which
unconditionally frees all devices in the bus list, including master->this:

drivers/i3c/master.c:i3c_master_detach_free_devs() {
	...
		i3c_master_free_i3c_dev(i3cdev);
	...
}

Because device_unregister() has not yet executed, sysfs attributes attached
to the master device remain active. 

If userspace reads a sysfs attribute during this window, would bcr_show() 
dereference the freed master->this pointer?

drivers/i3c/master.c:bcr_show() {
	...
	desc = dev_to_i3cdesc(dev);
	ret = sysfs_emit(buf, "0x%02x\n", desc->info.bcr);
	...
}

The teardown path does not appear to hold the bus->lock, so the
i3c_bus_normaluse_lock() in the sysfs handler would still succeed. Could this
cause a memory corruption or information leak?

Should device_unregister() or sysfs attribute removal happen before the bus
is cleaned up and the devices are freed?

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