Re: [PATCH v7 06/12] i3c: master: match I3C device through DT and ACPI

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized stack memory read and kernel stack information leak during device unregistration when `i3cdev->desc` is NULL.
- [High] NULL pointer dereference in sysfs attribute show functions when accessed concurrently with device removal.
--

--- Patch [6]: [PATCH v7 06/12] i3c: master: match I3C device through DT and ACPI ---
commit d545f133fafb1ddd78e75d979b76dc143ac8e0b0
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: master: match I3C device through DT and ACPI

This commit updates i3c_device_match() to allow SETAASA-based I3C devices
to be matched through Device Tree or ACPI. It checks if the device has a
SETAASA static address method and uses of_driver_match_device() or
acpi_driver_match_device() as fallbacks when standard ID matching is not
applicable.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 488177d95e484..f94e2410d37ba 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -345,15 +346,32 @@ static int i3c_device_match(struct device *dev, const struct device_driver *drv)
>  {
>  	struct i3c_device *i3cdev;
>  	const struct i3c_driver *i3cdrv;
> +	u8 static_addr_method = 0;
>  
>  	if (dev->type != &i3c_device_type)
>  		return 0;
>  
>  	i3cdev = dev_to_i3cdev(dev);
>  	i3cdrv = drv_to_i3cdrv(drv);
> -	if (i3c_device_match_id(i3cdev, i3cdrv->id_table))
> +
> +	if (i3cdev->desc && i3cdev->desc->boardinfo)
> +		static_addr_method = i3cdev->desc->boardinfo->static_addr_method;
> +
> +	/*
> +	 * SETAASA-based devices need not always have a matching ID since
> +	 * it is not mandatory for such devices to implement deviceinfo
> +	 * CCC commands. Allow them to register through DT or ACPI.
> +	 */
> +	if (i3cdrv->id_table && i3c_device_match_id(i3cdev, i3cdrv->id_table))

[Severity: High]
This is a pre-existing issue, but can i3c_device_match_id() read
uninitialized stack memory here?

During device unregistration, i3c_master_unregister_i3c_devs() explicitly
sets i3cdev->dev->desc = NULL before calling device_unregister().

If driver matching occurs concurrently, i3c_device_match_id() might be called
while desc is NULL:

drivers/i3c/device.c:i3c_device_match_id() {
    struct i3c_device_info devinfo;
    ...
    i3c_device_get_info(i3cdev, &devinfo);
    manuf = I3C_PID_MANUF_ID(devinfo.pid);
    ...
}

Because i3c_device_get_info() does not initialize devinfo when desc is NULL,
this leaves stack memory to be read unconditionally and used in the matching
logic. This same uninitialized memory can also leak to userspace through the
uevent MODALIAS string in i3c_device_uevent() when desc is NULL.

Should i3c_device_match_id() be guarded by checking i3cdev->desc, similar
to how static_addr_method is guarded above?

>  		return 1;
>  
> +	if (static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		if (of_driver_match_device(dev, drv))
> +			return 1;
> +		if (acpi_driver_match_device(dev, drv))
> +			return 1;
> +	}
> +
>  	return 0;
>  }

Also, while reviewing this, I noticed another pre-existing issue during device
unregistration:

[Severity: High]
This isn't a bug introduced by this patch, but can bcr_show() and dcr_show()
dereference a NULL pointer?

When i3c_master_unregister_i3c_devs() clears i3cdev->dev->desc = NULL, there
is a race window before the sysfs files are removed in device_del(). If a
user reads the bcr or dcr sysfs attributes during this window,
dev_to_i3cdesc() will return NULL:

drivers/i3c/master.c:bcr_show() {
    ...
    desc = dev_to_i3cdesc(dev);
    ret = sysfs_emit(buf, "0x%02x\n", desc->info.bcr);
    ...
}

Could this lead to a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728065955.809445-1-akhilrajeev@nvidia.com?part=6