Re: [PATCH v18 6/8] 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 2 potential issue(s) to consider:
- [High] Downstream devices discovered via ENTDAA are incorrectly instantiated on the parent bus, bypassing the hub's port routing logic.
- [High] Intentional Use-After-Free in `i3c_hub_detach_i3c_dev` when asynchronous IBI is still active.
--

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

i3c: hub: Add support for the I3C interface in the I3C hub
    
This commit adds virtual I3C bus support for the hub and provides an 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..5390287680844
> --- /dev/null
> +++ b/drivers/i3c/hub.c

[ ... ]

> +static void i3c_hub_detach_i3c_dev(struct i3c_dev_desc *dev)
> +{
> +	struct i3c_hub_dev_data *data = i3c_dev_get_master_data(dev);
> +	struct i3c_master_controller *parent;
> +
> +	if (!data)
> +		return;
> +
> +	parent = i3c_dev_get_master(&data->parent_desc);
> +
> +	/*
> +	 * parent_desc.ibi should already be cleared by i3c_hub_free_ibi()
> +	 * before we get here. If it is still set, the kfree(data) below frees
> +	 * a descriptor the parent controller can still reach via un-flushed
> +	 * asynchronous IBI work (use-after-free, not just a leak).
> +	 */
> +	WARN_ON_ONCE(data->parent_desc.ibi);
> +
> +	if (parent) {
> +		i3c_bus_maintenance_lock(&parent->bus);
> +		i3c_master_detach_i3c_dev_controller_locked(&data->parent_desc);
> +		i3c_bus_maintenance_unlock(&parent->bus);
> +	}
> +
> +	i3c_dev_set_master_data(dev, NULL);
> +	mutex_destroy(&data->parent_desc.ibi_lock);
> +	kfree(data);

[Severity: High]
Does this code introduce a use-after-free if the asynchronous IBI task is
still active?

When a downstream device is unbound while an IBI is active, the I3C core calls
i3c_hub_detach_i3c_dev() without first freeing the IBI. This occurs because
i3c_master_detach_free_devs() executes during i3c_master_bus_cleanup() without
clearing IBIs. 

While the warning correctly detects this state, calling kfree(data)
unconditionally frees the memory while the parent controller's un-flushed IBI
workqueue (i3c_master_handle_ibi) might still execute and dereference the
freed data->parent_desc.

Would it be safer to leak the allocation by returning early rather than freeing
actively referenced memory to avoid memory corruption?

> +}

[ ... ]

> +static int i3c_hub_do_daa(struct i3c_hub *hub,
> +			  struct i3c_master_controller *controller)
> +{
> +	struct i3c_master_controller *parent;
> +	int ret;

[ ... ]

> +	/*
> +	 * Downstream devices reachable through hub target-port routes share the
> +	 * parent controller's I3C address space. The hub gates access to a
> +	 * target-port network, but it does not create an independent dynamic
> +	 * address domain per virtual bus.
> +	 *
> +	 * Run DAA on the parent controller so dynamic addresses remain unique
> +	 * across all downstream devices, even when they are behind different
> +	 * target ports.
> +	 *
> +	 * The current hub model shares the parent controller's dynamic-address
> +	 * domain and does not support independent per-port DAA domains, so a
> +	 * DAA result does not go stale behind a port toggle. Addresses reserved
> +	 * from the Device Tree are held for the lifetime of the parent bus and
> +	 * are never released on a port toggle, so enabling or disabling a port
> +	 * only gates reachability and never causes an address to be handed out
> +	 * twice.
> +	 */
> +	ret = i3c_master_do_daa(parent);

[Severity: High]
Can delegating DAA entirely to the parent controller here cause downstream
devices to bypass the hub port routing logic?

If a downstream device lacks an assigned-address or static address in DT, it
is skipped by i3c_master_early_i3c_dev_add() and left to be discovered
dynamically. When i3c_master_do_daa() executes on the parent, the parent
controller performs ENTDAA, discovers the downstream device, and instantiates
the i3c_device on parent->bus.devs rather than the virtual controller's bus.

This would cause target drivers to bind to the device on the parent bus. Their
transfer requests would be routed directly to the parent controller, completely
bypassing i3c_hub_master_priv_xfers() and leaving the hub port closed, which
would cause wire transfers to fail.

> +	i3c_hub_disable_port(controller);
> +	mutex_unlock(&hub->lock);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903062108.2712296-1-lakshay.piplani@nxp.com?part=6

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