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

[sashiko-bot@kernel.org]

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?

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