Re: [PATCH v4 09/16] i3c: renesas: Return immediately if there is no transfer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Hardware Desynchronization and Memory Corruption on timeout
- [Critical] Missing Bounds Check causing Kernel Heap Buffer Overflow in Receive ISRs
- [High] Unchecked timeout leading to hardware desynchronization and incorrect success reporting in `renesas_i3c_i2c_xfers`
- [High] Missing Error Handling in `renesas_i3c_i3c_xfers` unconditionally returns success
- [High] Out-of-Bounds Read and Endianness violation on short TX transfers
--

commit e7c9bbe000eba8d228d206b4d05e155779672539
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Return immediately if there is no transfer

There is no need to allocate a transfer structure when i2c_nxfers is zero.
Return immediately instead of unnecessarily allocating memory.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 9edd2dc961073..fa9c15381ed6a 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1061,13 +1061,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	u8 start_bit = CNDCTL_STCND;
>  	int i;
>  
> +	if (!i2c_nxfers)
> +		return 0;
> +
>  	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
>  	if (!xfer)
>  		return -ENOMEM;
>  
> -	if (!i2c_nxfers)
> -		return 0;
> -
>  	renesas_i3c_bus_enable(m, false);
>  
>  	init_completion(&xfer->comp);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with
unchecked timeouts later in this same function:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
...
		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);

		if (cmd->err)
			break;
...

If wait_for_completion_timeout() times out, time_left will be 0 but cmd->err
remains 0 (its initialization value, since the ISR did not run to update it).
The check evaluates to false, causing the driver to continue the loop and issue
the next I2C message while the hardware may still be executing the previous one.
Finally, it unconditionally returns cmd->err (0), falsely reporting success to
the I2C subsystem on timeout. Should this explicitly check the timeout return
value?

[Severity: High]
This isn't a bug introduced by this patch, but renesas_i3c_i3c_xfers() appears
to unconditionally return success even on error:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
...
		renesas_i3c_wait_xfer(i3c, xfer);
	}

	return 0;
}

The function iterates through I3C transfers but ignores the error status
(xfer->ret) populated by the ISR on hardware failures (e.g., NACK, CRC errors).
Even if a timeout occurs, the function unconditionally returns 0 at the end,
masking all I3C transfer failures from the subsystem. Could it return xfer->ret
or an appropriate error code instead?

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem with hardware
desynchronization and memory corruption on timeout.

If a transfer timeout occurs, renesas_i3c_wait_xfer() removes the transfer from
the software queue, but the driver fails to abort the hardware operation. In
the ISR:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
...
		xfer = i3c->xferqueue.cur;
		if (!xfer) {
			/* Disable interrupts. */
			renesas_clear_bit(i3c->regs, NTIE, NTIE_TDBEIE0 | NTIE_RDBFIE0);
			/* Clear any error flags. */
			renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
...

The driver clears the BCTL_ABT bit instead of setting it. When a subsequent
transfer starts, interrupts are re-enabled and the hardware may fire a delayed
completion interrupt for the old transfer. The ISR will attribute this interrupt
to the newly enqueued transfer (i3c->xferqueue.cur), potentially causing data
corruption and out-of-bounds writes into the new transfer's buffer. Does
BCTL_ABT need to be set instead to properly abort?

[Severity: Critical]
This isn't a bug introduced by this patch, but there appears to be a missing
bounds check causing a kernel heap buffer overflow in the receive ISRs.

In both renesas_i3c_resp_isr() and renesas_i3c_rx_isr(), the length of incoming
data is calculated directly from hardware-provided registers without being
validated against the allocated buffer size (cmd->len).

For example, in renesas_i3c_resp_isr():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
...
			if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
				bytes_remaining = data_len - cmd->rx_count;

			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
...

And in renesas_i3c_rx_isr():

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);
...

A malfunctioning or malicious target device sending an oversized response could
cause i3c_readl_fifo() to write past the bounds of cmd->rx_buf. Can this be
safeguarded by checking against cmd->len?

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds read and
endianness violation on short TX transfers.

In renesas_i3c_start_xfer_locked(), immediate transfers of 1 to 3 bytes cast
the tx_buf pointer to a u32 * and dereference it:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_start_xfer_locked() {
...
		if (cmd->len <= 4) {
			cmd->cmd0 |= NCMDQP_CMD_ATTR(NCMDQP_IMMED_XFER);
			cmd->cmd0 |= NCMDQP_BYTE_CNT(cmd->len);
			cmd->tx_count = cmd->len;
			cmd1 = cmd->len == 0 ? 0 : *(u32 *)cmd->tx_buf;
		} else {
...

Since the caller may only provide a 1-byte array on the stack, this reads
out-of-bounds memory, potentially triggering KASAN or causing unaligned memory
access faults. Additionally, it reads the bytes in CPU host byte order, causing
the data sent over the I3C wire to be incorrectly ordered on Big Endian systems.
Would it be better to copy the bytes safely into a temporary u32 and convert to
the correct endianness?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=9

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