Re: [PATCH v16 6/8] i3c: hub: Add support for the I3C interface in the I3C hub

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Aug 26, 2026 at 11:04:07AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] Freeing `i3c_hub_dev_data` while it may still be referenced by the parent controller's asynchronous IBI path leads to a Use-After-Free.
> - [High] Dynamic downstream devices discovered via DAA are erroneously registered to the parent controller's bus, making them completely unreachable for subsequent transfers on port-switching hubs.
> - [High] Reserved address slots on the parent bus are permanently leaked when the hub driver is removed.
> - [Medium] Forwarding broadcast CCCs to the parent controller breaks virtual bus isolation by unintentionally affecting the entire parent bus.
> --
>
> commit 94c62ce56dcc9d461749adc3db10a8061865dc9b
> 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..6f5227215ae22
> > --- /dev/null
> > +++ b/drivers/i3c/hub.c
> > @@ -0,0 +1,798 @@
> [ ... ]
> > +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: Critical]
> Does this code introduce a use-after-free when IBI is still active?
>
> If data->parent_desc.ibi is not NULL, as explicitly checked by the
> WARN_ON_ONCE, the parent controller's asynchronous IBI path may still
> have a work item queued referencing data->parent_desc. By unconditionally
> calling kfree(data), we allow the pending async work to dereference
> freed memory when it executes.
>
> > +}
> > +
> > +/**
> > + * i3c_hub_do_daa() - Perform DAA via hub port
> > + * @hub: Hub instance
> > + * @controller: Virtual controller for a hub port
> > + *
> > + * Enables the port connection, performs DAA on the parent controller,
> > + * then disables the connection.
> > + */
> > +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;
> > +
> > +	/*
> > +	 * Lock order: hub routing mutex before the parent bus lock (taken here
> > +	 * inside i3c_master_do_daa()). The depth-keyed lockdep classes above
> > +	 * keep this nesting acyclic when the parent is itself a hub.
> > +	 */
> > +	mutex_lock(&hub->lock);
> > +	i3c_hub_enable_port(controller);
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	ret = i3c_master_do_daa(parent);
>
> [Severity: High]
> Does delegating DAA to the physical parent controller break reachability
> for downstream devices?
>
> When devices respond to ENTDAA, they are instantiated on the parent bus
> rather than the virtual bus. If drivers subsequently initiate transfers, they
> use the parent controller's transfer operations directly, bypassing the
> hub's i3c_hub_master_priv_xfers(). As a result, the hub target port is
> never enabled during the transfer, leaving these downstream devices
> permanently unreachable.

where reorder port, is it true?

>
> > +	i3c_hub_disable_port(controller);
> > +	mutex_unlock(&hub->lock);
> > +
> > +	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;
> > +
> > +	mutex_lock(&hub->lock);
> > +	i3c_hub_enable_port(controller);
> > +	ret = i3c_master_send_ccc_cmd(parent, cmd);
>
> [Severity: Medium]
> Can forwarding unfiltered broadcast CCCs to the parent controller break
> virtual bus isolation?
>
> If a driver on a specific virtual bus issues broadcast commands (such as
> SETMRL, SETMWL, or ENEC), this forwards them to the parent controller which
> broadcasts them globally. This will unintentionally alter the configuration
> or event state of all peer devices on the parent bus and other open target
> ports.

Look like system design limiation, add comments here incase it happen.

>
> > +	i3c_hub_disable_port(controller);
> > +	mutex_unlock(&hub->lock);
> > +
> > +	return ret;
> > +}
> [ ... ]
> > +int i3c_hub_reserve_parent_addrslots_from_dt(struct i3c_hub_controller *hubc,
> > +					     struct device_node *node)
> > +{
> > +	struct i3c_master_controller *parent = hubc->parent;
> > +	enum i3c_addr_slot_status status;
> > +	u32 assigned_addr;
> > +	u32 reg[3];
> > +	int ret;
> > +
> > +	if (!parent || !node)
> > +		return -ENODEV;
> > +
> > +	for_each_available_child_of_node_scoped(node, child) {
> > +		/*
> > +		 * Only consider addressable bus nodes: a valid "reg" is
> > +		 * required to describe a device, but its static-address value
> > +		 * does not affect the reservation below.
> > +		 */
> > +		ret = of_property_read_variable_u32_array(child, "reg", reg, 1, 3);
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		ret = of_property_read_u32(child, "assigned-address", &assigned_addr);
> > +		if (ret)
> > +			continue;
> > +
> > +		/* Skip nodes without a usable dynamic address. */
> > +		if (!assigned_addr || assigned_addr > I3C_MAX_ADDR)
> > +			continue;
> > +
> > +		i3c_bus_maintenance_lock(&parent->bus);
> > +		status = i3c_bus_get_addr_slot_status(&parent->bus,
> > +						      assigned_addr);
> > +		if (status == I3C_ADDR_SLOT_FREE)
> > +			i3c_bus_set_addr_slot_status(&parent->bus,
> > +						     assigned_addr,
> > +						     I3C_ADDR_SLOT_I3C_DEV);
>
> [Severity: High]
> Does this code permanently leak parent bus address slots when the hub driver
> is removed?
>
> The address slots in the parent bus state are set to I3C_ADDR_SLOT_I3C_DEV.
> However, there appears to be no corresponding teardown mechanism provided
> to revert these slots to I3C_ADDR_SLOT_FREE when the virtual controllers
> are destroyed or the hub driver is unbound. This leak can eventually exhaust
> the address space and cause failures for subsequent devices.

I suppose if hub have hotplug later, we have to reserver all static/assigned
address. Can you add comments here.

Thank you for great work, fixed the most problem.

Frank
>
> > +		i3c_bus_maintenance_unlock(&parent->bus);
> > +	}
> > +	return 0;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260826103819.1614843-1-lakshay.piplani@nxp.com?part=6

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