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

[Claudiu Beznea]

On 6/12/26 19:20, sashiko-bot at kernel.org wrote:
> 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?

Yes, it is a typo.

> 
> 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 scope of the code was to disable the IRQs that are disable on the path were 
the xfer is valid and clear the same bits, to avoid re-triggering the handler 
execution again.

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

I'm not sure if that can happen. The scope of the code was to touch the IRQ 
disable and status bits in the handlers they are handled on the normal path. 
However, I'll review it.

> 
> [ ... ]
> 
>> @@ -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.

Yes, that should have been done.

-- 
Thank you,
Claudiu