Re: [PATCH V2 1/8] i3c: master: Fix recursive locking during device registration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dropping the bus maintenance lock around device_register() exposes i3c_device_uevent() and driver probe functions to a Use-After-Free if a concurrent DAA replaces the device.

Pre-existing issues:
- [High] i3c_device_uevent() leaks uninitialized stack memory to userspace when i3cdev->desc is NULL.
--

--- Patch [1]: [PATCH V2 1/8] i3c: master: Fix recursive locking during device registration ---
commit 33a4dcd6c5626faef95bc7cd02ec4c56c6710c78
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: master: Fix recursive locking during device registration

This commit separates device creation from registration to fix a recursive
locking issue. It drops the bus maintenance lock around device_register()
to prevent deadlocks when probe callbacks invoke I3C helpers.

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index fd3e79d10c840..874f6e11b296d 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
[ ... ]
> @@ -1999,25 +2008,37 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		if (desc->boardinfo)
>  			desc->dev->dev.of_node = desc->boardinfo->of_node;
>  
> -		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]
Does dropping the maintenance lock around device_register() expose
i3c_device_uevent() and driver probes to a use-after-free?

If a concurrent I3C hot-join event or a sysfs write to do_daa occurs while
device_register() is running without the lock,
__i3c_master_add_i3c_dev_locked() could find a duplicate PID:

__i3c_master_add_i3c_dev_locked() {
        olddev = i3c_master_search_i3c_dev_duplicate(newdev);
        if (olddev) {
                newdev->dev = olddev->dev;
                if (newdev->dev)
                        newdev->dev->desc = newdev;
                ...
                i3c_master_detach_i3c_dev(olddev);
                i3c_master_free_i3c_dev(olddev);
        }
}

Meanwhile, device_register() triggers a KOBJ_ADD uevent. i3c_device_uevent()
reads i3cdev->desc locklessly:

i3c_device_uevent() {
        if (i3cdev->desc)
                devinfo = i3cdev->desc->info;
}

Could this lead to i3c_device_uevent() or driver probe functions dereferencing
the old descriptor while it is being freed?

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

[Severity: High]
This is a pre-existing issue, but does setting i3cdev->desc to NULL before
unregistration or cleanup lead to leaking uninitialized stack memory in
i3c_device_uevent()?

When a device is unregistered (or if registration fails and triggers a uevent),
i3cdev->desc is set to NULL. This bypasses the initialization of devinfo
in i3c_device_uevent():

i3c_device_uevent() {
        struct i3c_device_info devinfo;
        u16 manuf, part, ext;

        if (i3cdev->desc)
                devinfo = i3cdev->desc->info;

        manuf = I3C_PID_MANUF_ID(devinfo.pid);
        ...
        return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X",
                              devinfo.dcr, manuf);
}

Since devinfo is uninitialized on the stack when desc is NULL, could this
broadcast uninitialized kernel stack data to userspace processes listening
to netlink uevents?

> +		put_device(&i3cdev->dev);
> +		desc->dev = NULL;
>  	}
> +
> +	i3c_bus_maintenance_unlock(&master->bus);
>  }

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c