Re: [PATCH v7 05/12] i3c: master: Add support for devices without PID

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

New issues:
- [High] Dead code in `i3c_master_attach_boardinfo` fallback match logic prevents Device Tree node attachment for SETAASA devices discovered via DAA.
- [High] Duplicate I3C device creation for SETAASA devices participating in Dynamic Address Assignment.

Pre-existing issues:
- [High] Use-After-Free and Double-Free on driver teardown due to a dangling pointer left by `device_register()` failure.
--

--- Patch [5]: [PATCH v7 05/12] i3c: master: Add support for devices without PID ---
commit 978db33da88aedc2a14abd69fdc128b5e3961e92
Author: Akhil R <[email protected]>

i3c: master: Add support for devices without PID

Devices using SETAASA for address assignment are not required to have
a 48-bit PID according to the I3C specification. Allow such devices to
register and use the static address where PID was required.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 87749a739e62d..488177d95e484 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2001,8 +2001,17 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		desc->dev->dev.type = &i3c_device_type;
>  		desc->dev->dev.bus = &i3c_bus_type;
>  		desc->dev->dev.release = i3c_device_release;
> -		dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> -			     desc->info.pid);
> +
> +		/*
> +		 * For devices without PID (e.g., SETAASA devices), use
> +		 * static address for naming instead.
> +		 */
> +		if (desc->info.pid)
> +			dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> +				     desc->info.pid);
> +		else
> +			dev_set_name(&desc->dev->dev, "%d-%02x", master->bus.id,
> +				     desc->info.static_addr);
>  
>  		if (desc->boardinfo)
>  			device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);

[Severity: High]
This is a pre-existing issue, but does device_register() failure in this
function leave a dangling pointer that causes a use-after-free later?

Just below this hunk in i3c_master_register_new_i3c_devs():

    ret = device_register(&desc->dev->dev);
    if (ret) {
        dev_err(&master->dev,
            "Failed to add I3C device (err = %d)\n", ret);
        put_device(&desc->dev->dev);
    }

When put_device() frees desc->dev->dev, desc->dev is not set to NULL.
Later, if the master is unregistered, i3c_master_unregister_i3c_devs()
iterates through the list and dereferences this dangling pointer:

    if (!i3cdev->dev)
        continue;
    i3cdev->dev->desc = NULL;
    // ...
    else
        put_device(&i3cdev->dev->dev);

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

[ ... ]

> @@ -2401,8 +2410,18 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
>  	struct i3c_dev_boardinfo *i3cboardinfo;
>  
>  	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> -		if (i3cdev->info.pid != i3cboardinfo->pid)
> -			continue;
> +		/*
> +		 * For devices without PID (e.g., SETAASA devices), match by
> +		 * static address. For devices with PID, match by PID.
> +		 */
> +		if (i3cboardinfo->pid) {
> +			if (i3cdev->info.pid != i3cboardinfo->pid)
> +				continue;
> +		} else {
> +			if (!i3cboardinfo->static_addr ||
> +			    i3cdev->info.static_addr != i3cboardinfo->static_addr)
> +				continue;
> +		}

[Severity: High]
Will this fallback matching logic fail for SETAASA devices discovered
dynamically?

If a SETAASA device without a PID in the Device Tree loses its dynamic
address and asserts a Hot-Join, the master will execute Dynamic Address
Assignment (DAA).

During DAA, __i3c_master_add_i3c_dev_locked() allocates a new device
descriptor and initializes newdev's static_addr to 0, because the static
address cannot be retrieved via CCCs.

When i3c_master_attach_boardinfo(newdev) is then called,
i3cdev->info.static_addr will be 0. Would this cause the condition
i3cdev->info.static_addr != i3cboardinfo->static_addr to always evaluate
to true, skipping the boardinfo assignment and preventing the driver from
binding?

>  
>  		i3cdev->boardinfo = i3cboardinfo;
>  		i3cdev->info.static_addr = i3cboardinfo->static_addr;
> @@ -2416,8 +2435,12 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	struct i3c_master_controller *master = i3c_dev_get_master(refdev);
>  	struct i3c_dev_desc *i3cdev;
>  
> +	if (!refdev->info.pid)
> +		return NULL;
> +
>  	i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
> -		if (i3cdev != refdev && i3cdev->info.pid == refdev->info.pid)
> +		if (i3cdev != refdev && i3cdev->info.pid &&
> +		    i3cdev->info.pid == refdev->info.pid)
>  			return i3cdev;
>  	}
>  

[Severity: High]
Does this logic lead to duplicate device creation for SETAASA devices that
participate in Dynamic Address Assignment?

If a SETAASA device is defined in the DT without a PID, it is initially
added to the master's bus list with pid = 0. If the device later asserts a
Hot-Join, the master executes DAA, discovers the device, and retrieves its
true physical PID.

In __i3c_master_add_i3c_dev_locked(), the old device is searched using:
olddev = i3c_master_search_i3c_dev_duplicate(newdev);

Since the existing device has pid = 0, the check here for i3cdev->info.pid
and i3cdev->info.pid == refdev->info.pid will fail to match the newly
discovered PID to the existing device. Will this cause the master to add
the newly discovered device instead of detaching and updating the existing
one, resulting in duplicate devices on the bus?

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