Re: [PATCH v5 04/12] i3c: master: Add support for devices using SETAASA

[sashiko-bot@kernel.org]

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

New issues:
- [High] SETAASA broadcast is sent before SETDASA direct assignments, which breaks initialization for devices that natively support SETAASA but are configured for SETDASA.

Pre-existing issues:
- [High] Missing RSTDAA on error path due to premature address slot clearing leads to bus address collision.
- [High] Double free and Use-After-Free of `desc->dev` when `device_register()` fails.
--

commit 08b7cd00b36fcaa4fe6292af27e273df6dc64b5a
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: master: Add support for devices using SETAASA

Add support for devices using SETAASA attached to DDR5 memory modules that
do not support ENTDAA. Devices using SETAASA assign their pre-defined static
addresses as dynamic addresses during DAA.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4bba2bad897a7..3deae8cdae525 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1878,6 +1924,22 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_free_dev;
>  
> +	/*
> +	 * For devices using SETAASA instead of ENTDAA, the address is statically
> +	 * assigned. Update the dynamic address to the provided static address.
> +	 * Reattach the I3C device after updating the dynamic address with the same
> +	 * static address. It is not mandatory for such devices to implement CCC
> +	 * commands like GETPID, GETDCR etc. Hence, we can return after reattaching.
> +	 */
> +	if (i3cdev->boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		i3cdev->info.dyn_addr = i3cdev->boardinfo->static_addr;
> +		ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);
> +		if (ret)
> +			goto err_rstdaa;
> +
> +		return 0;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but missing RSTDAA on the error path due to
premature address slot clearing can lead to bus address collisions.

In i3c_master_early_i3c_dev_add(), the driver assigns the dynamic address
with SETDASA and reattaches the device. If reattaching fails, the error
path internally calls i3c_master_put_i3c_addrs(), which clears the
init_dyn_addr slot status back to I3C_ADDR_SLOT_FREE.

When the code jumps to err_rstdaa:

err_rstdaa:
	i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);

It calls i3c_master_rstdaa_locked() to reset the device's dynamic address.
However, i3c_master_rstdaa_locked() verifies the slot status:

drivers/i3c/master.c:i3c_master_rstdaa_locked() {
    ...
	if (addr != I3C_BROADCAST_ADDR && addrstat != I3C_ADDR_SLOT_I3C_DEV)
		return -EINVAL;
    ...
}

Because the slot was just freed, the function silently returns without
sending the RSTDAA CCC command.

Will this allow the hardware device to continue squatting on the dynamic
address while the master considers the slot free, causing collisions later?

>  	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
>  					i3cdev->boardinfo->init_dyn_addr);

[ ... ]

> @@ -2232,6 +2294,12 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	if (ret)
>  		goto err_bus_cleanup;
>  
> +	if (master->addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		ret = i3c_master_setaasa_locked(master);
> +		if (ret)
> +			goto err_bus_cleanup;
> +	}
> +

[Severity: High]
Is it possible that sending the SETAASA broadcast before direct SETDASA
assignments breaks initialization for devices that natively support SETAASA
but are configured for SETDASA?

According to the I3C specification, any device on the bus natively supporting
SETAASA will respond to this broadcast by adopting its static address as its
dynamic address. 

After this broadcast, the driver iterates through devices and attempts to
assign custom dynamic addresses via direct SETDASA commands:

drivers/i3c/master.c:i3c_master_early_i3c_dev_add() {
    ...
	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
					i3cdev->boardinfo->init_dyn_addr);
    ...
}

Since the target device already adopted its dynamic address during the
SETAASA broadcast, it is no longer in the unassigned state and will NACK
the subsequent SETDASA command.

Could this cause device initialization to abort entirely?

Should SETDASA direct assignments be executed before broadcasting SETAASA?

>  	/*
>  	 * Reserve init_dyn_addr first, and then try to pre-assign dynamic
>  	 * address and retrieve device information if needed.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but there is a potential double
free and use-after-free of desc->dev when device_register() fails.

In i3c_master_register_new_i3c_devs(), if device_register() fails,
put_device() is called:

drivers/i3c/master.c: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);
	}
    ...
}

This drops the reference count to 0 and triggers i3c_device_release(), which
frees desc->dev via kfree(i3cdev). However, the pointer desc->dev is not set
to NULL and remains dangling in the master->bus.devs.i3c list.

Later, when the master module is unloaded or unregistered, the cleanup
iterates over the list:

drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
    ...
		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);
		i3cdev->dev = NULL;
    ...
}

Because i3cdev->dev is a dangling pointer, the check passes. This leads to
writing to i3cdev->dev->desc (use-after-free) and calling put_device() again
(double free).

Could we set desc->dev to NULL after put_device() in the registration error
path?

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