Re: [PATCH V3 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 3 potential issue(s) to consider:

New issues:
- [Critical] The patch transforms a predictable NULL pointer dereference into an exploitable Use-After-Free of `master->this` when an I3C device outlives its master controller's unregistration.

Pre-existing issues:
- [High] Uninitialized stack memory (`devinfo`) is used for driver matching when `i3cdev->desc` is NULL.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition exists when accessing `i3cdev->desc->boardinfo`, leading to a Use-After-Free or NULL pointer dereference.
--

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

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

This patch modifies i3c_device_get_supported_xfer_mode() to use dev->bus
instead of dev->desc to obtain the master controller. This avoids a potential
unlocked dereference of dev->desc.

> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> index 101eaa77de686..a3778282e84c2 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -309,7 +309,7 @@ EXPORT_SYMBOL_GPL(i3c_device_match_id);
>   */
>  u32 i3c_device_get_supported_xfer_mode(struct i3c_device *dev)
>  {
> -	return i3c_dev_get_master(dev->desc)->this->info.hdr_cap | BIT(I3C_SDR);
> +	return i3c_bus_to_i3c_master(dev->bus)->this->info.hdr_cap | BIT(I3C_SDR);

[Severity: Critical]
Does this change introduce a use-after-free if the device outlives the master
controller?

When a master is unregistered, i3c_master_unregister() calls
i3c_master_unregister_i3c_devs() to unregister child devices. If a child
device has an elevated refcount (e.g., from an open file descriptor), its
struct i3c_device outlives the unregistration.

i3c_master_bus_cleanup() then calls i3c_master_detach_free_devs(), which
explicitly frees master->this.

If the child device later calls i3c_device_get_supported_xfer_mode(), it
would dereference the explicitly freed master->this pointer. The previous
code safely faulted on a NULL dev->desc, but this new path appears to access
freed memory.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does i3c_device_match() have a TOCTOU race
condition leading to a use-after-free?

drivers/i3c/master.c:i3c_device_match() {
        if (i3cdev->desc && i3cdev->desc->boardinfo)
                static_addr_method = i3cdev->desc->boardinfo->static_addr_method;
}

If i3c_device_match() accesses i3cdev->desc without holding the device lock
during concurrent master unregistration, a context switch could occur right
after i3cdev->desc is checked.

If the unregistration thread then frees the descriptor via kfree() in
i3c_master_bus_cleanup(), the subsequent evaluation of
i3cdev->desc->boardinfo will dereference a freed pointer.

[Severity: High]
This isn't a bug introduced by this patch, but can i3c_device_match_id()
evaluate uninitialized stack memory?

During concurrent driver registration and master unregistration, if
i3c_master_unregister_i3c_devs() sets i3cdev->dev->desc = NULL,
i3c_device_match() might still proceed to call i3c_device_match_id().

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

If i3cdev->desc is NULL, i3c_device_get_info() bails out safely without
populating devinfo, leaving the stack memory completely uninitialized when
it is subsequently read for driver matching.

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