Re: [PATCH] i3c: master: Fix device_register() error path

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

New issues:
- [High] Setting `desc->dev = NULL` on the `device_register()` error path introduces a retry loop that causes a double-put and Use-After-Free of the `boardinfo->of_node`.
- [High] Clearing `desc->dev->desc = NULL` introduces a data race and NULL pointer dereference for concurrent sysfs readers.

Pre-existing issues:
- [High] Uninitialized stack memory is leaked to userspace via sysfs uevents and attributes when an I3C device fails to register or is unregistered.
--

commit 31b28d71fcb9eca7e58c9138da77a1cc7e414f6d
Author: Adrian Hunter <[email protected]>

i3c: master: Fix device_register() error path

When device_register() fails in i3c_master_register_new_i3c_devs(),
put_device() is called to drop the reference taken by
device_register(). That drops the last reference, so the device's
release callback i3c_device_release() runs and frees the i3c_device.

Link: https://lore.kernel.org/linux-i3c/[email protected]/

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..ac408086ddcc7 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1934,7 +1934,9 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		if (ret) {
>  			dev_err(&master->dev,
>  				"Failed to add I3C device (err = %d)\n", ret);
> +			desc->dev->desc = NULL;

[Severity: High]
Does clearing desc->dev->desc here introduce a data race with concurrent
sysfs readers?

i3c_master_register_new_i3c_devs() is protected by a read lock.
If device_register() fails, we write desc->dev->desc = NULL. Concurrently,
sysfs attribute readers like bcr_show() also acquire the same read lock:

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

Can the reader fetch a NULL descriptor and crash since both paths only hold a
read lock?

[Severity: High]
This is a pre-existing issue, but can setting the descriptor to NULL cause
uninitialized stack memory to leak in the uevent and modalias paths?

When an I3C device fails to register (or is unregistered), clearing the
descriptor triggers a remove uevent. In i3c_device_uevent():

	struct i3c_device_info devinfo;
	...
	if (i3cdev->desc)
		devinfo = i3cdev->desc->info;

	manuf = I3C_PID_MANUF_ID(devinfo.pid);

If i3cdev->desc is NULL, devinfo remains uninitialized on the stack but its
fields are unconditionally extracted and formatted into the modalias string.
A similar leak happens concurrently in modalias_show(). Will this leak kernel
stack memory to userspace?

>  			put_device(&desc->dev->dev);
> +			desc->dev = NULL;

[Severity: High]
Would setting desc->dev to NULL here introduce a use-after-free of
boardinfo->of_node on subsequent retries?

If device_register() fails, put_device() calls i3c_device_release(), which
unconditionally calls of_node_put() on dev.of_node (consuming the single
reference originally held by boardinfo). 

By setting desc->dev = NULL, on a subsequent execution of
i3c_master_register_new_i3c_devs() (e.g., triggered by a hot-join event or
dynamic address assignment), the loop will retry registration, allocate a
new device, and reassign the now-freed of_node. Will any further failure or
successful unregistration then trigger a double-put?

>  		}
>  	}
>  }

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

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