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

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

[ ... ]

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