diff mbox series

[4/5] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt

Message ID 20260902-spi-sun4i-fixes-v1-4-19985ef75673@pengutronix.de (mailing list archive)
State New
Headers
Series spi: spi-sun4i: assorted fixes |

Commit Message

Jonas Rebmann Sept. 2, 2026, 3:35 p.m. UTC 
From: Marc Kleine-Budde <mkl@pengutronix.de>

In commit 196737912da5 ("spi: sun4i: Allow transfers larger than FIFO
size"), support for transfers larger than the FIFO size was added.

This commit moves the draining of the RX-FIFO from
sun4i_spi_transfer_one() (after completion of the transfer) to the IRQ
handler when the IRQ "transfer complete" is handled. In addition, the
IRQ "RX-FIFO 3/4 full" is activated for all transfers.

However, this does not take into account that the RX-IRQ for transfers
that exceed 3/4 of the FIFO size is still pending after the IRQ
"transfer complete" has been processed. All interrupt sources are only
deactivated after the wait_for_completion_timeout() in
sun4i_spi_transfer_one().

This opens a race window for "RX-FIFO 3/4 full" interrupts to come.
The sequence is as follows:

| sun4i_spi_transfer_one()
|     sun4i_spi_fill_fifo()       // fill TX-FIFO with 48 bytes
|     // enable RX-FIFO 3/4 full IRQ
|     wait_for_completion_timeout();
|
| // SPI controller transfers 48 bytes
| // SPI controller issues "transfer complete" and "RX-FIFO 3/4 full" IRQ
|
| // IRQ handler start
|     sun4i_spi_handler()
|         // ACK "transfer complete" IRQ
|         sun4i_spi_drain_fifo();
|         complete();                         ----.
|         return IRQ_HANDLED;                      \
| // IRQ handler end                                \__ race
|                                                   /   window
|     // wait_for_completion_timeout() continues   /
|     // disable all IRQ sources              ----'

Avoid the race condition by disabling all interrupts when handling the
"transfer complete" IRQ and before calling complete(). Also move the
draining of the RX-FIFO back into sun4i_spi_transfer_one() where it
was before commit 196737912da5 ("spi: sun4i: Allow transfers larger than
FIFO size").

This has the added benefit of spending a little less time in the IRQ
handler.

Cc: Tobias Schramm <t.schramm@manjaro.org>
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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 2, 2026, 3:47 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Data race and out-of-bounds write due to unsafe concurrent execution of `sun4i_spi_drain_fifo()` on the timeout path.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing IRQ synchronization on the timeout error path leads to use-after-free and state corruption.
--

commit 2cab2ade096996934bf270db7137df45f943bc9b
Author: Marc Kleine-Budde <mkl@pengutronix.de>

spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt

This commit moves the RX-FIFO draining back to sun4i_spi_transfer_one() and
attempts to close a race window by disabling interrupts early in the IRQ
handler.

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index ae009d5984505..18951c1972da6 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -333,6 +333,9 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
>  	start = jiffies;
>  	time_left = wait_for_completion_timeout(&sspi->done,
>  						msecs_to_jiffies(tx_time));
> +
> +	sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
> +

[Severity: High]
Can this unconditionally drain the FIFO while interrupts are still enabled?

If wait_for_completion_timeout() times out, the main thread will call
sun4i_spi_drain_fifo(). Because SPI interrupts are not disabled until the
subsequent timeout error block, a SUN4I_INT_CTL_RF_F34 interrupt can
fire concurrently.

This allows sun4i_spi_handler() to also invoke sun4i_spi_drain_fifo()
simultaneously:

CPU1:
sun4i_spi_transfer_one()
    sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
        if (sspi->rx_buf)
            *sspi->rx_buf++ = byte;

CPU2:
sun4i_spi_handler()
    if (status & SUN4I_INT_CTL_RF_F34) {
        sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
            if (sspi->rx_buf)
                *sspi->rx_buf++ = byte;

Could this concurrent execution lead to non-atomic increments of sspi->rx_buf,
pointer corruption, and potential out-of-bounds memory writes?

>  	end = jiffies;
>  	if (!time_left) {
>  		dev_warn(&host->dev,
> @@ -340,13 +343,12 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
>  			 dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>  			 jiffies_to_msecs(end - start), tx_time);
>  		ret = -ETIMEDOUT;
> +		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
>  		goto out;
>  	}
>  
>  
>  out:
> -	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
> -
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the timeout error path
need synchronization with currently executing IRQ handlers?

If an SPI transfer times out while the IRQ handler is already executing on
another CPU, the transfer function will disable interrupts at the controller
level, return -ETIMEDOUT, and the SPI core will tear down the transfer.

Since the code does not wait for in-flight handlers using synchronize_irq()
or a lock, the still-running sun4i_spi_handler() can subsequently write to
freed buffers in sun4i_spi_drain_fifo(), or call complete(&sspi->done)
which corrupts the state of the next transfer.

[ ... ]

> @@ -357,8 +359,7 @@ static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
>  
>  	/* Transfer complete */
>  	if (status & SUN4I_INT_CTL_TC) {
> -		sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
> -		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
> +		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
>  		complete(&sspi->done);
>  		return IRQ_HANDLED;
>  	}
diff mbox series

Patch

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index ae009d598450..18951c1972da 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -333,6 +333,9 @@  static int sun4i_spi_transfer_one(struct spi_controller *host,
 	start = jiffies;
 	time_left = wait_for_completion_timeout(&sspi->done,
 						msecs_to_jiffies(tx_time));
+
+	sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+
 	end = jiffies;
 	if (!time_left) {
 		dev_warn(&host->dev,
@@ -340,13 +343,12 @@  static int sun4i_spi_transfer_one(struct spi_controller *host,
 			 dev_name(&spi->dev), tfr->len, tfr->speed_hz,
 			 jiffies_to_msecs(end - start), tx_time);
 		ret = -ETIMEDOUT;
+		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
 		goto out;
 	}
 
 
 out:
-	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
-
 	return ret;
 }
 
@@ -357,8 +359,7 @@  static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
 
 	/* Transfer complete */
 	if (status & SUN4I_INT_CTL_TC) {
-		sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
-		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
 		complete(&sspi->done);
 		return IRQ_HANDLED;
 	}