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

On Wed, Jun 24, 2026 at 10:43:26AM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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.

Look like correct, but I am not sure if target will NACK SETDASA. Or should
use SETNEWDA for SETAASA method.

Frank

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

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