Re: [PATCH AUTOSEL 6.18-5.15] i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing prerequisite commit causes duplicate I3C dynamic address assignments.

Pre-existing issues:
- [Critical] Missing reinit_completion() in DAA loop leads to Use-After-Free of 'xfer' and stack corruption.
--

commit 832ce28e7937a4fe5df00bb4a2c81e5ebbcd17da
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA
    
This commit modifies the MIPI I3C HCI driver to ignore errors from
i3c_master_add_i3c_dev_locked() during Dynamic Address Assignment (DAA).
Instead of aborting the sequence on failure, it continues enumeration to
align with the behavior of other I3C master drivers.
    
Link: https://patch.msgid.link/20260612080107.11606-5-adrian.hunter@intel.com

> diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> index eb8a3ae2990d7..50290dfbe47dc 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> @@ -362,9 +362,7 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
>  		 * TODO: Extend the subsystem layer to allow for registering
>  		 * new device and provide BCR/DCR/PID at the same time.
>  		 */
> -		ret = i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
> -		if (ret)
> -			break;
> +		i3c_master_add_i3c_dev_locked(&hci->master, next_addr);

[Severity: Critical]
This isn't a bug introduced by this patch, but does the lack of a
reinit_completion() call in this loop lead to a use-after-free and stack
corruption?

Earlier in hci_cmd_v1_daa(), wait_for_completion_timeout() is called on the
stack-allocated completion:

	if (!wait_for_completion_timeout(&done, HZ) &&
	    hci->io->dequeue_xfer(hci, xfer, 1)) {
		ret = -ETIME;
		break;
	}

If the timeout expires while the hardware completes concurrently,
dequeue_xfer() returns false and the loop continues, but the completion
counter remains at 1.

In the next iteration, wait_for_completion_timeout() will return immediately
without waiting. The loop will process stale data and eventually exit,
freeing the xfer and destroying the completion on the stack, all while the
hardware DMA is actively processing the second transfer. Upon completion,
the IRQ handler will access the freed xfer and corrupt the stack.

[Severity: High]
Will removing this error check cause duplicate I3C dynamic address assignments
because a prerequisite commit is missing from this tree?

This change assumes the presence of upstream commit b3ba8383da4d0 ("i3c:
master: Prevent reuse of dynamic address on device add failure") which ensures
the address slot remains occupied on failure.

However, that prerequisite commit is missing from this branch. As a result,
if i3c_master_add_i3c_dev_locked() fails, the error path in
drivers/i3c/master.c will detach the device and free the dynamic address:

	err_detach_dev:
		if (newdev->dev && newdev->dev->desc)
			newdev->dev->desc = NULL;

		i3c_master_detach_i3c_dev(newdev);

Because the loop now continues instead of breaking, it will allocate this
exact same dynamic address again and program it into the hardware of the
next physical device, resulting in multiple I3C devices responding to the
same address.

>  	}
>  
>  	if (dat_idx >= 0)
> diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
> index efb4326a25b73..d3896ee93b5e3 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
> @@ -300,9 +300,7 @@ static int hci_cmd_v2_daa(struct i3c_hci *hci)
>  		 * TODO: Extend the subsystem layer to allow for registering
>  		 * new device and provide BCR/DCR/PID at the same time.
>  		 */
> -		ret = i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
> -		if (ret)
> -			break;
> +		i3c_master_add_i3c_dev_locked(&hci->master, next_addr);

[Severity: Critical]
This is a pre-existing issue, but does the same missing reinit_completion()
logic error apply to the v2 DAA path?

Like hci_cmd_v1_daa(), the loop here in hci_cmd_v2_daa() waits on the same
completion object:

	if (!wait_for_completion_timeout(&done, HZ) &&
	    hci->io->dequeue_xfer(hci, xfer, 2)) {
		ret = -ETIME;
		break;
	}

A timeout racing with an IRQ completion will desynchronize the completion
counter, causing subsequent iterations to return immediately. This will
eventually lead to a use-after-free of the xfer and stack corruption when
the hardware DMA finishes.

[Severity: High]
Will ignoring the error here also cause address collisions on the I3C bus?

Since this tree lacks the upstream b3ba8383da4d0 prerequisite, any failure
in i3c_master_add_i3c_dev_locked() will free the address. The next loop
iteration will then reallocate the exact same address to a different device,
leading to severe hardware bus conflicts.

>  	}
>  
>  	hci_free_xfer(xfer, 2);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-608-sashal@kernel.org?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c