Re: [PATCH v6 2/5] i3c: master: Report actual GET CCC payload length on success

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the Renesas I3C driver suffers from a KASAN out-of-bounds write and data corruption due to unbounded FIFO reads and missing buffer pointer accumulation.
- [High] This problem wasn't introduced by this patch, but the Silvaco I3C master driver mixes 32-bit register accessors (`readl`) and 8-bit stream accessors (`readsb`) on the same `SVC_I3C_MRDATAB` FIFO, causing endianness portability bugs.
- [High] This problem wasn't introduced by this patch, but the missing hardware abort on transfer timeout in the Renesas I3C driver leads to NULL pointer dereferences or cross-transfer state corruption.
--

--- Patch [2]: [PATCH v6 2/5] i3c: master: Report actual GET CCC payload length on success ---
commit fb225c2de835cd1f0fc73a0e9829ada753363f62
Author: Adrian Ng Ho Yin <[email protected]>

i3c: master: Report actual GET CCC payload length on success

Set dests[].payload.actual_len on successful GET CCC transfers in I3C
master drivers so the core can distinguish requested and received
buffer lengths. Switch core GET helpers to use actual_len instead of
len when interpreting GET CCC results.

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

[ ... ]

> @@ -1072,10 +1074,16 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)

[Severity: High]
This isn't a bug introduced by this patch, but what happens if a transfer
times out and we later receive a delayed hardware completion in this ISR?

If a timeout occurs, renesas_i3c_dequeue_xfer_locked() removes the transfer
from the software queue:

    if (i3c->xferqueue.cur == xfer)
        i3c->xferqueue.cur = NULL;

If this happens without issuing an abort command to the hardware, when the
hardware eventually completes the timed-out transfer, wouldn't
renesas_i3c_resp_isr() blindly dereference the now-NULL i3c->xferqueue.cur
pointer when pulling the command state?

    xfer = i3c->xferqueue.cur;
    cmd = xfer->cmds;
    ...
    cmd->rx_count = data_len;

Could this lead to a kernel panic or cross-transfer state corruption?

>  			break;
>  		case I3C_INTERNAL_STATE_CONTROLLER_READ:
>  		case I3C_INTERNAL_STATE_CONTROLLER_COMMAND_READ:
> -			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);
> +			if (!cmd->err) {
> +				u32 rx_count = min(cmd->rx_count, data_len);

[Severity: High]
This isn't a bug introduced by this patch, but while clamping rx_count
here to prevent unsigned underflows, is it possible we still have a memory
corruption issue originating in renesas_i3c_rx_isr()?

Currently, renesas_i3c_rx_isr() calculates read_bytes rounded up to a
4-byte multiple and reads into cmd->rx_buf:

    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;

Could this write past the end of the allocated payload buffer for small
unaligned GET CCCs?

Additionally, for large transfers triggering multiple interrupts, it looks
like rx_count is overwritten instead of accumulated, and the rx_buf pointer
is not advanced. Could this cause subsequent chunks to overwrite the
beginning of the buffer?

> +
> +				bytes_remaining = data_len - rx_count;
> +				if (bytes_remaining)
> +					i3c_readl_fifo(i3c->regs + NTDTBP0,
> +						       cmd->rx_buf + rx_count,
> +						       bytes_remaining);
> +				cmd->rx_count = data_len;
> +			}
>  			renesas_clear_bit(i3c->regs, NTIE, NTIE_RDBFIE0);
>  			break;
>  		default:

[ ... ]

> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 486c22840ea9a..e700392f0801f 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c

[ ... ]

> @@ -1725,8 +1725,8 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
>  		svc_i3c_master_dequeue_xfer(master, xfer);
>  	mutex_unlock(&master->lock);
>  
> -	if (cmd->actual_len != xfer_len)
> -		ccc->dests[0].payload.len = cmd->actual_len;
> +	if (ccc->rnw)
> +		ccc->dests[0].payload.actual_len = cmd->actual_len;

[Severity: High]
This isn't a bug introduced by this patch, but looking at the Silvaco driver
as a whole, is there an endianness portability issue with the FIFO reads?

In svc_i3c_master_read(), the 32-bit register accessor readl() is used
to read from the stream FIFO, which performs a CPU-to-LE byte-swap:

    for (i = 0; i < count; i++)
        in[offset + i] = readl(master->regs + SVC_I3C_MRDATAB);

However, svc_i3c_master_handle_ibi() accesses the exact same FIFO using
the 8-bit stream accessor readsb(), which does no byte-swapping:

    readsb(master->regs + SVC_I3C_MRDATAB, buf, count);

Could mixing these accessor families cause data corruption on big-endian
architectures due to inconsistent byte-swapping behavior?

>  
>  	ret = xfer->ret;
>  	svc_i3c_master_free_xfer(xfer);

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

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