Re: [PATCH V5 01/14] i3c: master: Fix recursive locking during device registration

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

New issues:
- [High] Dropping the bus lock during device_register() allows concurrent Dynamic Address Assignment (DAA) to free the underlying I3C device descriptor (i3cdev->desc), leading to a Use-After-Free in the uevent callback.

Pre-existing issues:
- [High] Unbalanced fwnode_handle_put in i3c_device_release leads to a refcount underflow of the boardinfo fwnode on device registration failure.
--

--- Patch [1]: [PATCH V5 01/14] i3c: master: Fix recursive locking during device registration ---
commit 7ad9b7134d768d59739289af1d4be93dc5d74a13
Author: Adrian Hunter <[email protected]>

i3c: master: Fix recursive locking during device registration
    
i3c_master_register_new_i3c_devs() registers newly discovered devices
while holding i3c_bus_normaluse_lock(), leading to a recursive
acquisition of the same rwsem. Fix this by separating device creation
from device registration.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f485b98805cf6..d2fb1a1105218 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -2104,25 +2113,37 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		if (desc->boardinfo)
>  			device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);
>  

[Severity: High]
This is a pre-existing issue, but does device_set_node() take a reference to
the fwnode here?

When an i3c boardinfo is added, a reference to the fwnode is acquired.
However, it appears device_set_node() assigns this fwnode without
incrementing the reference count. When the device is freed, the reference is
unconditionally dropped:

drivers/i3c/master.c:i3c_device_release() {
    ...
    fwnode_handle_put(dev->fwnode);
    ...
}

If device_register() fails, put_device() will invoke i3c_device_release()
and drop the fwnode reference. If the device is subsequently rediscovered, a
new device is created with the same fwnode assigned, potentially leading to a
refcount underflow and a use-after-free.

> -		ret = device_register(&desc->dev->dev);
> -		if (ret) {
> -			dev_err(&master->dev,
> -				"Failed to add I3C device (err = %d)\n", ret);
> -			desc->dev->desc = NULL;
> -			put_device(&desc->dev->dev);
> -			desc->dev = NULL;
> -		}
> +		list_add_tail(&desc->dev->node, &i3c_unreg_devs);
> +	}
> +
> +	i3c_bus_maintenance_unlock(&master->bus);
> +
> +	list_for_each_entry_safe(i3cdev, tmp, &i3c_unreg_devs, node) {
> +		ret = device_register(&i3cdev->dev);

[Severity: High]
Could dropping the bus lock around device_register() allow a concurrent
dynamic address assignment operation to free the i3c device descriptor?

During device_register(), the driver core invokes i3c_device_uevent(), which
accesses the descriptor without taking the bus lock:

drivers/i3c/master.c:i3c_device_uevent() {
    ...
    if (i3cdev->desc)
        devinfo = i3cdev->desc->info;
    ...
}

If a concurrent dynamic address assignment operation (like a hot-join)
executes, it takes the bus lock in write mode. If it detects the device as a
duplicate, it replaces the descriptor and frees the old one:

drivers/i3c/master.c:__i3c_master_add_i3c_dev_locked() {
    ...
    newdev->dev->desc = newdev;
    ...
    i3c_master_free_i3c_dev(olddev);
    ...
}

If i3c_device_uevent() fetches i3cdev->desc before the swap and dereferences
it after the free, is it possible this leads to a use-after-free?

> +		if (ret)
> +			dev_err(&master->dev, "Failed to add I3C device (err = %d)\n", ret);
> +		else
> +			list_del_init(&i3cdev->node);
> +	}

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

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