Re: [PATCH v7 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] Probing of I3C master controllers fails on ACPI platforms if they have child nodes.

Pre-existing issues:
- [High] Dangling pointer left in desc->dev upon device_register() failure leads to Use-After-Free and Double Free on teardown.
--

--- Patch [2]: [PATCH v7 02/12] i3c: master: Use unified device property interface ---
commit 67a3b30ddd40ca8910b28ac5cd83aad57174f7e8
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..4b3d9628bc39b 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1928,7 +1930,7 @@ 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, desc->boardinfo->fwnode);
>  
>  		ret = device_register(&desc->dev->dev);
>  		if (ret) {

[Severity: High]
This is a pre-existing issue, but it appears desc->dev is left pointing to
a freed device if device_register() fails. When put_device() frees the
underlying struct i3c_device (via i3c_device_release()), desc->dev isn't
set to NULL.

Later, if i3c_master_unregister_i3c_devs() runs during teardown, could this
lead to a use-after-free and double-free?

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))
            ...
        else
            put_device(&i3cdev->dev->dev);
    }
}

[ ... ]

> -static int of_i3c_master_add_dev(struct i3c_master_controller *master,
> -				 struct device_node *node)
> +static int i3c_master_add_dev(struct i3c_master_controller *master,
> +			      struct fwnode_handle *fwnode)
>  {
>  	u32 reg[3];
>  	int ret;
>  
>  	if (!master)
>  		return -EINVAL;
>  
> -	ret = of_property_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg));
> +	ret = fwnode_property_read_u32_array(fwnode, "reg", reg, ARRAY_SIZE(reg));
>  	if (ret)
>  		return ret;

[Severity: High]
Since this function is now called for ACPI child nodes via
fwnode_populate_i3c_bus(), will this cause I3C master probe to fail on ACPI
systems?

ACPI nodes typically use _ADR or _CRS instead of a "reg" property. If "reg"
is missing, fwnode_property_read_u32_array() returns an error, which
propagates up and causes the entire master registration to abort.

Before this patch, the loop in of_populate_i3c_bus() was skipped completely
on ACPI systems because dev->of_node was NULL.

>  
>  	/*
>  	 * The manufacturer ID can't be 0. If that's the case, we're
>  	 * dealing with an I2C device.
>  	 */
>  	if (!reg[1])

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