Re: [PATCH v11 7/9] i3c: hub: Add support for the I3C interface in the I3C hub

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Concurrent private transfers through the hub race on `hub_dev->info.dyn_addr`, causing data corruption.
- [Critical] Delegating DAA to the parent controller incorrectly attaches downstream devices to the parent bus.
- [Critical] Unsynchronized hub port enablement breaks virtual bus isolation, causing bus collisions.
- [Critical] `i3c_hub_master_priv_xfers` corrupts the parent bus address slots by freeing slots that belong to unrelated physical devices.
- [High] Double free and memory leak of the IBI workqueue in `i3c_hub_request_ibi` error path.
- [High] Deterministic NULL pointer dereference in `i3c_dev_free_ibi_locked` after hub recursive call.
- [High] Unlocked write to `hub_dev->info.dyn_addr` introduces a data race on the error path.
- [High] Failure in `priv_xfers` address reattachment permanently orphans the hub device.
- [High] IBI support for downstream hub devices is broken because devices are never added to the physical parent's bus list.
- [High] Lockdep splat due to nested acquisition of `bus.lock` without subclass annotations.
- [Medium] I2C transfers silently succeed without performing any communication, masking failures.
--

commit f3f90aaf2c0040d96007fed285ccd49e4c6e8e7a
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

i3c: hub: Add support for the I3C interface in the I3C hub

Add virtual I3C bus support for the hub and provide interface to enable
or disable downstream ports.

> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 0000000000000..c37b01fddcb4c
> --- /dev/null
> +++ b/drivers/i3c/hub.c

[ ... ]

> +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);

[Severity: Critical]
Does enabling the port outside of the parent controller's bus lock break
virtual bus isolation? If multiple virtual ports execute global broadcasts
concurrently, they might both enable their respective hardware ports before
blocking on parent->bus.lock. This could cause the parent to execute the
broadcast across all enabled ports simultaneously, potentially resulting in
severe bus collisions.

> +	ret = i3c_master_do_daa(parent);

[Severity: Critical]
Could delegating the Dynamic Address Assignment procedure to the physical
parent controller incorrectly attach downstream devices to the parent bus?

Since i3c_master_do_daa() natively creates and attaches devices to the
physical parent bus, any transfers targeting these devices would bypass the
hub's virtual controller callbacks. As a result, i3c_hub_enable_port()
would never be invoked prior to communication, potentially causing I3C
transfers to the downstream devices to fail.

> +	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);

[Severity: Critical]
Is it possible for concurrent CCC broadcasts to cause bus collisions here?
Similar to i3c_hub_do_daa(), enabling the port outside of parent->bus.lock
could allow multiple ports to be enabled simultaneously if they execute
broadcasts concurrently.

> +	ret = i3c_master_send_ccc_cmd(parent, cmd);
> +	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);

[Severity: High]
Does acquiring this lock cause a lockdep splat?

This function is invoked from contexts where the core framework has already
locked the virtual downstream controller's bus.lock (e.g., via
i3c_bus_normaluse_lock() which takes a down_read). Acquiring a second lock
of the same class without using down_write_nested() might violate lock
nesting rules.

> +		hub_dev->info.dyn_addr = target_addr;
> +
> +		ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
> +		up_write(&parent->bus.lock);
> +		if (ret) {
> +			hub_dev->info.dyn_addr = hub_addr;

[Severity: High]
Is it safe to modify hub_dev->info.dyn_addr outside of the bus write lock?
Modifying this shared bus state variable after up_write(&parent->bus.lock)
appears to introduce a data race with concurrent bus operations.

[Severity: High]
If i3c_master_reattach_i3c_dev_locked() fails, are we permanently orphaning
the hub device?

When it fails, the core code automatically marks both the new and old address
slots as I3C_ADDR_SLOT_FREE via i3c_master_put_i3c_addrs(). Because this
error path restores dyn_addr but fails to reinstate the address allocation
on the parent bus, the parent controller might re-assign the hub's legitimate
address to new hot-joined devices.

> +			goto disable;
> +		}
> +	}
> +
> +	ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);

[Severity: Critical]
Could concurrent private transfers through the hub race on
hub_dev->info.dyn_addr?

This function drops the write lock before calling i3c_device_do_xfers(),
which only acquires the lock in read mode. If multiple threads concurrently
initiate transfers to different downstream ports, they might take the read
lock simultaneously and race to overwrite hub_dev->info.dyn_addr, potentially
causing transfers to be misrouted.

> +
> +	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: Critical]
Does passing target_addr as the old dynamic address corrupt the parent bus
address slots?

When the core function interprets target_addr as the old dynamic address of
the device on the parent bus:

drivers/i3c/master.c:i3c_master_reattach_i3c_dev_locked() {
    ...
    if (old_dyn_addr)
        i3c_bus_set_addr_slot_status(&master->bus, old_dyn_addr,
                                     I3C_ADDR_SLOT_FREE);
    ...
}

Because target_addr is a downstream address independently assigned by the
virtual controller, this might blindly free an in-use address slot on the
parent bus that belongs to an unrelated physical device.

> +
> +		up_write(&parent->bus.lock);
> +		if (!ret)
> +			ret = xfer_ret;
> +	}
> +disable:
> +	i3c_hub_disable_port(controller);
> +	return ret;
> +}

[ ... ]

> +static int i3c_hub_i2c_xfers(struct i2c_dev_desc *dev,
> +			     struct i2c_msg *xfers, int nxfers)
> +{
> +	return 0;
> +}

[Severity: Medium]
Should this callback attempt to forward the I2C transfers through the hub to
the parent bus, or return an error like -EOPNOTSUPP? Unconditionally
returning 0 might lead the kernel and userspace applications to falsely
assume that I2C transactions to downstream devices were successful when no
actual hardware communication occurred.

[ ... ]

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

[ ... ]

> +	parent = i3c_dev_get_master(hub->hub_dev->desc);
> +
> +	down_write(&parent->bus.lock);

[Severity: High]
Does acquiring parent->bus.lock here cause a lockdep splat? As with
i3c_hub_master_priv_xfers(), this might acquire a second lock of the
same class without using down_write_nested().

> +
> +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> +
> +	ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);

[Severity: High]
Are downstream devices missing from the physical parent's bus list?

i3c_master_direct_attach_i3c_dev_locked() explicitly skips adding the device
to the parent's bus.devs.i3c list. When the hardware subsequently receives an
IBI, the core framework (i3c_master_handle_ibi) might look up the source
device by matching the IBI address against its bus.devs.i3c list. Since the
downstream device is missing from the list, the lookup would predictably
fail, causing IBIs to be ignored.

> +	if (ret) {
> +		i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +		up_write(&parent->bus.lock);
> +		return ret;
> +	}
> +
> +	kfree(desc->ibi);
> +	desc->ibi = NULL;

[Severity: High]
Are we leaking the IBI workqueue here?

When i3c_dev_request_ibi_locked() handles an IBI request, it allocates a
new ibi structure and a workqueue (ibi->wq). Freeing desc->ibi without
destroying ibi->wq seems to leak the workqueue.

Furthermore, if the nested i3c_dev_request_ibi_locked() call fails, this
hub callback will return an error to the outer caller. The outer caller's
error handling path might then unconditionally execute kfree(ibi) using its
original local pointer:

drivers/i3c/master.c:i3c_dev_request_ibi_locked() {
    ...
    ret = master->ops->request_ibi(dev, req);
    if (ret) {
        kfree(ibi);
        dev->ibi = NULL;
    }
    ...
}

Since the hub callback already freed it, wouldn't this result in a
double-free?

> +	ret = i3c_dev_request_ibi_locked(desc, req);
> +	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;

[ ... ]

> +	down_write(&parent->bus.lock);
> +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> +	i3c_master_direct_detach_i3c_dev_locked(desc);
> +	i3c_dev_free_ibi_locked(desc);

[Severity: High]
Does this nested call to i3c_dev_free_ibi_locked() cause a NULL pointer
dereference upon returning to the outer caller?

The inner call tears down the IBI state and sets desc->ibi = NULL.
When control returns to the outer i3c_dev_free_ibi_locked():

drivers/i3c/master.c:i3c_dev_free_ibi_locked() {
    ...
    master->ops->free_ibi(dev);

    if (dev->ibi->wq) {
    ...
}

It seems to unconditionally evaluate dev->ibi->wq immediately after the
callback returns, which would blindly dereference the newly nullified
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/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=7

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