Re: [PATCH v5 02/12] i3c: master: Use unified device property interface

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

New issues:
- [High] Fwnode references acquired for I2C and I3C boardinfo entries are permanently leaked.

Pre-existing issues:
- [High] Dangling `desc->dev` pointer causes Use-After-Free and Double Free when device registration fails.
--

commit cf59bb0f706cf267c9ca158815835766a81fc214
Author: Akhil R <[email protected]>

i3c: master: Use unified device property interface

Replace all OF-specific functions with unified device property functions
as a prerequisite to support both ACPI and device tree.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..b20f56f7b68e8 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1928,7 +1930,8 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  			     desc->info.pid);
>  
>  		if (desc->boardinfo)
> -			desc->dev->dev.of_node = desc->boardinfo->of_node;
> +			device_set_node(&desc->dev->dev,
> +					fwnode_handle_get(desc->boardinfo->fwnode));

[Severity: High]
Does this take an extra reference to the fwnode that ends up leaking?

The original code simply assigned the pointer, but the new code calls
fwnode_handle_get(), acquiring an additional reference. When the device is
subsequently released via i3c_device_release(), it will drop this second
reference.

Since the original reference stored in desc->boardinfo->fwnode does not appear
to be dropped during master teardown, does this cause the initial reference to
permanently leak on the normal success path?

>  		ret = device_register(&desc->dev->dev);
>  		if (ret) {

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave a
dangling pointer in desc->dev?

If device_register() fails, put_device(&desc->dev->dev) is called, which drops
the reference count to 0 and invokes i3c_device_release(). The release
function frees desc->dev (the i3cdev structure). However, desc->dev is not set
to NULL.

Later, when i3c_master_unregister_i3c_devs() iterates through the bus,
it will dereference the dangling pointer:

drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
    ...
    i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
        if (!i3cdev->dev)
            continue;

        i3cdev->dev->desc = NULL;
        if (device_is_registered(&i3cdev->dev->dev))
            device_unregister(&i3cdev->dev->dev);
        else
            put_device(&i3cdev->dev->dev);
        i3cdev->dev = NULL;
    }
    ...
}

Could this lead to a use-after-free and double-free when it evaluates
i3cdev->dev and attempts to call put_device() again?

[ ... ]

> @@ -2649,14 +2656,14 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
>  	boardinfo->lvr = reg[2];
>  
>  	list_add_tail(&boardinfo->node, &master->boardinfo.i2c);
> -	of_node_get(node);
> +	fwnode_handle_get(fwnode);
>  
>  	return 0;
>  }

[Severity: High]
Does this code leak the fwnode reference?

The fwnode_handle_get(fwnode) call increments the reference count, but the
resulting reference is neither stored in boardinfo nor dropped before the
function returns. This appears to leak the reference immediately on the
success path.

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