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

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

New issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition leading to a NULL pointer dereference during device matching.
- [High] Missing OF and ACPI MODALIAS generation in the bus uevent callback, preventing module autoloading for newly supported devices.
- [Medium] Latent API trap: DT/ACPI matching bypasses the I3C ID table match, causing a NULL pointer dereference in driver probe functions.

Pre-existing issues:
- [High] Uninitialized kernel stack memory is deterministically leaked to user-space via the `KOBJ_REMOVE` uevent.
--

--- Patch [6]: [PATCH v6 06/12] i3c: master: match I3C device through DT and ACPI ---
commit 7db2eb24719ceb614bd3ec6e5efb369d3b92fe6e
Author: Akhil R <[email protected]>

i3c: master: match I3C device through DT and ACPI

SETAASA-based devices cannot always be identified by PID or DCR; the
standard I3C id_table matching may not be applicable. Allow such devices to
match through Device Tree or ACPI.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 7b2f819bf4437..43bf530bc6619 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;

[Severity: High]
Can i3cdev->desc be modified concurrently here?

Looking at i3c_master_unregister_i3c_devs(), it locklessly sets
i3cdev->dev->desc = NULL during device unregistration.

Since the check in i3c_device_match() is done without locks or READ_ONCE(),
could the compiler emit multiple loads for i3cdev->desc, leading to a NULL
pointer dereference when subsequently accessing boardinfo?

> +
> +	/*
> +	 * 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))
>  		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;
> +	}

[Severity: Medium]
Will this fallback matching break the assumptions in existing driver probe
functions?

Currently, drivers typically call i3c_device_match_id() in their probe
routine and unconditionally dereference the returned pointer.

When an I3C device binds via DT or ACPI through this new path,
i3c_device_match_id() will return NULL. If developers add an of_match_table
to existing drivers to support SETAASA devices, won't this cause an immediate
NULL pointer dereference during probe?

[Severity: High]
How does this change affect module autoloading for newly supported devices?

While the patch allows devices to match via DT or ACPI, it appears the bus
uevent callback, i3c_device_uevent(), was not updated and only emits the
generic I3C modalias.

Since of_device_uevent_modalias() or acpi_device_uevent_modalias() are not
called, won't the uevent lack the required of:N... string, preventing udev
from matching the device against the driver's MODULE_DEVICE_TABLE(of, ...)?

[Severity: High]
This is a pre-existing issue, but can the removal of an I3C device leak
uninitialized kernel stack memory to user-space?

When an I3C device is removed, i3c_master_unregister_i3c_devs() sets
i3cdev->dev->desc = NULL before calling device_unregister(), which
triggers a KOBJ_REMOVE uevent.

In i3c_device_uevent():

	struct i3c_device_info devinfo;

	if (i3cdev->desc)
		devinfo = i3cdev->desc->info;

Since i3cdev->desc is unconditionally NULL during removal, devinfo is left
uninitialized. The function then proceeds to construct the MODALIAS string
using devinfo:

	return add_uevent_var(env,
			      "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04Xext%04X",
			      devinfo.dcr, manuf, part, ext);

Doesn't this deterministically embed uninitialized kernel stack data into the
uevent environment variables, exposing it to user-space programs like udev?

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

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