Re: [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround

[sashiko-bot@kernel.org]

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

New issues:
- [High] Integer underflow in `sun4i_spi_fill_fifo()` causes an infinite loop and buffer overflow when calculating available TX FIFO space.

Pre-existing issues:
- [High] Unbounded writes to the receive buffer in `sun4i_spi_drain_fifo` can cause a heap buffer overflow.
--

commit 293e40dd169fc9a0b7d04d7abe8b6d6292a688f1
Author: Marc Kleine-Budde <mkl@pengutronix.de>

spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround

This commit removes the length argument from sun4i_spi_fill_fifo() and
directly limits the TX FIFO fill level internally to work around timeouts
on A10s SoCs. It ensures the workaround is always applied regardless of
where the FIFO is refilled.

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 2e23244539053..3649bcabcc9a7 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -142,15 +142,21 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does sun4i_spi_drain_fifo() lack bounds
checking against the allocated receive buffer size?

Looking at sun4i_spi_drain_fifo():

static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
{
        ...
        if (len > cnt)
                len = cnt;

        while (len--) {
                byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);
                if (sspi->rx_buf)
                        *sspi->rx_buf++ = byte;
        }
}

Data from the hardware is read based on what the hardware reports as
available, and unconditionally written to sspi->rx_buf. It doesn't appear
to track the total bytes written against the transfer length limit.

If the hardware asserts spurious interrupts or reports more bytes than
requested (e.g., when called from sun4i_spi_handler with SUN4I_FIFO_DEPTH),
couldn't this blindly advance the pointer past the end of the allocation
and cause a heap buffer overflow?

>  
> -static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len)
> +static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi)
>  {
>  	u32 cnt;
> +	int len;
>  	u8 byte;
>  
> -	/* See how much data we can fit */
> -	cnt = SUN4I_FIFO_DEPTH - sun4i_spi_get_tx_fifo_count(sspi);
> +	/*
> +	 * See how much data we can fit
> +	 *
> +	 * Filling the FIFO fully causes timeout for some reason
> +	 * at least on spi2 on A10s
> +	 */
> +	cnt = SUN4I_FIFO_DEPTH - 1 - sun4i_spi_get_tx_fifo_count(sspi);
>  
> -	len = min3(len, (int)cnt, sspi->len);
> +	len = min_t(int, cnt, sspi->len);
>  
>  	while (len--) {
>  		byte = sspi->tx_buf ? *sspi->tx_buf++ : 0;

[Severity: High]
Can this subtraction underflow if the hardware reports a full FIFO?

If sun4i_spi_get_tx_fifo_count(sspi) returns 64 (the physical maximum
depth), cnt will evaluate to 63 - 64 = -1. Because cnt is a u32, this
underflows to 0xFFFFFFFF.

The min_t() macro then compares (int)0xFFFFFFFF (which is -1) against
sspi->len. Assuming sspi->len is greater than zero, min_t() returns -1.

The subsequent while (len--) loop will post-decrement on the negative
integer and could spin indefinitely, leading to out-of-bounds reads from
sspi->tx_buf and continuous writes to the memory-mapped register.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-spi-sun4i-fixes-v3-0-9fe919d5cecf@pengutronix.de?part=2