Re: [PATCH V4 07/14] i3c: master: Fix potential UAF in i3c_device_match()

On 06/08/2026 16:45, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Removing the NULL check for `i3cdev->desc` introduces a deterministic NULL pointer dereference during device unregistration and registration error paths.
> - [High] `i3c_device_get_info()` dereferences `dev->desc` without checking for NULL under the lock, causing a NULL pointer dereference.
> --
> 
> --- Patch [7]: [PATCH V4 07/14] i3c: master: Fix potential UAF in i3c_device_match() ---
> commit 4b5a3c82a1c4c5f9bf31e925977c2286520960fc
> Author: Adrian Hunter <[email protected]>
> 
> i3c: master: Fix potential UAF in i3c_device_match()
> 
> This commit protects access to i3cdev->desc with the normal-use lock to
> prevent a use-after-free when the descriptor pointer is concurrently
> replaced. Since the descriptor is assumed to remain valid while the lock is
> held, the NULL check is removed.
> 
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index ee771ac0b203e..83e04a2c202a9 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -347,8 +347,10 @@ static int i3c_device_match(struct device *dev, const struct device_driver *drv)
>>  	i3cdev = dev_to_i3cdev(dev);
>>  	i3cdrv = drv_to_i3cdrv(drv);
>>  
>> -	if (i3cdev->desc && i3cdev->desc->boardinfo)
>> +	i3c_bus_normaluse_lock(i3cdev->bus);
>> +	if (i3cdev->desc->boardinfo)
> 
> [Severity: High]
> Is it safe to remove the NULL check for i3cdev->desc here?
> 
> If a userspace process writes to a driver's bind file via sysfs concurrently
> with a device registration failure, bind_store() will call
> driver_match_device() and eventually reach i3c_device_match().
> 
> In the error path of i3c_master_register_new_i3c_devs(), i3cdev->desc is set

i3c_master_register_new_i3c_devs() adds the device, so the error path
means the device was not added, so it cannot concurrently be being bound.

> to NULL under the write lock. When this code subsequently acquires the read
> lock, i3cdev->desc will be NULL, which could cause a NULL pointer
> dereference.
> 
> Similarly, i3c_master_unregister_i3c_devs() locklessly sets i3cdev->desc to
> NULL during teardown, which can also race with this path.
> 
>>  		static_addr_method = i3cdev->desc->boardinfo->static_addr_method;
>> +	i3c_bus_normaluse_unlock(i3cdev->bus);
>>  
>>  	/*
>>  	 * SETAASA-based devices need not always have a matching ID since
>>  	 * it is not mandatory for such devices to implement deviceinfo
>>  	 * CCC commands. Allow them to register through DT or ACPI.
>>  	 */
>>  	if (i3cdrv->id_table && i3c_device_match_id(i3cdev, i3cdrv->id_table))
> 
> [Severity: High]
> Does this call to i3c_device_match_id() have a similar vulnerability?
> 
> i3c_device_match_id() eventually calls i3c_device_get_info(). While
> i3c_device_get_info() does acquire the normal-use lock, it directly reads
> dev->desc->info without checking if dev->desc is valid.
> 
> If a bind operation races with device unregistration or a registration error

As above bind operation races with registration error is nonsense.

Unregistration path was fixed by "i3c: master: Fix info leak and UAF in
device unregister path" currently in i3c-next

> path, dev->desc could be set to NULL before the lock is acquired, leading to
> a crash when reading dev->desc->info.
> 
>>  		return 1;
>>  
>>  	if (static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> 
> [ ... ]
> 


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