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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The removal of the `i3cdev->desc` NULL check introduces a deterministic NULL pointer dereference in `i3c_device_match` during concurrent driver binding.

Pre-existing issues:
- [High] Incomplete fix: A Use-After-Free of the descriptor is still possible during master unregistration because it executes locklessly.
--

--- Patch [7]: [PATCH V5 07/14] i3c: master: Fix potential UAF in i3c_device_match() ---
commit 8aa5de79b60445da0abee6c6a17c70b1b9ec8be9
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: master: Fix potential UAF in i3c_device_match()

i3c_device_match() dereferences i3cdev->desc without holding the bus
normal-use lock.  Since the descriptor pointer can be replaced
concurrently, the dereference can race with descriptor replacement and
result in a use-after-free.

Protect access to i3cdev->desc with the normal-use lock.  While the lock
is held, the descriptor is guaranteed to remain valid, so the NULL check
is also unnecessary and can be 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]
Can removing this NULL check lead to a NULL pointer dereference during
concurrent driver binding?

Looking at master unregistration, the descriptor pointer is cleared
locklessly:

drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
        ...
        i3cdev->dev->desc = NULL;
        ...
}

If a concurrent sysfs bind operation triggers i3c_device_match(), it might
acquire the read lock but still observe a NULL descriptor since unregistration
does not acquire i3c_bus_normaluse_lock(). Does the removed NULL check
now guarantee a crash in this race condition?

[Severity: High]
This is a pre-existing issue, but does taking the normal-use lock fully
prevent the use-after-free this patch aims to fix?

Master unregistration modifies and initiates cleanup of descriptors without
acquiring any bus locks, bypassing this protection entirely:

drivers/i3c/master.c:i3c_master_bus_cleanup() {
        ...
        i3c_master_detach_free_devs(master);
        ...
}

If a concurrent sysfs bind operation reads i3cdev->desc just before it is
cleared, the read lock here doesn't seem to serialize against the
unregistration thread proceeding to free the descriptor. Can the descriptor
still be freed while i3c_device_match() is dereferencing it?

>  		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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807145638.168865-1-adrian.hunter@intel.com?part=7