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

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

New issues:
- [High] Lockless access to `i3cdev->desc` introduces a Time-of-Check to Time-of-Use (TOCTOU) race, Use-After-Free (UAF), and NULL pointer dereference when a device is concurrently matched and unregistered.
- [Medium] Missing OF and ACPI modalias generation in `i3c_device_uevent` breaks module autoloading for SETAASA-based I3C devices.

Pre-existing issues:
- [High] Matching logic reads uninitialized stack memory when i3cdev->desc is NULL.
--

commit 3dbcf3bf72698f3de90e327e59cfb2913ecd7503
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 be matched through Device Tree or ACPI.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index bcc9c2d29c342..1bd545447b817 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 this lockless access to i3cdev->desc cause a Use-After-Free or NULL
pointer dereference?

If the I3C master is unbound or unregistered concurrently,
i3c_master_unregister_i3c_devs() sets i3cdev->dev->desc = NULL locklessly and
calls device_unregister().

Since i3c_device_match() is called from bus_for_each_dev() without taking the
device lock, if the thread is preempted between checking i3cdev->desc and
reading desc->boardinfo, desc could be freed by i3c_master_bus_cleanup(),
causing a Use-After-Free.

Alternatively, without READ_ONCE(), could the compiler reload i3cdev->desc as
NULL, causing a direct NULL pointer dereference?

> +
> +	/*
> +	 * 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() operate on
uninitialized stack memory here?

If i3cdev->desc is NULL (which can happen concurrently during device teardown
via i3c_master_unregister_i3c_devs()), i3c_device_get_info() returns early
without modifying the devinfo output buffer:

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);
	...
}

This leaves devinfo uninitialized, causing the matching logic to extract
the manufacturer and part IDs from stack garbage.

>  		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]
Since this patch enables Device Tree and ACPI matching, does the bus uevent
callback (i3c_device_uevent()) also need to generate OF and ACPI modaliases?

Without of_device_uevent_modalias() or acpi_device_uevent_modalias(), udev
cannot automatically load the driver based on the DT compatible string or ACPI
HID, which breaks module autoloading for SETAASA-based devices.

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