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

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

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

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

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