Re: [PATCH v4 05/16] i3c: renesas: Reset the controller on resume

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

Pre-existing issues:
- [Critical] Missing hardware cancellation on transfer timeout causes use-after-free and memory corruption.
- [High] The driver ignores transfer timeouts and errors, silently reporting success.
--

commit 06142498cb9164f084dca924bf2625138121a705
Author: Claudiu Beznea <[email protected]>

i3c: renesas: Reset the controller on resume

This commit updates renesas_i3c_reset() to use read_poll_timeout_atomic()
since the resume callback runs in the noirq phase. It also resets the
controller on resume to follow the probe sequence and avoid ordering
related failures.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 4b81c32b1fdec..67b9a31300d09 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -1504,6 +1508,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>  
>  	return 0;
>  
> +err_clks_disable:
> +	clk_bulk_disable(i3c->num_clks, i3c->clks);
>  err_presetn:
>  	reset_control_assert(i3c->presetn);
>  err_tresetn:

[Severity: Critical]
This is a pre-existing issue, but does the driver have a structural flaw
with transfer timeouts causing memory corruption?

When a transfer times out in renesas_i3c_wait_xfer(), the transfer is
dequeued and can be freed:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_wait_xfer() {
    ...
    if (!time_left)
        renesas_i3c_dequeue_xfer(i3c, xfer);
    ...
}

However, the active hardware transaction is never aborted. If the target
eventually completes the transaction, the hardware will write a response
descriptor to NRSPQP.

When a subsequent transfer is initiated, the driver re-enables the interrupt,
and the ISR will fire, picking up the new transfer:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
    ...
    xfer = i3c->xferqueue.cur;
    ...
    data_len = NRSPQP_DATA_LEN(resp_descriptor);
    ...
    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
    ...
}

Will the ISR blindly read the stale data from the old completed hardware
transaction into the new transfer's rx_buf?

If the old transfer's length exceeds the new transfer's buffer size, couldn't
this overflow the buffer and cause memory corruption?

[Severity: High]
This is a pre-existing issue, but does the driver ignore transfer timeouts
and errors, silently reporting success?

In renesas_i3c_i3c_xfers(), the function ignores transfer timeouts and errors,
returning 0 unconditionally:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
    ...
    if (xfer_failed)
        renesas_i3c_irqs_mask_and_clear(i3c);

    return 0;
}

Similarly, in renesas_i3c_i2c_xfers():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
    ...
    if (xfer_failed)
        renesas_i3c_irqs_mask_and_clear(i3c);

    return cmd->err;
}

Since cmd->err is only updated to a negative code by the ISR upon an explicit
NACK, it remains 0 on timeout. Doesn't this falsely claim that the message
was successfully delivered to the core I3C subsystem?

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

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