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

[sashiko-bot@kernel.org]

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

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?

> +
> +	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.

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/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=1

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