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

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

[Adrian Hunter]

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.

Two problems follow from that:

i3c_device_release() does WARN_ON(i3cdev->desc), so it warns because
desc->dev->desc still points back at the descriptor.  Clear it before
calling put_device().

After put_device() frees the i3c_device, desc->dev is left pointing at
freed memory, so clear desc->dev as well.  That prevents, for example,
i3c_master_unregister_i3c_devs() seeing desc->dev as non-NULL and
dereferencing it.

Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-i3c/20260701203053.8F3971F000E9@smtp.kernel.org/
Fixes: cab63f6488761 ("i3c: Fix potential refcount leak in i3c_master_register_new_i3c_devs")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 979457e635bd..7254b13fe4f0 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1942,7 +1942,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;
 			put_device(&desc->dev->dev);
+			desc->dev = NULL;
 		}
 	}
 }
-- 
2.53.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[sashiko-bot]

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 <adrian.hunter@intel.com>

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/20260701203053.8F3971F000E9@smtp.kernel.org/

> 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/20260702183644.60827-1-adrian.hunter@intel.com?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Frank Li]

On Thu, Jul 02, 2026 at 09:36:44PM +0300, Adrian Hunter wrote:
> 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.
>
> Two problems follow from that:
>
> i3c_device_release() does WARN_ON(i3cdev->desc), so it warns because
> desc->dev->desc still points back at the descriptor.  Clear it before
> calling put_device().
>
> After put_device() frees the i3c_device, desc->dev is left pointing at
> freed memory, so clear desc->dev as well.  That prevents, for example,
> i3c_master_unregister_i3c_devs() seeing desc->dev as non-NULL and
> dereferencing it.
>
> Reported-by: sashiko-bot@kernel.org
> Link: https://lore.kernel.org/linux-i3c/20260701203053.8F3971F000E9@smtp.kernel.org/
> Fixes: cab63f6488761 ("i3c: Fix potential refcount leak in i3c_master_register_new_i3c_devs")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i3c/master.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 979457e635bd..7254b13fe4f0 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1942,7 +1942,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;
>  			put_device(&desc->dev->dev);
> +			desc->dev = NULL;
>  		}
>  	}
>  }
> --
> 2.53.0
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c