Re: [PATCH] i3c: master: renesas: reset xfer status before each wait

[Frank Li <Frank.li@oss.nxp.com>]

On Thu, Jun 25, 2026 at 03:20:58AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Buffer Overflow and Data Corruption in Transfer Interrupt Handlers.
> - [High] Missing Hardware Teardown on Timeout causing Use-After-Free and NULL Pointer Dereference.
> - [High] Ignored transfer timeout leads to Hardware State Desync and silent failures.
> - [High] Unvalidated hardware response length causes Integer Underflow and Undefined Behavior in GENMASK.
> --
>
> commit 1827d9b795e70cd1f909f688b702f06c732c89e1
> Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
>
> i3c: master: renesas: reset xfer status before each wait
>
> renesas_i3c_wait_xfer() reinitializes the completion before queuing a
> transfer, but it leaves xfer->ret unchanged. After one transfer completes
> successfully, a later timeout can therefore reuse the old zero status.
>
> Reset xfer->ret to -ETIMEDOUT before each wait, return the transfer
> status from the wait helper, and propagate it from DAA, CCC and private
> I3C transfers.
>
> > diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> > index f39c449922ca2..7f123cd72cdf3 100644
> > --- a/drivers/i3c/master/renesas-i3c.c
> > +++ b/drivers/i3c/master/renesas-i3c.c
> > @@ -433,15 +433,18 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
> [ ... ]
> > -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> > +static int renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> >  {
> >  	unsigned long time_left;
> >
> > +	xfer->ret = -ETIMEDOUT;
> >  	renesas_i3c_enqueue_xfer(i3c, xfer);
> >
> >  	time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
> >  	if (!time_left)
> >  		renesas_i3c_dequeue_xfer(i3c, xfer);
> > +
> > +	return xfer->ret;
> >  }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this timeout path safely
> abort the active hardware operation?
>
> If a timeout occurs, the driver removes the transfer from the queue via
> renesas_i3c_dequeue_xfer() but does not appear to halt the hardware DMA or
> interrupt execution.
>
> If the caller then frees the xfer struct, and the hardware later completes the
> operation, could the delayed interrupt in renesas_i3c_resp_isr() dereference
> a freed or unrelated transfer?

This is one problem, although it is less possibility. suggest fix later,
you may reduce timeout value and add some debug message isr, this problem
should be triggered.

Frank

>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
>     ...
>     xfer = i3c->xferqueue.cur;
>     cmd = xfer->cmds;
>     ...
> }
>
> [ ... ]
> > @@ -687,7 +690,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
> >  		    NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
> >  		    NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
> >
> > -	renesas_i3c_wait_xfer(i3c, xfer);
> > +	ret = renesas_i3c_wait_xfer(i3c, xfer);
> > +	if (ret)
> > +		return ret;
> >
> >  	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
>
> [Severity: High]
> This is a pre-existing issue, but does this calculation handle the case where
> no devices are attached?
>
> If no devices respond during Dynamic Address Assignment, cmd->rx_count will
> equal i3c->maxdevs. This would cause the subtraction to evaluate to -1,
> leading to undefined behavior in the GENMASK macro.
>
> Could this result in newdevs evaluating to all 1s and incorrectly registering
> empty slots as real devices?
>
> [ ... ]
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does renesas_i3c_i2c_xfers()
> properly handle transfer timeouts?
>
> When wait_for_completion_timeout() times out, it does not set cmd->err. The
> loop then checks if (cmd->err) and proceeds to the next message:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
>     ...
>     wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
>
>     if (cmd->err)
>         break;
>     ...
> }
>
> Could this ignore the timeout, incorrectly return success, and allow a delayed
> hardware operation to corrupt the buffer of the subsequent message?
>
> [Severity: Critical]
> This is a pre-existing issue, but do the interrupt handlers correctly manage
> buffer lengths and offsets for transfers?
>
> In renesas_i3c_rx_isr(), the byte count is rounded up to the nearest 32-bit
> word, which can write past the actual buffer length for unaligned transfers:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_rx_isr() {
>     ...
>     read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
>     i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
>     ...
> }
>
> Additionally, the transmit ISR writes the entire transfer length without tracking
> bytes already sent, which might overflow the hardware FIFO:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_tx_isr() {
>     ...
>     i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
>     ...
> }
>
> Finally, the response ISR reads the remaining bytes into the start of the
> buffer, potentially overwriting data previously received by the rx_isr:
>
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
>     ...
>     bytes_remaining = data_len - cmd->rx_count;
>     i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
>     ...
> }
>
> Could this lead to memory corruption and out-of-bounds writes?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260625030357.87402-1-pengpeng@iscas.ac.cn?part=1
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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