Re: [PATCH v4 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:
- [Medium] Permanent leak of firmware node references in I2C and I3C boardinfo structures.

Pre-existing issues:
- [High] Dangling pointer `desc->dev` causes Use-After-Free on device unregistration if device_register() fails.
--

commit 89419216775e33fb2b850943aa4a2781f19278a1
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 109aa50eb1f8f..3b19a5e8f46d0 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -1902,7 +1904,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));
>  
>  		ret = device_register(&desc->dev->dev);
>  		if (ret) {

[Severity: High]
This is a pre-existing issue, but can this error path cause a use-after-free
and double-free?

If device_register() fails, put_device() is called which frees the memory
backing desc->dev, but the desc->dev pointer is not set to NULL.

Later during cleanup, the dangling pointer bypasses the NULL check:

i3c_master_unregister_i3c_devs() {
    ...
    if (!i3cdev->dev)
        continue;

    i3cdev->dev->desc = NULL;
    ...
}

Does desc->dev need to be set to NULL after put_device() to prevent this?

[ ... ]
> @@ -2623,14 +2630,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: Medium]
Does this code leak the fwnode reference?

The result of fwnode_handle_get() is completely discarded. Since
i2c_new_client_device() separately takes and drops its own fwnode reference,
does this initial reference remain unreleased?

[ ... ]
> @@ -2670,14 +2677,14 @@ of_i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>  		return -EINVAL;
>  
>  	boardinfo->init_dyn_addr = init_dyn_addr;
> -	boardinfo->of_node = of_node_get(node);
> +	boardinfo->fwnode = fwnode_handle_get(fwnode);
>  	list_add_tail(&boardinfo->node, &master->boardinfo.i3c);
>  
>  	return 0;
>  }

[Severity: Medium]
Does this also leak the fwnode reference?

The reference is acquired and stored in the devres-managed boardinfo
structure, but there seems to be no teardown logic for the master->boardinfo
lists to drop it before devres frees them.

As seen earlier in i3c_master_register_new_i3c_devs():

device_set_node(&desc->dev->dev, fwnode_handle_get(desc->boardinfo->fwnode));

A second reference is taken there which is properly dropped by
i3c_device_release(), but does the first reference stored in
boardinfo->fwnode remain stranded?

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