Re: [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Endianness Portability Bug: Register accessors used for stream FIFOs
- [High] Out-of-bounds Read when handling trailing bytes in TX FIFO
- [High] Unaligned Memory Access when casting caller-provided buffer
--

commit 88ae252ec612f81d6b6f55d8dd0ef78352a438bf
Author: Billy Tsai <billy_tsai@aspeedtech.com>

i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold

This commit introduces a new quirk, HCI_QUIRK_TX_START_THLD, to clear the
TX start threshold in DMA mode on ASPEED platforms to prevent transfer
stalls. It also moves the PIO access area definitions from pio.c to a new
pio.h header file so they can be shared.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
> index 439578a6eb54c..6bd5e43364a1d 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/pio.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/pio.c

[ ... ]

While reviewing this file, I noticed a few pre-existing issues that are not
introduced by this patch, but remain uncorrected in this series.

In drivers/i3c/master/mipi-i3c-hci/pio.c:hci_pio_do_rx():

	while (xfer->data_left >= 4) {
		/* bail out if FIFO hasn't reached the threshold value yet */
		if (!(pio_reg_read(INTR_STATUS) & STAT_RX_THLD))
			return false;
		nr_words = min(xfer->data_left / 4, pio->rx_thresh_size);
		/* extract data from FIFO */
		xfer->data_left -= nr_words * 4;
		dev_dbg(&hci->master.dev, "now %d left %d",
			nr_words * 4, xfer->data_left);
		while (nr_words--)
			*p++ = pio_reg_read(XFER_DATA_PORT);
	}

[Severity: High]
This isn't a bug introduced by this patch, but does reading from the stream
FIFO (XFER_DATA_PORT) using pio_reg_read() cause unintended byteswapping on
big-endian architectures?

According to the I/O Accessors Subsystem Guidelines, stream FIFOs must
exclusively use stream accessors (readsl() / writesl()) to avoid corrupting
the I3C byte stream payload on big-endian systems.

Additionally, if the caller provides an arbitrary buffer in xfer->data that
is not aligned to a 4-byte boundary, doesn't casting it directly to a u32
pointer and dereferencing it via *p++ cause an alignment fault (kernel
panic) on architectures that do not support hardware unaligned accesses?

The same patterns appear in hci_pio_do_tx() for writes:

drivers/i3c/master/mipi-i3c-hci/pio.c:hci_pio_do_tx():

	while (xfer->data_left >= 4) {
		/* bail out if FIFO free space is below set threshold */
		if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
			return false;
		/* we can fill up to that TX threshold */
		nr_words = min(xfer->data_left / 4, pio->tx_thresh_size);
		/* push data into the FIFO */
		xfer->data_left -= nr_words * 4;
		dev_dbg(&hci->master.dev, "now %d left %d",
			nr_words * 4, xfer->data_left);
		while (nr_words--)
			pio_reg_write(XFER_DATA_PORT, *p++);
	}

[Severity: High]
This is a pre-existing issue, but does writing to the TX stream FIFO using
pio_reg_write() also byteswap the data incorrectly on big-endian
architectures, and does it similarly suffer from unaligned memory access
panics?

Further down in hci_pio_do_tx(), trailing bytes are handled:

	if (xfer->data_left) {
		/*
		 * There are trailing bytes to send. We can simply load
		 * them from memory as a word which will keep those bytes
		 * in their proper place even on a BE system. This will
		 * also get some bytes past the actual buffer but no one
		 * should care as they won't be sent out.
		 */
		if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
			return false;
		dev_dbg(&hci->master.dev, "trailing %d", xfer->data_left);
		pio_reg_write(XFER_DATA_PORT, *p);
		xfer->data_left = 0;
	}

[Severity: High]
This isn't a bug introduced by this patch, but does the 32-bit dereference
of *p for the trailing bytes read past the end of the allocated buffer?

If an I3C transfer occurs where the provided payload buffer length leaves
1 to 3 trailing bytes and unprivileged userspace passes an unpadded buffer
via an ioctl, reading past the allocation could trigger KASAN splats or
cause a fatal page fault if the buffer ends exactly at a page boundary or
a SLUB debugging redzone.

Finally, in hci_pio_get_ibi_segment():

drivers/i3c/master/mipi-i3c-hci/pio.c:hci_pio_get_ibi_segment():

		/* extract the data from the IBI port */
		nr_words = thresh_val;
		ibi->seg_cnt -= nr_words * 4;
		dev_dbg(&hci->master.dev, "now %d left %d",
			nr_words * 4, ibi->seg_cnt);
		while (nr_words--)
			*p++ = pio_reg_read(IBI_PORT);

[Severity: High]
This is a pre-existing issue, but does reading from the IBI FIFO stream
using pio_reg_read() incorrectly byteswap the data payload on big-endian
platforms?

Does this need to be updated to use readsl() or similar stream accessors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=4

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c