Re: [PATCH v5 07/17] i3c: renesas: Clean DATBAS register on detach

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

Pre-existing issues:
- [High] The ENTDAA procedure in `renesas_i3c_daa` is incompatible with fragmented device indices, miscalculates masks, and leaks speculative `DATBAS` state.
- [High] Hardware transfer is not aborted on timeout and queued transfers are never started, leading to cross-transfer corruption and use-after-free.
--

--- Patch [7]: [PATCH v5 07/17] i3c: renesas: Clean DATBAS register on detach ---
commit c5598a36339bc89f9663008d7b023828e4ec62e5
Author: Claudiu Beznea <[email protected]>

i3c: renesas: Clean DATBAS register on detach

The controller uses DATBAS registers on TX/RX logic. Clean the DATBAS
register for the detached I3C device to avoid issues.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index acc30ed615ab9..b9784d238f61c 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -935,6 +935,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct renesas_i3c *i3c = to_renesas_i3c(m);
>  
> +	renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +
>  	i3c_dev_set_master_data(dev, NULL);
>  	i3c->addrs[data->index] = 0;
>  	i3c->free_pos |= BIT(data->index);

While reviewing this patch, I noticed a couple of pre-existing issues in
drivers/i3c/master/renesas-i3c.c that might need attention.

In renesas_i3c_daa():

>         newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
>         newdevs &= ~olddevs;

[Severity: High]
This is a pre-existing issue, but does this miscalculate the mask if zero
devices join during DAA?

If zero devices join, cmd->rx_count equals i3c->maxdevs, which would make
GENMASK(maxdevs - rx_count - 1, 0) evaluate to GENMASK(-1, 0). Could this
underflow to ~0UL and falsely add phantom devices for every free position?

Also in renesas_i3c_daa():

>         NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
>         NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;

If free indices are fragmented (for example, after a detach like the one this
patch addresses), does this instruct the hardware to assign a sequential block
of devices? 

It appears this might override the DATBAS configuration of already-occupied
higher indices, resulting in duplicate I3C addresses. Furthermore, do
speculative DATBAS writes for indices that do not join get cleaned up, or
could they permanently pollute the controller's state?

In renesas_i3c_wait_xfer():

>         time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
>         if (!time_left)
>                 renesas_i3c_dequeue_xfer(i3c, xfer);

[Severity: High]
This isn't a bug introduced by this patch, but does this timeout path leave
the hardware actively processing the transfer?

The timeout removes the transfer from the software queue and allows its
memory to be freed, but if an abort isn't sent to the hardware, could this
create a use-after-free race?

In renesas_i3c_resp_isr():

>         xfer = list_first_entry_or_null(&i3c->xferqueue.list,
>                                         struct renesas_i3c_xfer, node);
>         if (xfer)
>                 list_del_init(&xfer->node);
> 
>         i3c->xferqueue.cur = xfer;

If the next transfer is popped from the queue and assigned to cur, but never
actually started in the hardware, will it permanently stall and guarantee a
timeout?

If a new transfer is later started and becomes cur, could the old delayed
hardware interrupt fire and process old events against the new cur transfer,
writing PIO data into the new buffer (or previously freed stack space)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

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