Re: [PATCH v16 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 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.

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

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

> +		i3c_bus_maintenance_unlock(&parent->bus);
> +	}
> +	return 0;
> +}

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