Re: [PATCH v10 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 10 potential issue(s) to consider:

New issues:
- [Critical] Unconditional NULL pointer dereference in `i3c_dev_free_ibi_locked()` via recursive `i3c_hub_free_ibi()`.
- [High] Missing `up_write(&parent->bus.lock)` release on the error path in `i3c_hub_request_ibi()`.
- [High] Double free of `desc->ibi` and leaked workqueue on the error path in `i3c_hub_request_ibi()`.
- [High] `i3c_hub_master_priv_xfers()` modifies shared hub device state and parent bus topology without holding the parent controller's bus lock.
- [High] Bus address slot tracking is bypassed in `i3c_hub_master_priv_xfers()` due to identical address arguments.
- [High] Failure to restore the hub device's dynamic address (`hub_dev->info.dyn_addr`) on the reattach error path in `i3c_hub_master_priv_xfers()`.
- [High] Architectural flaw: DAA logic incorrectly delegates directly to the parent controller, permanently bypassing the virtual hub.
- [High] Missing `i3c_master_direct_detach_i3c_dev_locked()` on the error path in `i3c_hub_request_ibi()`.
- [Medium] Bitwise OR incorrectly used to combine signed error codes, resulting in corrupted errno values.

Pre-existing issues:
- [High] Leaked workqueue on the error path of IBI requests in the I3C master core.
--

commit 75489fdfdbca4c7d1708dfb61bdd09e7018b9a0b
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 by creating virtual master controllers
for downstream ports and forwarding bus operations through the hub device. It
also provides an interface used by hub drivers to enable or disable downstream
ports during bus transactions.

> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 0000000000000..da0e209ee5b79
> --- /dev/null
> +++ b/drivers/i3c/hub.c
[ ... ]
> @@ -54,26 +54,26 @@ static void i3c_hub_detach_i3c_dev(struct i3c_dev_desc *dev)
>   */
>  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);
>  	i3c_hub_disable_port(controller);

[Severity: High]
Does this discovery logic bypass the virtual hub permanently?

When calling i3c_master_do_daa(parent), newly discovered downstream devices
are attached directly to the parent controller's bus. Future transfers or
IBI requests targeting these devices will use the parent controller directly
instead of i3c_hub_master_priv_xfers().

Because of this, will i3c_hub_enable_port() never be invoked for these
devices, leaving them unreachable?

[ ... ]
> @@ -126,45 +126,45 @@ static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
>   */
>  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;
>  	u8 hub_addr, target_addr;
>  	struct i3c_hub *hub;
>  	int ret;
>  
>  	hub_controller = dev_get_drvdata(&controller->dev);
>  	if (!hub_controller || !hub_controller->hub)
>  		return -ENODEV;
>  
>  	hub = hub_controller->hub;
>  
>  	if (!hub->hub_dev)
>  		return -ENODEV;
>  
>  	hub_dev = hub->hub_dev->desc;
>  
>  	i3c_hub_enable_port(controller);
>  
>  	hub_addr = hub_dev->info.dyn_addr ?
>  		   hub_dev->info.dyn_addr : hub_dev->info.static_addr;
>  
>  	target_addr = dev->info.dyn_addr ?
>  		      dev->info.dyn_addr : dev->info.static_addr;
>  
>  	if (hub_addr != target_addr) {
>  		hub_dev->info.dyn_addr = target_addr;
>  		ret = i3c_master_reattach_i3c_dev_locked(hub_dev, target_addr);

[Severity: High]
Can this sequence cause a race condition when modifying shared hub state?

This code modifies hub_dev->info.dyn_addr and invokes
i3c_master_reattach_i3c_dev_locked() without holding parent->bus.lock for
writing.

If multiple virtual master controllers (hub ports) initiate transfers
concurrently, might threads race to manipulate these shared hardware address
tables and enable multiple downstream ports simultaneously without
arbitration, leading to physical bus collisions?

[Severity: High]
Will bus address slot tracking be bypassed here?

The code sets hub_dev->info.dyn_addr = target_addr and then passes
target_addr as the old_dyn_addr argument to
i3c_master_reattach_i3c_dev_locked().

Inside the core function, the condition (dev->info.dyn_addr != old_dyn_addr)
will evaluate to false, which skips updating the addr_slot_status tracking
array. Could this permanently corrupt the parent controller's view of free
and used bus addresses?

>  		if (ret)
>  			goto disable;

[Severity: High]
Should the dynamic address be restored on this error path?

If i3c_master_reattach_i3c_dev_locked() fails, the code jumps to the disable
label and returns. Since hub_dev->info.dyn_addr is not restored to hub_addr,
does this leave the software descriptor desynchronized from the actual
hardware state?

>  	}
>  
>  	ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
>  
>  	if (hub_addr != target_addr) {
>  		hub_dev->info.dyn_addr = hub_addr;
>  		ret |= i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
>  	}
>  
>  disable:

[Severity: Medium]
Is it safe to use a bitwise OR to combine these error codes?

Both i3c_device_do_xfers() and i3c_master_reattach_i3c_dev_locked() can
return negative error codes like -EAGAIN or -EBUSY. Performing a bitwise OR
combines these signed values, which can corrupt the resulting error code
and return a nonsensical status to the caller.

[ ... ]
> @@ -250,45 +250,45 @@ static bool i3c_hub_master_supports_ccc_cmd(struct i3c_master_controller *contro
>   */
>  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;
>  
>  	hub_controller = dev_get_drvdata(&controller->dev);
>  	if (!hub_controller || !hub_controller->hub)
>  		return -ENODEV;
>  
>  	hub = hub_controller->hub;
>  
>  	if (!hub->hub_dev)
>  		return -ENODEV;
>  
>  	parent = i3c_dev_get_master(hub->hub_dev->desc);
>  
>  	down_write(&parent->bus.lock);
>  
>  	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
>  
>  	ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
>  	if (ret) {
>  		i3c_hub_update_desc_parent(&desc->common, orig_parent);
>  		return ret;

[Severity: High]
Is a lock release missing on this error path?

A write lock is acquired on parent->bus.lock, but if the attachment fails,
the function returns immediately. Should up_write(&parent->bus.lock) be
called here to prevent a system deadlock on future parent bus operations?

>  	}
>  
>  	kfree(desc->ibi);
>  	desc->ibi = NULL;

[Severity: High]
Could this lead to a double free and a resource leak?

When i3c_dev_request_ibi_locked() is called, it allocates desc->ibi and a
workqueue. This code blindly calls kfree(desc->ibi), which leaves the
workqueue memory leaked.

Furthermore, if the inner call to i3c_dev_request_ibi_locked() fails, it
sets desc->ibi to NULL. The outer i3c_dev_request_ibi_locked() will then
receive the error and execute kfree(ibi) on the original pointer that was
already freed, leading to a double free.

>  	ret = i3c_dev_request_ibi_locked(desc, req);

[Severity: High]
If this recursive call fails, should the device be detached?

The downstream device is directly attached to the parent hardware controller.
If i3c_dev_request_ibi_locked() fails, the function restores the descriptor
and returns without detaching the device from the parent. Since free_ibi
will never be called to clean up a failed request, does this permanently
leak the hardware attachment resources in the parent controller?

[Severity: High]
This isn't a bug introduced by this patch, but does the core I3C code leak a
workqueue on this error path?

In drivers/i3c/master.c:i3c_dev_request_ibi_locked(), an ordered workqueue
is allocated and assigned to ibi->wq. If the controller's request_ibi
callback fails, the code frees the ibi context but does not destroy the
workqueue, leading to a memory and kernel resource leak.

>  
>  	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;
>  
>  	hub_controller = dev_get_drvdata(&controller->dev);
>  	if (!hub_controller || !hub_controller->hub)
>  		return;
>  
>  	hub = hub_controller->hub;
>  
>  	parent = i3c_dev_get_master(hub->hub_dev->desc);
>  
>  	i3c_hub_enable_port(controller);
>  
>  	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: Critical]
Does this recursive call result in a guaranteed NULL pointer dereference?

When the core invokes the hub's free_ibi callback, this function recursively
calls i3c_dev_free_ibi_locked() using the parent context. The inner call
frees dev->ibi and sets dev->ibi = NULL.

Upon returning to the outer i3c_dev_free_ibi_locked(), the core immediately
evaluates if (dev->ibi->wq). Since dev->ibi was just set to NULL, won't this
trigger a NULL pointer dereference on the success path?

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