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

On Tue, Jun 16, 2026 at 10:19:18AM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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.

Look AI correct, please check it.

Frank
>
> [ ... ]
>
> > @@ -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/[email protected]?part=4

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