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

From: Marc Kleine-Budde <mkl@pengutronix.de>

In commit 6d9fe44bd73d ("spi: sun4i: fix FIFO limit"), the TX FIFO is
filled max to SUN4I_FIFO_DEPTH - 1 (= 63) bytes to work around
timeouts observed on A10s SoCs.

Commit 196737912da5 ("spi: sun4i: Allow transfers larger than FIFO
size") added support for transfers larger than the FIFO size. This
commit did not consider the A10 workaround (limit the TX FIFO fill
size to SUN4I_FIFO_DEPTH - 1 (= 63) bytes) for refilling the TX FIFO
in the IRQ handler.

To apply the workaround independent from where sun4i_spi_fill_fifo()
is called, remove the length argument from the function and directly
take the max fill level of SUN4I_FIFO_DEPTH - 1 into account when
calculating the free space in the FIFO.

Due to the lack of HW this patch has not been tested on an A10 SoC,
but on an A20 SoC. It was not possible to reproduce the timeout on the
A20.

Fixes: 196737912da5 ("spi: sun4i: Allow transfers larger than FIFO size")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 drivers/spi/spi-sun4i.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)
  

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.

[ ... ]
  

Dear humans,

On 14.09.2026 15:50:08, sashiko-bot@kernel.org wrote:
> 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?

Neither spurious interrupts nor more bytes than requested have been
observed. So far we trust the hardware.

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

With this change the hardware TX buffer is only filled up to
SUN4I_FIFO_DEPTH - 1, so it will never report to be filled with
SUN4I_FIFO_DEPTH.

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

regards,
Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |