Re: [PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it

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

Pre-existing issues:
- [High] The I3C and I2C transfer functions ignore hardware timeouts and errors, erroneously returning success (0) and continuing operations.
- [High] DAA transfer timeouts are treated as successful assignments, registering phantom devices on the bus.
- [High] The patch's NULL-deref fix is incomplete: failing to abort the hardware on timeout allows delayed interrupts to corrupt subsequent enqueued transfers.
--

--- Patch [1]: [PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it ---
commit 34ed2a5d1d43de932d3b546c4674b92c1ec5bc03
Author: Claudiu Beznea <[email protected]>

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.

If the completion times out, the waiting thread dequeues the transfer and
free it. If an interrupt fires after that, the handler may access freed
memory, leading to crashes.

Check that the transfer is still valid before accessing it in the
interrupt handler. With it clear any status flags and disable all
the interrupts to avoid triggering the same interrupts again.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: [email protected]
Signed-off-by: Claudiu Beznea <[email protected]>

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..38b8428f464c3 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1014,6 +1029,11 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;

[Severity: High]
This is a pre-existing issue, but does this fix leave the hardware in a state
where delayed interrupts can corrupt subsequent transfers?

If a transfer times out in renesas_i3c_wait_xfer(), it is dequeued but the
hardware is not aborted:

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

If a new transfer B is enqueued and becomes i3c->xferqueue.cur, and then the
delayed interrupt for the timed-out transfer A fires, xfer will not be NULL.

The ISR would then process transfer A's hardware events against transfer B's
commands, which could lead to cross-transfer data corruption.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing I noticed that
hardware timeouts and errors appear to be ignored in the I3C and I2C transfer
functions, which could lead to data corruption or protocol failures.

In renesas_i3c_i3c_xfers(), the error state in xfer->ret is ignored entirely
and the function unconditionally returns 0:

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

	return 0;
}

Does this code hide failures by falsely assuming operations succeeded?

Similarly, in renesas_i3c_i2c_xfers(), wait_for_completion_timeout() returns 0
on timeout, but cmd->err remains 0 (its zero-initialized value):

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

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

Is it possible for this loop to continue operations and eventually return 0
despite a hardware timeout?

[Severity: High]
This is another pre-existing issue, but I also noticed that DAA transfer
timeouts might be treated as successful assignments.

In renesas_i3c_daa(), if the DAA command times out, cmd->rx_count remains 0:

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

	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
	newdevs &= ~olddevs;

	for (pos = 0; pos < i3c->maxdevs; pos++) {
		if (newdevs & BIT(pos))
			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
	}
    ...
}

Does this generate a full mask of new devices and incorrectly register them on
the bus when the bus hangs or a device fails DAA?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c