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

On 04/08/2026 18:11, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Removal of the NULL check for `i3cdev->desc` introduces a guaranteed NULL pointer dereference in concurrent unregistration paths.

Not valid

> - [High] Unconditional access to `i3cdev->bus` in `i3c_device_match()` causes a Use-After-Free if the parent master controller has been freed.

Makes no sense.

> 
> Pre-existing issues:
> - [High] Preexisting use-after-free race condition in AMD SBRMI (`sbrmi_i3c_probe`) accessing `i3cdev->desc` without holding the bus lock.
> --
> 
> --- Patch [7]: [PATCH V3 07/14] i3c: master: Fix potential UAF in i3c_device_match() ---
> commit d088359c0cccb56067d426e92d0a49f24376de53
> Author: Adrian Hunter <[email protected]>
> 
> i3c: master: Fix potential UAF in i3c_device_match()
> 
> This commit protects access to i3cdev->desc with the bus normal-use lock
> in i3c_device_match() to prevent a use-after-free during concurrent
> descriptor replacement. It also removes the NULL check for i3cdev->desc,
> assuming the lock guarantees validity.
> 
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index 947ab3c681d5e..e7ea410078891 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);
> 
> [Severity: High]
> Does accessing i3cdev->bus here risk a use-after-free if the parent master
> controller has been freed?
> 
> During unregistration, i3c_master_unregister() unregisters child devices,
> and device_del() drops the child's reference to the parent by calling
> put_device(parent). Once unregistration finishes, the parent platform device
> unbinds and its devres frees the I3C master struct. 
> 
> If a concurrent bus_for_each_dev() iterator (such as from driver registration)

That makes no sense.

> holds a reference to the child device, the child outlives the parent. The
> child's i3cdev->bus pointer would then remain pointing to the freed master
> memory, leading to a use-after-free when acquiring the lock.
> 
>> +	if (i3cdev->desc->boardinfo)
> 
> [Severity: High]
> Could removing the NULL check for i3cdev->desc lead to a NULL pointer
> dereference during concurrent unregistration?
> 
> In i3c_master_unregister_i3c_devs(), i3cdev->dev->desc = NULL; is set without
> holding any bus locks. 

The DAA and registration paths are already blocked by
master->shutting_down flag - refer i3c_master_shutdown()

> 
> If a concurrent driver matching thread invokes i3c_device_match(), it can
> proceed in parallel because driver_match_device() operates outside
> device_lock(). The thread acquires the bus lock, but since unregistration
> doesn't hold it, it could read desc as NULL and crash when accessing
> i3cdev->desc->boardinfo.
> 
>>  		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
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a similar locking problem in
> the AMD SBRMI driver.
> 
> In drivers/misc/amd-sbi/rmi-i2c.c:sbrmi_i3c_probe():
> 
>     if (I3C_PID_INSTANCE_ID(i3cdev->desc->info.pid) != 1)
> 
> This dereferences i3cdev->desc without acquiring i3c_bus_normaluse_lock()
> or using the safe i3c_device_get_info() API.
> 
> Since probe runs holding device_lock(), it is not mutually exclusive with
> the bus maintenance write-lock used for descriptor replacement in
> __i3c_master_add_i3c_dev_locked(). If a device's address is concurrently
> re-assigned during DAA hot-join, could probe dereference a freed descriptor
> when it resumes?
>