Re: [PATCH v11 1/9] i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of `i3c_master_reattach_i3c_dev_locked()` is ignored in `i3c_master_add_i3c_dev_locked()`, leading to bus state corruption if reattachment fails.
- [High] Aggressive cleanup in `i3c_master_reattach_i3c_dev_locked()` sabotages the caller's hardware rollback path (`RSTDAA`) in `i3c_master_early_i3c_dev_add()`.
- [High] `i3c_device_uevent()` accesses `i3cdev->desc` without acquiring `bus->lock`, leading to a Use-After-Free if a hot-join device replacement occurs concurrently.
--

commit 8a7af37c292c3791d232cdb1c2f37ae5aba2ca10
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

i3c: master: rename i3c_master_reattach_i3c_dev() to *_locked
    
Rename i3c_master_reattach_i3c_dev() to *_locked() to make the locking
requirement explicit and consistent with other I3C core helpers that
require the bus lock to be held by the caller.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 6b8df8089a357..c45b450af319d 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1742,7 +1742,7 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
>  		goto err_detach_dev;
>  
>  	i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
> -	ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
> +	ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't the error path
here break if the reattach fails?

If master->ops->reattach_i3c_dev() fails,
i3c_master_reattach_i3c_dev_locked() immediately marks the
dev->info.dyn_addr slot as I3C_ADDR_SLOT_FREE via i3c_master_put_i3c_addrs().

This seems to sabotage the fallback path at err_rstdaa:

drivers/i3c/master.c:i3c_master_early_i3c_dev_add() {
    ...
    err_rstdaa:
        i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
    ...
}

Since the address slot is now marked free, i3c_master_rstdaa_locked()
will instantly abort with -EINVAL because the slot status is no longer
I3C_ADDR_SLOT_I3C_DEV. Will this leave the physical device retaining
the dynamic address while the master controller incorrectly considers it free?

>  	if (ret)
>  		goto err_rstdaa;
>  

[ ... ]

> @@ -2358,7 +2358,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  		if (!ret) {
>  			old_dyn_addr = newdev->info.dyn_addr;
>  			newdev->info.dyn_addr = expected_dyn_addr;
> -			i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
> +			i3c_master_reattach_i3c_dev_locked(newdev, old_dyn_addr);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return
value of i3c_master_reattach_i3c_dev_locked() here?

If the underlying master->ops->reattach_i3c_dev() fails, the reattach
function calls i3c_master_put_i3c_addrs(), marking the device's assigned
dynamic address slot as I3C_ADDR_SLOT_FREE in the bus bitmask.

Since this failure is completely ignored and the caller returns success,
the hardware remains configured to listen on the new dynamic address, but
the software allocator considers the slot free. Could a subsequent
hot-join or device addition be assigned this exact same dynamic address,
leading to unrecoverable bus collisions?

[Severity: High]
Additionally, this isn't a bug introduced by this patch, but does
i3c_master_add_i3c_dev_locked() race with userspace uevents during a
hot-join device replacement?

Later in this function, it identifies duplicate devices, migrates
the generic struct device from the old descriptor to the new one, and
eventually frees the old descriptor via i3c_master_free_i3c_dev(olddev).
This process is protected by the bus maintenance lock.

However, i3c_device_uevent() is triggered by sysfs and directly
dereferences i3cdev->desc without acquiring the bus lock or employing RCU:

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

If a thread executing i3c_device_uevent() evaluates i3cdev->desc, gets
preempted while i3c_master_add_i3c_dev_locked() frees olddev, and then
resumes, will this trigger a use-after-free during the structure copy?

>  		} else {
>  			dev_err(&master->dev,
>  				"Failed to assign reserved/old address to device %d%llx",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=1