Re: [PATCH v11 2/9] i3c: master: Expose the APIs to support I3C hub

On Fri, Jun 12, 2026 at 11:44:20AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [Medium] API documentation mismatch: i3c_master_reattach_i3c_dev_locked() claims to return -EBUSY on collision, but unconditionally overwrites slot status.
> - [Low] Invalid lock context expressions and typos in newly added IBI kernel-doc.
>
> Pre-existing issues:
> - [Critical] Multiple Use-After-Free vulnerabilities in i3c_dev_free_ibi_locked() due to improper synchronization and teardown order.
> - [High] Unbalanced release of address slots in i3c_master_reattach_i3c_dev_locked() failure path leads to bus corruption.
> --
>
> commit 13a1ff5c64b48ceff03545ae3bebd5e60a136c1e
> Author: Aman Kumar Pandey <[email protected]>
>
> i3c: master: Expose the APIs to support I3C hub
>
> This patch exposes several internal I3C master functions to allow new I3C
> hub drivers to use them. It adds kernel-doc comments and exports symbols
> like i3c_master_reattach_i3c_dev_locked and various IBI handling functions.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index c45b450af319d..ec3547f75469e 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1652,8 +1652,23 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
> >  	return 0;
> >  }
> >
> > -static int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
> > -					      u8 old_dyn_addr)
> > +/**
> > + * i3c_master_reattach_i3c_dev_locked() - reattach an I3C device with a new address
> > + * @dev: I3C device descriptor to reattach
> > + * @old_dyn_addr: previous dynamic address of the device
> > + *
> > + * This function reattaches an existing I3C device to the bus when its dynamic
> > + * address has changed. It updates the bus address slot status accordingly:
> > + * - Marks the new dynamic address as occupied by an I3C device.
> > + * - Frees the old dynamic address slot if applicable.
> > + *
> > + * This function must be called with the bus lock held in write mode.
> > + *
> > + * Return: 0 on success, or a negative error code if reattachment fails
> > + *         (e.g. -EBUSY if the new address slot is not free).
> > + */
> > +int i3c_master_reattach_i3c_dev_locked(struct i3c_dev_desc *dev,
> > +				       u8 old_dyn_addr)
> >  {
>
> [Severity: Medium]
> The kernel-doc states this returns -EBUSY if the new address slot is not free,
> but does the implementation actually check this?
>
> Looking at the rest of i3c_master_reattach_i3c_dev_locked(), it appears
> i3c_bus_set_addr_slot_status() is called unconditionally to set
> I3C_ADDR_SLOT_I3C_DEV without first checking if the slot is available.
>
> Could this silently overwrite an existing occupied slot if an external driver
> relies on the documented failure behavior?
>
> [Severity: High]
> This is a pre-existing issue, but does the error path safely handle a failure
> from master->ops->reattach_i3c_dev()?
>
> When the reattach operation fails, i3c_master_put_i3c_addrs(dev) is called,
> which marks the address slots as free. However, it doesn't appear to revert
> the hardware device's assigned address.
>
> Since callers like i3c_master_add_i3c_dev_locked() completely ignore the
> return value of this function, could this leave the bus manager out of sync
> with the hardware state and cause severe address collisions if the slot is
> subsequently reassigned?
>
> [ ... ]
>
> > @@ -3195,6 +3211,16 @@ int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev, struct i3c_xfer *xfers,
> >  	return master->ops->i3c_xfers(dev, xfers, nxfers, mode);
> >  }
> >
> > +/**
> > + * i3c_dev_disable_ibi_locked() - Disable IBIs coming from a specific device
> > + * @dev: device on which IBIs should be disabled
> > + *
> > + * This function disable IBIs coming from a specific device and wait for
> > + * all pending IBIs to be processed.
> > + *
> > + * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
> > + * Return: 0 in case of success, a negative error core otherwise.
> > + */
>
> [Severity: Low]
> Is the lock context expression correct here?
>
> The parameter is of type struct i3c_dev_desc, which does not have a desc
> member, so &dev->desc->ibi_lock appears to be structurally invalid.
> Should this be &dev->ibi_lock instead?

Make sense, please fix it

>
> Also, there is a minor typo: "error core" should likely be "error code".

Please this typo

Frank
>
> [ ... ]
>
> > +/**
> > + * i3c_dev_free_ibi_locked() - Free all resources needed for IBI handling
> > + * @dev: device on which you want to release IBI resources
> > + *
> > + * This function is responsible for de-allocating resources previously
> > + * allocated by i3c_device_request_ibi(). It should be called after disabling
> > + * IBIs with i3c_device_disable_ibi().
> > + *
> > + * Context: Must be called with mutex_lock(&dev->desc->ibi_lock) held.
> > + */
> >  void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
>
> [Severity: Critical]
> This is a pre-existing issue, but does this teardown order introduce a
> use-after-free regression when exposed to external drivers?
>
> Looking at the body of i3c_dev_free_ibi_locked(), the function calls
> master->ops->free_ibi(dev) before destroy_workqueue(dev->ibi->wq).
>
> If free_ibi() frees the underlying IBI pool memory, and destroy_workqueue()
> subsequently flushes pending async work items, could those work items
> execute and dereference the already freed pool?
>
> Additionally, if i3c_dev_disable_ibi_locked() had failed to disable the
> hardware interrupt, could a firing interrupt access dev->ibi after it is
> unconditionally freed by kfree(dev->ibi) at the end of this function?
>
> >  {
> >  	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > @@ -3301,6 +3366,7 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> >  	kfree(dev->ibi);
> >  	dev->ibi = NULL;
> >  }
> > +EXPORT_SYMBOL_GPL(i3c_dev_free_ibi_locked);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

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