[PATCH v4 01/16] i3c: renesas: Check that the transfer is valid before accessing it

[sashiko-bot at kernel.org]

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

New issues:
- [High] The `!xfer` early return path in `renesas_i3c_rx_isr` fails to disable the `NTIE_RDBFIE0` interrupt, causing an interrupt storm.
- [High] A typo in the `!xfer` early return path of `renesas_i3c_tend_isr` duplicates the `BIE_TENDIE` flag, failing to mask the NACK interrupt.
- [High] The `!xfer` early return path in `renesas_i3c_start_isr` fails to disable the `BIE_STCNDDIE` interrupt.

Pre-existing issues:
- [Critical] The transfer functions `renesas_i3c_i3c_xfers` and `renesas_i3c_i2c_xfers` silently ignore timeouts and hardware errors, falsely returning success.
- [Critical] Hardware State Desync causing cross-transfer corruption due to failure to abort hardware on timeout.
- [Critical] FIFO overflow in I3C writes due to unconditionally writing the entire transfer length without progress tracking.
- [Critical] Out-of-bounds write and pointer offset corruption in `renesas_i3c_rx_isr` during I3C reads.
- [High] Hardware interrupt registers are modified via non-atomic read-modify-write operations from process context without locking, causing lost mask updates.
--

commit 80958e2863c878df9b87948c2774499050e47aee
Author: Claudiu Beznea <claudiu.beznea.uj at bp.renesas.com>

    i3c: renesas: Check that the transfer is valid before accessing it
    
    The Renesas I3C driver uses an asynchronous model to transfer data. It
    prepares a struct renesas_i3c_xfer, enqueues it, and waits for completion.
    The interrupt handler dequeues the transfer, updates/uses it, and signals
    the waiting thread.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..bc1a0ae1d12d8 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -1138,6 +1153,15 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			/* Disable interrupts. */
> +			renesas_clear_bit(i3c->regs, BIE, BIE_TENDIE | BIE_TENDIE);

[Severity: High]
Does this duplicated flag fail to mask the NACK interrupt?

It appears the code is meant to mask both the Transmit End and NACK
interrupts (as the next line clears BST_NACKDF | BST_TENDF). 

The failure to include BIE_NACKDIE leaves the NACK interrupt unmasked,
which could cause an interrupt storm if a NACK triggered the ISR.

> +			renesas_clear_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
> +			/* Clear any status flag. */
> +			renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF);
> +			return IRQ_HANDLED;
> +		}

[ ... ]

> @@ -1184,6 +1208,14 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			/* Clear any status registers. */
> +			renesas_clear_bit(i3c->regs, BST, BST_SPCNDDF);
> +			/* Clear the Read Buffer Full status flag. */
> +			renesas_clear_bit(i3c->regs, NTST, NTST_RDBFF0);
> +			return IRQ_HANDLED;
> +		}

[Severity: High]
Can this early return path cause an interrupt storm?

When a Read Buffer Full interrupt fires, this attempts to handle an aborted
transfer by simply clearing the status flag NTST_RDBFF0. 

Because the hardware fifo remains full and the interrupt enable bit
NTIE_RDBFIE0 is not disabled, the interrupt will immediately re-trigger
when the ISR returns.

[ ... ]

> @@ -1259,6 +1293,12 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			/* Clear any status registers. */
> +			renesas_clear_bit(i3c->regs, BST, BST_STCNDDF);
> +			return IRQ_HANDLED;
> +		}

[Severity: High]
Does this early return path fail to disable the Start Condition interrupt?

The early return clears the BST_STCNDDF status flag but doesn't disable the 
Start Condition interrupt via BIE_STCNDDIE. 

This could leave the interrupt permanently enabled, leading to stray
interrupts whenever a Start condition is detected on the bus.


Additionally, while reviewing this file, I noticed a few pre-existing regressions:

[Severity: High]
This is a pre-existing issue, but are hardware interrupt registers modified
without locking, potentially causing lost mask updates?

In drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers():

    renesas_set_bit(i3c->regs, BIE, BIE_NACKDIE);
    renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
    renesas_set_bit(i3c->regs, BIE, BIE_STCNDDIE);

These configure hardware interrupt registers using renesas_set_bit() from
process context without holding the i3c->xferqueue.lock spinlock.

renesas_set_bit() performs a non-atomic read-modify-write sequence. If a
hardware interrupt fires between the read and the write, the ISR's interrupt
disabling operations (which properly hold the lock) could be entirely
overwritten and lost when the process context writes its stale value back.

[Severity: High]
This is a pre-existing issue, but does renesas_i3c_i3c_xfers() silently ignore
timeouts and hardware errors?

In drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers():

    time_left = renesas_i3c_wait_xfer(i3c, xfer);
    if (!time_left)
        xfer_failed = true;

    if (xfer_failed)
        renesas_i3c_irqs_mask_and_clear(i3c);

    return 0;

If renesas_i3c_wait_xfer() times out, xfer_failed is set to true, but the
loop continues processing subsequent transfers, and the function
unconditionally returns 0 at the end.

Hardware errors propagated via xfer->ret are never checked or returned,
which could lead to silent data corruption as subsystems assume transfers
succeeded.

[Severity: High]
This is a pre-existing issue, but can a timeout cause hardware state desync
and cross-transfer corruption?

In drivers/i3c/master/renesas-i3c.c: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);

When this times out, it dequeues xfer from the software queue but does not
instruct the hardware controller to abort the ongoing physical transaction.

The caller loop in renesas_i3c_i3c_xfers() then immediately queues the next
transfer using the exact same xfer memory allocation:

    struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);

Because the hardware is still processing the first transfer, its delayed
interrupt will match !xfer == false (as cur now points to the same re-queued
xfer structure). The ISR could complete the second transfer prematurely using
the results of the first.

[Severity: High]
This is a pre-existing issue, but can I3C writes overflow the hardware fifo
due to unconditionally writing the entire transfer length?

In drivers/i3c/master/renesas-i3c.c:renesas_i3c_tx_isr():

    } else {
        i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
    }

Both the initial enqueue and the tx interrupt handler blindly call
i3c_writel_fifo() with cmd->len bytes. The hardware fifo depth is fixed at 16
words (64 bytes).

If cmd->len is larger than 64, the fifo could immediately overflow and
corrupt the transfer and hardware state. Also, since cmd->tx_count is
neither used nor updated, each subsequent tx interrupt would repeatedly
write the entire buffer from offset 0.

[Severity: High]
This is a pre-existing issue, but can I3C reads cause an out-of-bounds write
and pointer offset corruption?

In drivers/i3c/master/renesas-i3c.c:renesas_i3c_rx_isr():

    } else {
        read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
        i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
        cmd->rx_count = read_bytes;
    }

When an I3C read triggers, it calculates read_bytes by multiplying the number
of 32-bit words in the hardware fifo by sizeof(u32).

If the requested read length is unaligned (e.g., 2 bytes), reading 4 bytes
could corrupt the adjacent heap memory. Moreover, the pointer cmd->rx_buf is
never advanced, and cmd->rx_count is assigned rather than incremented,
meaning multi-part transfers could repeatedly overwrite the beginning of the
buffer.

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