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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing CCC command whitelisting in I3C master drivers causes entire bus initialization to fail.
- [High] Premature SETAASA broadcast preempts and breaks SETDASA initialization.
- [High] Missing physical device address reset (RSTDAA) on SETAASA attach failure allows address collisions.

Pre-existing issues:
- [High] Bypassing PID validation allows duplicate PIDs, triggering a pre-existing Use-After-Free during teardown.
--

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

i3c: master: Add support for devices using SETAASA

Add support for devices using SETAASA, such as SPD5118 and SPD5108
attached to DDR5 memory modules that do not support ENTDAA. Follow the
guidelines proposed by the MIPI Discovery and Configuration
Specification [1] for discovering such devices.

SETAASA (Set All Addresses to Static Address) differs from standard I3C
address assignment that uses ENTDAA or SETDASA to assign dynamic
addresses. Devices using SETAASA assign their pre-defined static addresses
as their dynamic addresses during DAA, and it is not mandatory for these
devices to implement standard CCC commands like GETPID, GETDCR, or GETBCR.
For such devices, it is generally recommended to issue SETHID (specified
by JEDEC JESD300) as a prerequisite for SETAASA to stop HID bit flipping.

Link: https://www.mipi.org/mipi-disco-for-i3c-download

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f0e05bcac26da..557332d932576 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1852,6 +1898,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(i3cdev, 0);
> +		if (ret)
> +			goto err_detach_dev;
               ^^^^^^^^

[Severity: High]
Does skipping the physical device address reset (RSTDAA) here allow address
collisions?

If i3c_master_reattach_i3c_dev() fails for a SETAASA device, the code jumps to
err_detach_dev. Unlike the SETDASA error path which jumps to err_rstdaa to
issue a directed RSTDAA command and clear the dynamic address, this path
leaves the physical device active at that address.

Meanwhile, i3c_master_detach_i3c_dev() frees the address slot in the master's
software state. During subsequent Dynamic Address Assignment (DAA), the master
may assign this supposedly "free" address to another device, resulting in two
physical devices responding to the same dynamic address.

[ ... ]

> @@ -2206,6 +2268,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]
Will this unconditional broadcast of SETAASA cause the entire bus initialization
to fail on certain controllers?

Several I3C master drivers (e.g., Cadence, SVC, Renesas) implement a strict
whitelist via the supports_ccc_cmd callback. Because these whitelists were
not updated to allow I3C_CCC_SETAASA (0x29) and I3C_CCC_VENDOR(0, true)
(SETHID), the master driver returns -EOPNOTSUPP. This error propagates up
through i3c_master_setaasa_locked(), causing i3c_master_bus_init() to
immediately abort.

[Severity: High]
Does this premature SETAASA broadcast break SETDASA initialization?

By broadcasting SETAASA before processing devices that require SETDASA:

    list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
        ...
        if (i3cboardinfo->static_addr)
            i3c_master_early_i3c_dev_add(master, i3cboardinfo);
    }

any device with a static address will accept the broadcast and adopt its
static address as its dynamic address. If a device was configured to use
SETDASA, it will erroneously accept the SETAASA broadcast.

When i3c_master_early_i3c_dev_add() subsequently attempts to issue the
directed SETDASA command, the device already has an assigned dynamic address
and will ignore the command. This causes the software state to desynchronize
from the physical device state. The MIPI I3C spec explicitly mandates that
SETDASA commands must be sent prior to the SETAASA command.

>  	/*
>  	 * Reserve init_dyn_addr first, and then try to pre-assign dynamic

[ ... ]

> @@ -2724,6 +2799,9 @@ i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>  			return -EINVAL;
>  	}
>  
> +	/* Update the address methods required for device discovery */
> +	master->addr_method |= boardinfo->static_addr_method;
> +
>  	boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
>  
>  	if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
        ^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does allowing duplicate PIDs
(such as PID=0 for SETAASA devices) trigger a pre-existing Use-After-Free during
teardown?

Since SETAASA devices don't require standard CCC commands like GETPID,
configuring multiple SETAASA devices with an identical dummy PID (like PID=0)
passes the validation here.

When i3c_master_register_new_i3c_devs() is called, dev_set_name() assigns
identical sysfs names to these duplicate devices. device_register() then fails
with -EEXIST. In the error path, put_device() is called, freeing desc->dev,
but desc->dev is not set to NULL.

During bus teardown in i3c_master_unregister_i3c_devs(), the core iterates
over all devices, sees the non-NULL dangling pointer, and calls
device_is_registered(&desc->dev->dev), leading to a Use-After-Free.

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