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

On Tue, Jul 14, 2026 at 10:03:22AM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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.

Please check it ?

Frank

>
> 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/[email protected]?part=1

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c