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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Devices returning a PID of 0 during Dynamic Address Assignment (DAA) bypass duplicate detection, leading to sysfs naming collisions that reliably trigger a preexisting Use-After-Free.
- [Medium] The driver incorrectly allows non-SETAASA I3C devices to be defined in the device tree with a PID of 0, permanently breaking device matching upon Hot-Join re-attachment.
- [Low] The newly added static address matching logic in `i3c_master_attach_boardinfo()` will never successfully match any device, rendering the `else` branch dead code.
- [Low] Redundant boolean check for `i3cdev->info.pid` inside the duplicate search loop.

Pre-existing issues:
- [High] Dangling pointer in `desc->dev` leads to a Use-After-Free and Double Free if `device_register()` fails.
- [High] Calling `device_register()` while holding a read lock on the bus rwsem can lead to a deadlock if the driver's probe function attempts an I3C transfer.
--

commit 76e7c85069f65f75e5476a13e6f4ab2ae705589f
Author: Akhil R <akhilrajeev@nvidia.com>

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 3deae8cdae525..bd0dc76c7ba1e 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1989,8 +1989,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,

[Severity: High]
This is a pre-existing issue, but does failing to clear desc->dev when
device_register() fails lead to a double free and use-after-free later?

Looking at 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);
    }

If device_register() fails, put_device() is called which frees the i3c_device
structure, but desc->dev is left dangling. Later, when the master is
unregistered in i3c_master_unregister_i3c_devs(), the loop checks:

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

Since desc->dev is not NULL, this writes to the freed memory and then calls
put_device() again, resulting in a use-after-free and double free.

[Severity: High]
This is also a pre-existing issue, but can calling device_register() while
holding a read lock on the bus rwsem lead to a deadlock?

In i3c_master_reg_work_fn():

    i3c_bus_normaluse_lock(&master->bus);
    if (!master->shutting_down)
        i3c_master_register_new_i3c_devs(master);

The device_register() call inside i3c_master_register_new_i3c_devs()
synchronously triggers the driver's probe function. If the probe attempts an
I3C transfer, it calls i3c_device_do_priv_xfers(), which recursively requests
the read lock on the bus.

If a concurrent thread, such as a Hot Join, requests a write lock between
these two read lock attempts, the rwsem blocks the second read lock to prevent
writer starvation. The writer blocks waiting for the first read lock to be
released, which won't happen until device_register() returns, resulting in an
ABBA deadlock.

[ ... ]

> @@ -2383,8 +2392,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]
Will this newly added else block ever successfully match a device?

i3c_master_attach_boardinfo() is called from
__i3c_master_add_i3c_dev_locked() to handle devices discovered dynamically via
DAA or Hot-Join. For these devices, the hardware does not provide a static
address during DAA, so i3cdev->info.static_addr is strictly 0.

Because of this, the condition
i3cdev->info.static_addr != i3cboardinfo->static_addr
will evaluate to 0 != non_zero, which is always true. This causes the loop
to unconditionally continue, making this fallback dead code.

>  
>  		i3cdev->boardinfo = i3cboardinfo;
>  		i3cdev->info.static_addr = i3cboardinfo->static_addr;
> @@ -2398,8 +2417,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;
> +

[Severity: High]
Does returning NULL when PID is 0 allow devices to bypass duplicate detection
and trigger the pre-existing use-after-free bug?

If a dynamically discovered device reports a PID of 0, this check
unconditionally bypasses duplicate rejection, so the device is repeatedly
added.

Because static_addr is also 0 for DAA-discovered devices, the new naming
fallback in i3c_master_register_new_i3c_devs():

    dev_set_name(&desc->dev->dev, "%d-%02x", master->bus.id,
                 desc->info.static_addr);

will name multiple devices the same (e.g., %d-00). The first instance will
register successfully, but subsequent ones will fail device_register() due to
sysfs name collisions. This perfectly triggers the desc->dev use-after-free
on the error path.

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

[Severity: Low]
Is the check for i3cdev->info.pid redundant here?

Since refdev->info.pid was explicitly checked for being non-zero right before
this loop, the equality check i3cdev->info.pid == refdev->info.pid already
guarantees that i3cdev->info.pid is non-zero. The boolean check isn't strictly
necessary.

>  			return i3cdev;
>  	}

[ ... ]

> @@ -2832,9 +2855,15 @@ i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>  
>  	boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
>  
> -	if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> -	    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> -		return -EINVAL;
> +	/* For SETAASA devices, validate the static address instead of PID */
> +	if (boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		if (!boardinfo->static_addr)
> +			return -EINVAL;
> +	} else {
> +		if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> +		    I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> +			return -EINVAL;
> +	}

[Severity: Medium]
Could this logic incorrectly allow non-SETAASA I3C devices in the device tree
to have a PID of 0, permanently breaking device matching upon Hot-Join?

If a device is configured with mipi-i3c-static-method = SETDASA, it will reach
this else block. If its PID is 0, both boardinfo->pid & GENMASK_ULL(63, 48)
and I3C_PID_RND_LOWER_32BITS(boardinfo->pid) evaluate to 0, which bypasses
validation, and the device registers with PID 0.

If the device later Hot-Joins, DAA will retrieve its real, non-zero PID. When
i3c_master_attach_boardinfo() runs, it sees i3cboardinfo->pid == 0 and falls
back to matching by static_addr. Because static_addr is 0 during DAA, it
fails to match and the device permanently loses its DT properties.

>  
>  	boardinfo->init_dyn_addr = init_dyn_addr;
>  	boardinfo->fwnode = fwnode_handle_get(fwnode);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=5