Re: [PATCH v11 3/9] i3c: master: Add APIs for I3C hub support

On Fri, Jun 12, 2026 at 11:48:43AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] i3c_master_direct_detach_i3c_dev_locked lacks the unaddressable device check present in its attach counterpart, leading to a NULL pointer dereference in controller drivers.
> --
>
> commit a7f74140d15672b8f2824666071ce06e3051e0d8
> 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 to prepare for I3C hub support.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index ec3547f75469e..59e0f82de6ebd 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> > +					    struct i3c_dev_desc *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * We don't attach devices to the controller until they are
> > +	 * addressable on the bus.
> > +	 */
> > +
> > +	if (!dev->info.static_addr && !dev->info.dyn_addr)
> > +		return 0;
> > +
> > +	/* Do not attach the master device itself. */
> > +	if (master->this != dev && master->ops->attach_i3c_dev)
> > +		ret = master->ops->attach_i3c_dev(dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
>
> [ ... ]
>
> > +void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
> > +{
> > +	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > +
> > +	/* Do not detach the master device itself. */
> > +	if (master->this != dev && master->ops->detach_i3c_dev)
> > +		master->ops->detach_i3c_dev(dev);
>
> [Severity: High]
> Does this missing address check in the detach helper lead to a NULL pointer
> dereference in controller drivers?
>
> The attach helper i3c_master_direct_attach_i3c_dev_locked() gracefully skips
> attaching devices without a static or dynamic address and returns 0 (success).
> Because it returns success, callers might naturally invoke this companion
> detach API during teardown for these same devices.
>
> However, since this helper lacks the corresponding address check, it
> unconditionally invokes the controller's detach callback. Controller drivers
> often retrieve the device's private data and immediately dereference it. Since
> the attach phase was skipped, this private data would be NULL.
>
> Should this function also check if the device has an address before calling
> the detach callback?

Make sense, please fix it.

Frank

>
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

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