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

On Tue, Jul 28, 2026 at 04:19:35PM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

look like make sense.

Frank

>
> 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/[email protected]?part=1