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

[sashiko-bot at kernel.org]

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 <aman.kumarpandey at nxp.com>

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?

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

[ ... ]

> +/**
> + * 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/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=2