Re: [PATCH v3 1/3] i3c: master: dw: 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:
- [Critical] Heap/stack buffer overflow when reading from the hardware RX FIFO.
- [High] Transfer queue stall and cascading timeouts after a single transfer timeout.
- [High] Spurious completion of new transfers due to unhandled late interrupts from previous timeouts.
--

commit 097603b8b0f1e852cd40f174d2e0bd01af652c87
Author: Adrian Ng Ho Yin <[email protected]>

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

On successful GET CCC transfers, set dests[0].payload.len from
RESPONSE_PORT_DATA_LEN so the I3C core receives the number of bytes
actually read. Core helpers such as i3c_master_getmrl_locked() use
dest.payload.len after the transfer to interpret the response.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index a7593d6efac5a..e42cf3ddcec8f 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c

[ ... ]

> @@ -780,7 +783,10 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
>  		dw_i3c_master_dequeue_xfer(master, xfer);

[Severity: High]
This is a pre-existing issue, but does dw_i3c_master_dequeue_xfer_locked()
properly restart the queue after a timeout?

When a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the
hardware and clears master->xferqueue.cur:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_dequeue_xfer_locked() {
    ...
		master->xferqueue.cur = NULL;

		writel(RESET_CTRL_RX_FIFO | RESET_CTRL_TX_FIFO |
		       RESET_CTRL_RESP_QUEUE | RESET_CTRL_CMD_QUEUE,
		       master->regs + RESET_CTRL);
    ...
}

However, it doesn't seem to dequeue the next transfer from
master->xferqueue.list and start it.

Because cur is NULL and no code advances the queue, will all pending transfers
remain stranded and individually time out unless a new transfer is enqueued?

[Severity: High]
This is a pre-existing issue, but can unhandled late interrupts from a timeout
cause spurious completions for new transfers?

If a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the
hardware queues, dropping nresp to 0, but it does not clear latched interrupt
statuses like INTR_TRANSFER_ERR_STAT.

If a new transfer is enqueued before the delayed IRQ handler acquires the
lock, the ISR will fire based on the old interrupt.

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_end_xfer_locked() {
    ...
	nresp = readl(master->regs + QUEUE_STATUS_LEVEL);
	nresp = QUEUE_STATUS_LEVEL_RESP(nresp);

	for (i = 0; i < nresp; i++) {
		// ... skipped if nresp == 0
    ...
}

dw_i3c_master_end_xfer_locked() reads nresp = 0 and skips the error parsing
loop, leaving ret = 0.

Will this erroneously complete the newly enqueued transfer with a success
status without actually executing it on the bus?

>  
>  	ret = xfer->ret;
> -	if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
> +	cmd = &xfer->cmds[0];
> +	if (!ret)
> +		ccc->dests[0].payload.len = cmd->rx_len;

[Severity: Critical]
This is a pre-existing issue, but is there a risk of a buffer overflow when
reading from the hardware RX FIFO?

In dw_i3c_master_end_xfer_locked(), the driver reads the response length from
the hardware and directly overwrites cmd->rx_len:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_end_xfer_locked() {
    ...
		cmd->rx_len = RESPONSE_PORT_DATA_LEN(resp);
		cmd->error = RESPONSE_PORT_ERR_STATUS(resp);
		if (cmd->rx_len && !cmd->error)
			dw_i3c_master_read_rx_fifo(master, cmd->rx_buf,
						   cmd->rx_len);
    ...
}

The length from the hardware is not validated against the originally requested
buffer size.

If a peripheral sends more data than requested, could this directly overflow
the cmd->rx_buf into adjacent memory?

It seems the payload length validation introduced in subsequent patches of this
series only checks the length after the buffer has already been overwritten.

> +	if (cmd->error == RESPONSE_ERROR_IBA_NACK)
>  		ccc->err = I3C_ERROR_M2;
>  
>  	return ret;

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