Re: [PATCH V4 01/14] i3c: master: Fix recursive locking during device registration

On 06/08/2026 16:39, [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 the I3C device descriptor to concurrent Use-After-Free.
> 
> Pre-existing issues:
> - [High] Unbalanced fwnode reference count decrement in i3c_device_release() leads to Use-After-Free.
> --
> 
> --- Patch [1]: [PATCH V4 01/14] i3c: master: Fix recursive locking during device registration ---
> commit b31a8482aa2ea710ccc853d0c74b77edebb131ff
> Author: Adrian Hunter <[email protected]>
> Date:   Thu Aug 6 13:18:44 2026 +0000
> 
>     i3c: master: Fix recursive locking during device registration
> 
> This commit addresses a recursive locking lockdep warning by decoupling
> device creation from registration. It collects newly populated devices into a
> temporary list under the maintenance lock, then releases the lock prior to
> invoking device_register() to prevent probe deadlocks. Finally, the lock is
> re-acquired to clean up any devices that failed registration.
> 
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -2104,25 +2113,37 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> 
> [ ... ]
> 
>> +		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 bus maintenance lock around device_register() expose the
> I3C device descriptor to a concurrent use-after-free?
> 
> During registration, the driver core can call subsystem functions like
> i3c_device_match(), which read i3cdev->desc locklessly:
> 
> drivers/i3c/master.c:i3c_device_match() {
>     ...
>     if (i3cdev->desc && i3cdev->desc->boardinfo)
>         static_addr_method = i3cdev->desc->boardinfo->static_addr_method;

The fix is in patch 7 "i3c: master: Fix potential UAF in i3c_device_match()"

>     ...
> }
> 
> Concurrently, an I3C Hot-Join or sysfs-triggered Dynamic Address Assignment
> (DAA) could execute i3c_master_do_daa(), take the i3c_bus_maintenance_lock,
> and call __i3c_master_add_i3c_dev_locked(). If DAA detects the device as a
> duplicate, it replaces the descriptor and frees the old one:
> 
> drivers/i3c/master.c:__i3c_master_add_i3c_dev_locked() {
>     ...
>     i3c_master_detach_i3c_dev(olddev);
>     i3c_master_free_i3c_dev(olddev);
>     ...
> }
> 
> If the registering thread is actively evaluating i3cdev->desc when this
> happens, will it lead to memory corruption when reading fields like
> desc->boardinfo?
> 
>> +		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;
>> +		put_device(&i3cdev->dev);
> 
> [Severity: High]
> This is a pre-existing issue, but does this error path cause an unbalanced
> fwnode reference count decrement leading to a use-after-free?
> 
> During device initialization, device_set_node() assigns the fwnode without
> incrementing its reference count:
> 
> drivers/i3c/master.c:i3c_master_register_new_i3c_devs() {
>     ...
>     if (desc->boardinfo)
>         device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);
>     ...
> }
> 
> However, when put_device() is called here on registration failure,
> the release function unconditionally drops a reference:
> 
> drivers/i3c/master.c:i3c_device_release() {
>     ...
>     fwnode_handle_put(dev->fwnode);
>     ...
> }
> 
> Since the device object never acquired a reference to the boardinfo's fwnode,
> will repeated registration failures or unbind operations drop the refcount
> below zero, eventually prematurely freeing the fwnode?
> 
>> +		desc->dev = NULL;
>>  	}
>> +
>> +	i3c_bus_maintenance_unlock(&master->bus);
>>  }
>