Re: [PATCH v12 1/7] i3c: master: Add APIs for I3C hub support

On Wed, Jun 17, 2026 at 11:20:35AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Bypassing core state tracking in `direct_attach` / `direct_detach` causes Use-After-Free, NULL pointer dereferences, and Memory Leaks.
> - [High] Bypassing address slot reservation in `direct_attach` leads to unrecoverable I3C bus address collisions.
> --
>
> commit ce08f4a6bc0d82c966d4e3337e6b4d082a9d09d2
> Author: Aman Kumar Pandey <[email protected]>
>
> i3c: master: Add APIs for I3C hub support
>
> This commit introduces helper functions for attaching and detaching I3C
> devices, alongside CCC command helpers. These additions prepare the core
> framework for handling I3C hubs.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1775,6 +1775,70 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
>
> [ ... ]
>
> > +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> > +					    struct i3c_dev_desc *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * We don't attach devices to the controller until they are
> > +	 * addressable on the bus.
> > +	 */
> > +
> > +	if (!dev->info.static_addr && !dev->info.dyn_addr)
> > +		return 0;
> > +
> > +	/* Do not attach the master device itself. */
> > +	if (master->this != dev && master->ops->attach_i3c_dev)
> > +		ret = master->ops->attach_i3c_dev(dev);
>
> [Severity: High]
> Does bypassing address slot reservation here lead to I3C bus collisions?
>
> In the standard attachment flow, i3c_master_get_i3c_addrs() is called to
> reserve the address slot in the controller's bus.addrslots bitmap. Because
> this direct helper skips that reservation, the parent controller's address
> map is never updated to reflect the newly occupied address.
>
> Since the parent controller and the virtual hub share the physical I3C bus,
> the parent might still believe the downstream device's address is free. If
> a new device hot-joins, could the parent controller allocate the exact same
> address, resulting in an unrecoverable address collision?

Should check address before attached.

>
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
> > +
> > +/**
> > + * i3c_master_direct_detach_i3c_dev_locked() - Detach an I3C device from its
> > + * master controller.
>
> [ ... ]
>
> > +void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
> > +{
> > +	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > +
> > +	/*
> > +	 * We don't attach devices to the controller until they are
> > +	 * addressable on the bus, so skip detach for unaddressed devices.
> > +	 */
> > +	if (!dev->info.static_addr && !dev->info.dyn_addr)
> > +		return;
> > +
> > +	/* Do not detach the master device itself. */
> > +	if (master->this != dev && master->ops->detach_i3c_dev)
> > +		master->ops->detach_i3c_dev(dev);
>
> [Severity: Critical]
> Can this lead to a use-after-free or a NULL pointer dereference by bypassing
> the core state tracking?
>
> When i3c_master_direct_detach_i3c_dev_locked() invokes the controller's
> detach callback, master drivers typically free the master_priv data. However,
> the dev->common.master_priv pointer is never cleared here.

It'd better clean it and free dyn_addr.

Frank

>
> If i3c_hub_free_ibi() calls this detach helper, and later a transfer is sent
> via i3c_hub_master_priv_xfers():
>
> drivers/i3c/hub.c:i3c_hub_master_priv_xfers() {
>     ...
>     xfer_ret = i3c_master_priv_xfers(dev, xfers, nxfers, mode);
>     ...
> }
>
> Will the parent controller end up dereferencing the freed master_priv
> pointer?
>
> Additionally, if a device is never explicitly attached via the direct helper
> (leaving master_priv NULL), would a standard transfer immediately cause a
> NULL pointer dereference when forwarded to the parent driver?
>
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
>
> --
> 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