Re: [PATCH v14 1/8] i3c: master: Add APIs for I3C hub support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Asymmetrical cleanup in `i3c_master_direct_detach_i3c_dev_locked` leaks resources for unaddressed devices.

Pre-existing issues:
- [High] IDR entry deletion on `i3c_bus_init` failure causes ID corruption and sysfs collisions.
- [High] Missing NULL check for `dev->boardinfo` in `i3c_master_get_i3c_addrs` leads to kernel panic.
--

--- Patch [1]: [PATCH v14 1/8] i3c: master: Add APIs for I3C hub support ---
commit 1db519c7da22819d977429d77c0d05ba94daedf9
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>

i3c: master: Add APIs for I3C hub support

Add helpers for attaching and detaching I3C devices and CCC helpers
to check CCC support and send CCC commands, address slot helpers to
query and update I3C bus address slot state, registering virtual
masters with an explicit firmware node, and exposing the bus maintenance
lock helpers.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..3b1c342be2bc5 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1775,6 +1792,83 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
>  	return 0;
>  }
>  
> +/**
> + * i3c_master_direct_attach_i3c_dev_locked() - Attach an I3C device to a master
> + * @master: I3C master controller to attach the device to
> + * @dev: I3C device descriptor representing the device
> + *
> + * Attach an I3C device to its master controller after reserving the device
> + * addresses on the bus. The device must belong to @master.
> + *
> + * The master device itself is not passed to the controller driver's
> + * ->attach_i3c_dev() callback.
> + *
> + * Context: Caller must hold @master->bus.lock in write mode.
> + *
> + * Return: 0 on success, or a negative error code if address reservation fails
> + * or if the master controller driver's ->attach_i3c_dev() callback fails.
> + */
> +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> +					    struct i3c_dev_desc *dev)
> +{
> +	int ret;
> +
> +	if (WARN_ON(i3c_dev_get_master(dev) != master))
> +		return -EINVAL;
> +
> +	ret = i3c_master_get_i3c_addrs(dev);

[Severity: High]
This is a pre-existing issue, but does exposing this new API introduce a
potential NULL pointer dereference?

If a caller manually constructs a virtual I3C device without an OF node
where dev->boardinfo is NULL, but it has a static_addr, calling this
function will invoke i3c_master_get_i3c_addrs(). Inside that function,
if the static address slot is not free:

i3c_master_get_i3c_addrs() {
    ...
    status = i3c_bus_get_addr_slot_status(&master->bus,
                                          dev->info.static_addr);
    if (status != I3C_ADDR_SLOT_FREE &&
        dev->info.static_addr != dev->boardinfo->init_dyn_addr)
        return -EBUSY;
    ...
}

Could this dereference dev->boardinfo without checking if it is NULL, leading
to a kernel panic?

> +	if (ret)
> +		return ret;
> +
> +	/* Do not attach the master device itself. */
> +	if (master->this != dev && master->ops->attach_i3c_dev) {
> +		ret = master->ops->attach_i3c_dev(dev);

[Severity: High]
Could this cause an asymmetrical resource leak for unaddressed devices?

The original i3c_master_attach_i3c_dev() function includes a check at the
beginning to return early if the device has no static or dynamic address:

if (!dev->info.static_addr && !dev->info.dyn_addr)
    return 0;

This new attach function lacks that check, meaning it invokes the master
controller's attach_i3c_dev() hook even for unaddressed devices.

However, the new i3c_master_direct_detach_i3c_dev_locked() function
explicitly skips detach for unaddressed devices:

if (!dev->info.static_addr && !dev->info.dyn_addr)
    return;

Will any resources allocated by the controller driver during the attach
hook be permanently leaked when the device is detached?

> +		if (ret) {
> +			i3c_master_put_i3c_addrs(dev);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);

[ ... ]

> @@ -3195,34 +3342,31 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)

[ ... ]

> +int i3c_master_register_fwnode(struct i3c_master_controller *master,
> +			       struct device *parent,
> +			       struct fwnode_handle *fwnode,
> +			       const struct i3c_master_controller_ops *ops,
> +			       bool secondary)
> +{
> +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_MAX_RATE;
> +	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
> +	enum i3c_bus_mode mode = I3C_BUS_MODE_PURE;
> +	struct device_node *np = NULL;
> +	struct i2c_dev_boardinfo *i2cbi;
> +	int ret;

[ ... ]

> +	ret = i3c_bus_init(i3cbus, master->dev.of_node);
> +	if (ret)
> +		goto err_put_dev;

[Severity: High]
This isn't a bug introduced by this patch, but does an error here cause
global IDR corruption?

If i3c_bus_init() fails during idr_alloc(), it returns an error with
i3cbus->id still set to 0. Taking the err_put_dev path calls put_device() on
master->dev, which invokes i3c_masterdev_release(). 

i3c_masterdev_release() unconditionally calls i3c_bus_cleanup(), which runs:

i3c_bus_cleanup() {
    ...
    idr_remove(&i3c_bus_idr, i3cbus->id);
    ...
}

If i3cbus->id is uninitialized (0), does this mistakenly remove ID 0 from
the global i3c_bus_idr? If a valid bus was already registered with ID 0, it
seems it would lose its IDR entry, causing a subsequent bus registration to
be assigned ID 0 again and triggering sysfs collisions (i3c-0).

> +
> +	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> +
> +	ret = of_populate_i3c_bus(master);
> +	if (ret)
> +		goto err_put_dev;

[ ... ]

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