Re: [PATCH v8 10/18] spi: cadence-quadspi: refactor direct read path for PHY support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `cqspi_memcpy_fromio()` incorrectly byte-swaps the memory-mapped byte stream on big-endian architectures.
- [High] `cqspi_memcpy_fromio()` triggers fatal alignment faults on strict-alignment architectures if the source address is odd.
- [High] `cqspi_memcpy_fromio()` is vulnerable to integer underflow and buffer overflow if called with a count of 1.
--

--- Patch [10]: [PATCH v8 10/18] spi: cadence-quadspi: refactor direct read path for PHY support ---
commit 590a74a26c162a104820e05cc0e00a87a2e899bf
Author: Santhosh Kumar K <[email protected]>

spi: cadence-quadspi: refactor direct read path for PHY support

This patch extracts the DMA transfer code from cqspi_direct_read_execute()
into a new cqspi_direct_read_dma() helper. It also adds cqspi_memcpy_fromio()
to handle non-DMA transfers, using 2-byte-aligned I/O reads for 8D-8D-8D mode
to satisfy the minimum 16-bit AHB transaction width required by the Cadence
OSPI controller.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> +static void cqspi_memcpy_fromio(const struct spi_mem_op *op, void *to,
> +				const void __iomem *from, size_t count)
> +{
> +	if (op->data.buswidth == 8 && op->data.dtr) {
> +		unsigned long from_addr = (unsigned long)from;
> +
> +		/* Handle unaligned start with 2-byte read */
> +		if (count && !IS_ALIGNED(from_addr, 4)) {
> +			put_unaligned_le16(__raw_readw(from), to);
> +			from += 2;
> +			to += 2;
> +			count -= 2;
> +		}

[Severity: High]
If an unprivileged user initiates a 1-byte SPI read at an unaligned address
(e.g., offset 2), this block in cqspi_memcpy_fromio() executes when count
is 1.

Since it writes 2 bytes into the 1-byte destination buffer, doesn't this
cause a buffer overflow?

Also, count -= 2 will underflow the unsigned size_t count to SIZE_MAX,
leading to a massive out-of-bounds read/write loop in the while statement
below.

Could we ensure count >= 2 before performing a 2-byte read?

[Severity: High]
If from_addr is an odd address (1-byte aligned), __raw_readw(from) in
cqspi_memcpy_fromio() attempts an unaligned 16-bit MMIO read.

On strict-alignment architectures like ARM, wouldn't an unaligned 16-bit
MMIO read trigger a Data Abort (Alignment Fault)?

Furthermore, adding 2 to an odd address results in another odd address,
meaning the subsequent __raw_readl() loop will also fault.

> +
> +		/*
> +		 * Use an explicit 32-bit read loop for the bulk; this
> +		 * guarantees the minimum 16-bit AHB transaction width required
> +		 * in 8D-8D-8D mode on all architectures.
> +		 */
> +		while (count >= 4) {
> +			put_unaligned_le32(__raw_readl(from), to);

[Severity: High]
Does this combination of __raw_readl() and put_unaligned_le32() in
cqspi_memcpy_fromio() corrupt data on big-endian architectures?

On a big-endian CPU, __raw_readl() performs a native 32-bit load. For
example, reading the byte stream [A, B, C, D] results in the value
0xAABBCCDD.

Passing this native 32-bit value to put_unaligned_le32() stores it in
little-endian format, writing out [D, C, B, A] and reversing the bytes in
every 4-byte chunk.

Should this use stream I/O functions like ioread32_rep(), or pair native
loads with native stores to preserve the original stream order regardless
of CPU endianness?

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