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

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?

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