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

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

New issues:
- [Low] The fallback logic added to `i3c_master_attach_boardinfo` for matching devices without a PID is completely unreachable dead code.

Pre-existing issues:
- [High] Dangling pointer in `i3c_master_register_new_i3c_devs()` error path leads to Use-After-Free and Double-Free.
--

commit 02ce08803703bc5626ea5224b66caedd628224e4
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 557332d932576..bcc9c2d29c342 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1963,8 +1963,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,
>  					fwnode_handle_get(desc->boardinfo->fwnode));
>  
>  		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);
>  		}

[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling
pointer that can cause memory corruption?

If device_register() fails, put_device() drops the reference and frees
desc->dev. However, desc->dev is not set to NULL.

When the I3C master later unregisters, i3c_master_unregister_i3c_devs()
iterates through the bus devices. Since the pointer is left dangling, it
passes the if (!i3cdev->dev) check, leading to write-after-free when
setting i3cdev->dev->desc = NULL, followed by a double-free when calling
put_device() again.

Should desc->dev be set to NULL after calling put_device() here?

[ ... ]

> @@ -2357,8 +2366,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: Low]
Is this else block reachable?

Looking at i3c_master_add_i3c_boardinfo(), it requires a valid PID for all
non-SETAASA devices. Since SETAASA devices bypass
i3c_master_attach_boardinfo() entirely as they are attached early via
i3c_master_early_i3c_dev_add(), wouldn't i3cboardinfo->pid be strictly
non-zero for any device reaching this function?

Additionally, for dynamically discovered devices, wouldn't
i3cdev->info.static_addr be 0 prior to this call, causing
i3cdev->info.static_addr != i3cboardinfo->static_addr to always be true
anyway?

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

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c