Re: [PATCH V2 1/8] i3c: master: Fix recursive locking during device registration

On 28/07/2026 22:56, Frank Li wrote:
> On Tue, Jul 28, 2026 at 04:19:35PM +0000, [email protected] wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>>
>> New issues:
>> - [High] Dropping the bus maintenance lock around device_register() exposes i3c_device_uevent() and driver probe functions to a Use-After-Free if a concurrent DAA replaces the device.
>>
>> Pre-existing issues:
>> - [High] i3c_device_uevent() leaks uninitialized stack memory to userspace when i3cdev->desc is NULL.
>> --
>>
>> --- Patch [1]: [PATCH V2 1/8] i3c: master: Fix recursive locking during device registration ---
>> commit 33a4dcd6c5626faef95bc7cd02ec4c56c6710c78
>> Author: Adrian Hunter <[email protected]>
>>
>> i3c: master: Fix recursive locking during device registration
>>
>> This commit separates device creation from registration to fix a recursive
>> locking issue. It drops the bus maintenance lock around device_register()
>> to prevent deadlocks when probe callbacks invoke I3C helpers.
>>
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index fd3e79d10c840..874f6e11b296d 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> [ ... ]
>>> @@ -1999,25 +2008,37 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>>>  		if (desc->boardinfo)
>>>  			desc->dev->dev.of_node = desc->boardinfo->of_node;
>>>
>>> -		ret = device_register(&desc->dev->dev);
>>> -		if (ret) {
>>> -			dev_err(&master->dev,
>>> -				"Failed to add I3C device (err = %d)\n", ret);
>>> -			desc->dev->desc = NULL;
>>> -			put_device(&desc->dev->dev);
>>> -			desc->dev = NULL;
>>> -		}
>>> +		list_add_tail(&desc->dev->node, &i3c_unreg_devs);
>>> +	}
>>> +
>>> +	i3c_bus_maintenance_unlock(&master->bus);
>>> +
>>> +	list_for_each_entry_safe(i3cdev, tmp, &i3c_unreg_devs, node) {
>>> +		ret = device_register(&i3cdev->dev);
>>
>> [Severity: High]
>> Does dropping the maintenance lock around device_register() expose
>> i3c_device_uevent() and driver probes to a use-after-free?
> 
> look like make sense.

Yes, but it is not new.  A plain sysfs read like:

	cat /sys/bus/i3c/devices/<bus.id>-<pid>/uevent

will invoke i3c_device_uevent() without any lock.  It has been that
way since commit 6cf7b65f7029 ("i3c: Use i3cdev->desc->info instead
of calling i3c_device_get_info() to avoid deadlock")

After this patch it should now be possible to take a lock in
i3c_device_uevent(), or alternatively, put a copy of pid and dcr on
struct i3c_device for use by i3c_device_uevent() so no lock is needed.

In any case, it is a separate fix.

> 
> Frank
> 
>>
>> If a concurrent I3C hot-join event or a sysfs write to do_daa occurs while
>> device_register() is running without the lock,
>> __i3c_master_add_i3c_dev_locked() could find a duplicate PID:
>>
>> __i3c_master_add_i3c_dev_locked() {
>>         olddev = i3c_master_search_i3c_dev_duplicate(newdev);
>>         if (olddev) {
>>                 newdev->dev = olddev->dev;
>>                 if (newdev->dev)
>>                         newdev->dev->desc = newdev;
>>                 ...
>>                 i3c_master_detach_i3c_dev(olddev);
>>                 i3c_master_free_i3c_dev(olddev);
>>         }
>> }
>>
>> Meanwhile, device_register() triggers a KOBJ_ADD uevent. i3c_device_uevent()
>> reads i3cdev->desc locklessly:
>>
>> i3c_device_uevent() {
>>         if (i3cdev->desc)
>>                 devinfo = i3cdev->desc->info;
>> }
>>
>> Could this lead to i3c_device_uevent() or driver probe functions dereferencing
>> the old descriptor while it is being freed?
>>
>>> +		if (ret)
>>> +			dev_err(&master->dev, "Failed to add I3C device (err = %d)\n", ret);
>>> +		else
>>> +			list_del_init(&i3cdev->node);
>>> +	}
>>> +
>>> +	i3c_bus_maintenance_lock(&master->bus);
>>> +
>>> +	list_for_each_entry_safe(i3cdev, tmp, &i3c_unreg_devs, node) {
>>> +		list_del(&i3cdev->node);
>>> +		desc = i3cdev->desc;
>>> +		i3cdev->desc = NULL;
>>
>> [Severity: High]
>> This is a pre-existing issue, but does setting i3cdev->desc to NULL before
>> unregistration or cleanup lead to leaking uninitialized stack memory in
>> i3c_device_uevent()?
>>
>> When a device is unregistered (or if registration fails and triggers a uevent),
>> i3cdev->desc is set to NULL. This bypasses the initialization of devinfo
>> in i3c_device_uevent():
>>
>> i3c_device_uevent() {
>>         struct i3c_device_info devinfo;
>>         u16 manuf, part, ext;
>>
>>         if (i3cdev->desc)
>>                 devinfo = i3cdev->desc->info;
>>
>>         manuf = I3C_PID_MANUF_ID(devinfo.pid);
>>         ...
>>         return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X",
>>                               devinfo.dcr, manuf);
>> }
>>
>> Since devinfo is uninitialized on the stack when desc is NULL, could this
>> broadcast uninitialized kernel stack data to userspace processes listening
>> to netlink uevents?
>>
>>> +		put_device(&i3cdev->dev);
>>> +		desc->dev = NULL;
>>>  	}
>>> +
>>> +	i3c_bus_maintenance_unlock(&master->bus);
>>>  }
>>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1