Re: [PATCH V5 02/14] i3c: Fix unlocked dereference of dev->desc in i3c_device_get_supported_xfer_mode()

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

Pre-existing issues:
- [High] Unresolved lockless dereferences of `i3cdev->desc` and uninitialized stack memory leaks to user-space.
- [High] I3C master controllers are allocated using `devm_kzalloc` while embedding a `struct device`, leading to a Use-After-Free during unbinding.
--

--- Patch [2]: [PATCH V5 02/14] i3c: Fix unlocked dereference of dev->desc in i3c_device_get_supported_xfer_mode() ---
commit e7d56c173e65ff36b79485393042fbeb29cba21d
Author: Adrian Hunter <[email protected]>

i3c: Fix unlocked dereference of dev->desc in i3c_device_get_supported_xfer_mode()

This patch resolves an unlocked dereference of dev->desc by using dev->bus
to obtain the master controller instead. The commit notes that dev->desc
must not be dereferenced unless bus->lock is held.

> i3c_device_get_supported_xfer_mode() uses dev->desc to obtain the
> master controller.  However, dev->desc must not be dereferenced unless
> bus->lock is held, and this function does not take that lock.

[Severity: High]
This isn't a bug introduced by this patch, but since we are establishing
that dev->desc must not be dereferenced without bus->lock, are there
similar unresolved lockless dereferences elsewhere?

For example, looking at drivers/i3c/master.c:i3c_device_uevent():

	if (i3cdev->desc)
		devinfo = i3cdev->desc->info;
	manuf = I3C_PID_MANUF_ID(devinfo.pid);

If this races with descriptor freeing (such as during DAA/Hot-Join events
that swap or free descriptors), could this cause a Use-After-Free?

Additionally, if i3cdev->desc is NULL, devinfo remains completely
uninitialized on the stack. The function then unconditionally formats
devinfo.pid and devinfo.dcr into the MODALIAS string, which is exposed to
user-space via the uevent sysfs file. Could this leak kernel stack memory?

Similar lockless or NULL-unsafe accesses appear to exist in
drivers/i3c/device.c:i3c_device_match_id(),
drivers/i3c/master.c:i3c_device_match(), and driver probes like
drivers/misc/amd-sbi/rmi-i2c.c:sbrmi_i3c_probe().

[ ... ]

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index d2fb1a1105218..c7bb52b71d884 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -102,12 +102,6 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
>  	up_read(&bus->lock);
>  }
>  
> -static struct i3c_master_controller *
> -i3c_bus_to_i3c_master(struct i3c_bus *i3cbus)
> -{
> -	return container_of(i3cbus, struct i3c_master_controller, bus);
> -}
> -
>  static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev)
>  {
>  	return container_of(dev, struct i3c_master_controller, dev);

[Severity: High]
This is a pre-existing issue, but looking at the master controller lifecycle
managed here, is there a Use-After-Free risk during unbinding?

Almost all I3C master drivers (e.g. drivers/i3c/master/dw-i3c-master.c)
allocate their master controller structure using devm_kzalloc():

drivers/i3c/master/dw-i3c-master.c:dw_i3c_probe() {
...
	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
...
}

This structure embeds a struct i3c_master_controller, which in turn embeds a
struct device (accessed right here via dev_to_i3cmaster()).

When the driver is unbound, devres automatically frees the devm_kzalloc
memory once the remove callback finishes. If a user holds a sysfs attribute
open, the device reference count doesn't immediately drop to zero. Later,
when the sysfs file is closed, the driver core invokes the device's release
callback:

drivers/i3c/master.c:i3c_masterdev_release() {
...
	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
...
	if (master->wq)
		destroy_workqueue(master->wq);
...
}

Since the memory was already freed by devres, won't accessing master->wq
here trigger a Use-After-Free?

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

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