[PATCH v13 5/7] i3c: hub: Add support for the I3C interface in the I3C hub
Frank Li
Frank.li at oss.nxp.com
Wed Jul 1 12:42:20 PDT 2026
On Wed, Jul 01, 2026 at 07:23:18AM +0000, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [Critical] `i3c_hub_request_ibi()` intentionally frees `desc->ibi` out from under its caller without cleaning up its workqueue, causing a memory leak and a double-free on failure.
> - [High] `i3c_hub_free_ibi()` causes a guaranteed NULL pointer dereference in the core IBI free path by recursively clearing `dev->ibi`.
> - [High] Address spoofing in `i3c_hub_master_priv_xfers` corrupts parent bus address reservations and suffers from an unprotected race window.
> - [High] Missing `xfer_lock` in several hub operations allows concurrent port enabling, breaking virtual bus isolation.
> - [High] `i3c_hub_free_ibi()` permanently destroys downstream device address reservations on the parent bus.
> - [High] `i3c_hub_do_daa` delegates DAA entirely to the parent controller, breaking boardinfo matching and DT topology for downstream devices.
> - [Medium] Acquiring `parent->bus.lock` while holding `child->bus.lock` triggers a lockdep splat due to unannotated same-class lock nesting.
> --
>
> commit 22c2a31197952a434777498d93a1ed925009d6de
> Author: Lakshay Piplani <lakshay.piplani at nxp.com>
> Subject: i3c: hub: Add support for the I3C interface in the I3C hub
>
> This patch adds virtual I3C bus support for the hub and provides an
> interface to enable or disable downstream ports. Virtual master controllers
> are created for the downstream ports, and bus operations are forwarded
> through the hub device.
>
> > diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> > new file mode 100644
> > index 0000000000000..d29ed40b6f735
> > --- /dev/null
> > +++ b/drivers/i3c/hub.c
> > @@ -0,0 +1,566 @@
>
> [ ... ]
>
> > +static int i3c_hub_do_daa(struct i3c_hub *hub,
> > + struct i3c_master_controller *controller)
> > +{
> > + struct i3c_master_controller *parent;
> > + int ret;
> > +
> > + if (!hub || !hub->hub_dev)
> > + return -ENODEV;
> > +
> > + parent = i3c_dev_get_master(hub->hub_dev->desc);
> > + if (!parent)
> > + return -ENODEV;
> > +
> > + i3c_hub_enable_port(controller);
> > + ret = i3c_master_do_daa(parent);
>
> [Severity: High]
> Does delegating DAA directly to the parent controller bypass the virtual hub
> bus entirely?
>
> By calling i3c_master_do_daa(parent), it appears the parent discovers and
> attaches downstream devices to the physical bus instead of the virtual
> controller. Does this cause boardinfo matching to fail since the devices
> are defined under the hub's device tree node?
Add comments here: like
/*
All target devices which connect active ports of hub are tread as
connect to parent bus at same time. So keep simple, all devices dynamatic
address should be in the same address space, that means it is differences
for all target devices even it is at difference ports. So far, address
(128, need double check) is enough now.
*/
Please rephrase it.
>
> > + i3c_hub_disable_port(controller);
> > +
> > + return ret;
> > +}
>
> [ ... ]
>
> > +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> > + struct i3c_master_controller *controller,
> > + struct i3c_ccc_cmd *cmd)
> > +{
> > + struct i3c_master_controller *parent;
> > + int ret;
> > +
> > + if (!hub || !hub->hub_dev)
> > + return -ENODEV;
> > +
> > + parent = i3c_dev_get_master(hub->hub_dev->desc);
> > + if (!parent)
> > + return -ENODEV;
> > +
> > + i3c_hub_enable_port(controller);
> > + ret = i3c_master_send_ccc_cmd(parent, cmd);
>
> [Severity: High]
> Could the omission of hub->xfer_lock in this function allow concurrent port
> enabling?
look like correct, you acquire it at i3c_hub_master_priv_xfers(), should
be similar here.
>
> The lock is used to serialize private transfers, but callbacks like
> i3c_hub_send_ccc_cmd(), i3c_hub_do_daa(), and i3c_hub_enable_ibi() do not
> acquire it. If a private transfer runs concurrently with a CCC command,
> could multiple downstream ports be enabled simultaneously and cause the hub
> to broadcast traffic incorrectly?
>
> > + i3c_hub_disable_port(controller);
> > +
> > + return ret;
> > +}
>
> [ ... ]
>
> > +static int i3c_hub_master_priv_xfers(struct i3c_dev_desc *dev,
> > + struct i3c_xfer *xfers,
> > + int nxfers,
> > + enum i3c_xfer_mode mode)
> > +{
> > + struct i3c_master_controller *controller = i3c_dev_get_master(dev);
> > + struct i3c_hub_controller *hub_controller;
> > + struct i3c_dev_desc *hub_dev;
> > + struct i3c_master_controller *parent;
> > + u8 hub_addr, target_addr;
> > + struct i3c_hub *hub;
> > + int ret, xfer_ret;
>
> [ ... ]
>
> > + if (hub_addr != target_addr) {
> > + down_write(&parent->bus.lock);
call i3c_bus_maintenance_lock(), move it to API.
>
> [Severity: Medium]
> Will acquiring parent->bus.lock here trigger a lockdep warning?
>
> Since virtual hub operations are invoked by the I3C core with the child
> bus's lock already held, acquiring the parent bus's lock (which belongs to
> the same lock class) without nesting annotations might produce a false
> positive lockdep splat. This also happens in i3c_hub_enable_ibi() and
> i3c_hub_disable_ibi().
>
> > + hub_dev->info.dyn_addr = target_addr;
> > +
> > + ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
> > + if (ret) {
> > + up_write(&parent->bus.lock);
> > + hub_dev->info.dyn_addr = hub_addr;
> > + goto disable;
> > + }
> > + up_write(&parent->bus.lock);
> > + }
> > +
> > + ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
> > +
> > + if (hub_addr != target_addr) {
> > + down_write(&parent->bus.lock);
> > + hub_dev->info.dyn_addr = hub_addr;
> > +
> > + xfer_ret = i3c_master_reattach_i3c_dev_locked(hub_dev,
> > + target_addr);
>
> [Severity: High]
> Does this sequence permanently destroy device tree static address
> reservations?
>
> When the hub's dynamic address is altered via
> i3c_master_reattach_i3c_dev_locked(), the hub's original address slot is
> set to free on the parent bus, leaving it vulnerable to DAA theft since the
> parent write-lock is dropped during the transfer. When the address is
> restored later by passing target_addr to i3c_master_reattach_i3c_dev_locked,
> this marks the downstream device's address as free, which seems to wipe out
> its reservation.
It makes sense
i3c_bus_maintenance_lock()
i3c_master_reattach_i3c_dev_locked()
i3c_device_do_xfers_locked()
i3c_master_reattach_i3c_dev_locked()
i3c_bus_maintenance_unlock()
Another problem is previous lockdep().
int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_xfer *xfers,
int nxfers, enum i3c_xfer_mode mode)
{
...
i3c_bus_normaluse_lock(dev->bus);
ret = i3c_dev_do_xfers_locked(dev->desc, xfers, nxfers, mode);
i3c_bus_normaluse_unlock(dev->bus);
...
return ret;
}
simple method is promote i3c_bus_normaluse_lock() to i3c_bus_maintenance_lock()
when dev attached to hub devices.
and also lock parent master's i3c_bus_maintenance_lock(),
So here needn't lock in hub.c
if parent master use write lock, hub->xfer_lock needn't
>
> > +
> > + up_write(&parent->bus.lock);
> > + if (!ret)
> > + ret = xfer_ret;
> > + }
> > +disable:
> > + i3c_hub_disable_port(controller);
> > + mutex_unlock(&hub->xfer_lock);
> > + return ret;
> > +}
>
> [ ... ]
>
> > +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> > + const struct i3c_ibi_setup *req)
> > +{
> > + struct i3c_master_controller *controller = i3c_dev_get_master(desc);
> > + struct i3c_hub_controller *hub_controller;
> > + struct i3c_master_controller *orig_parent;
> > + struct i3c_master_controller *parent;
> > + struct i3c_hub *hub;
> > + int ret;
>
> [ ... ]
>
> > + orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> > +
> > + ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
All IBI cap devices should be already attached to master bus when DAA? need
attach again here?
Actived port's devices logically attached to master bus?
So needn't switch port.
> > + if (ret) {
> > + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> > + up_write(&parent->bus.lock);
> > + return ret;
> > + }
> > +
> > + kfree(desc->ibi);
> > + desc->ibi = NULL;
> > + ret = i3c_dev_request_ibi_locked(desc, req);
>
> [Severity: Critical]
> Can freeing desc->ibi here cause a memory leak and a double-free?
>
> If a downstream device requests an IBI, the core i3c_dev_request_ibi_locked()
> allocates ibi and ibi->wq before calling this hub operation. By calling
> kfree(desc->ibi) here, the outer workqueue is leaked.
>
> Furthermore, if the recursive call to i3c_dev_request_ibi_locked() fails,
> the outer caller will execute:
>
> if (ret) {
> kfree(ibi);
> dev->ibi = NULL;
> }
>
> Could this result in a double-free on the original allocation?
look like make sense
Frank
>
> > + if (ret)
> > + i3c_master_direct_detach_i3c_dev_locked(desc);
> > +
> > + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> > +
> > + up_write(&parent->bus.lock);
> > +
> > + return ret;
> > +}
> > +
> > +static void i3c_hub_free_ibi(struct i3c_dev_desc *desc)
> > +{
> > + struct i3c_master_controller *controller = i3c_dev_get_master(desc);
> > + struct i3c_hub_controller *hub_controller;
> > + struct i3c_master_controller *orig_parent;
> > + struct i3c_master_controller *parent;
> > + struct i3c_hub *hub;
>
> [ ... ]
>
> > + i3c_hub_enable_port(controller);
> > +
> > + down_write(&parent->bus.lock);
> > + orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> > + i3c_dev_free_ibi_locked(desc);
> > + i3c_master_direct_detach_i3c_dev_locked(desc);
>
> [Severity: High]
> Does reparenting the device to the upstream master before detaching it
> destroy its address reservations?
>
> The detach logic calls i3c_master_put_i3c_addrs(), which resets the device's
> static and dynamic address slots to free. Because the device is temporarily
> parented to the upstream master, this frees the addresses on the parent bus.
>
> [Severity: High]
> Also, does the recursive call to i3c_dev_free_ibi_locked(desc) guarantee a
> NULL pointer dereference?
>
> The core i3c_dev_free_ibi_locked() function calls master->ops->free_ibi(dev).
> When this hub implementation recursively calls i3c_dev_free_ibi_locked(), it
> frees the IBI and sets desc->ibi = NULL. Upon returning to the outer function,
> it executes:
>
> if (dev->ibi->wq) {
> destroy_workqueue(dev->ibi->wq);
>
> which unconditionally dereferences the newly set NULL pointer.
>
> > + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> > + up_write(&parent->bus.lock);
> > +
> > + i3c_hub_disable_port(controller);
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260701065755.2067793-1-lakshay.piplani@nxp.com?part=5
More information about the linux-i3c
mailing list