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

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.

With all interrupts disabled once the transfer completed, omit the
clearing of the Transfer Complete bit in sun4i_spi_handler. This is safe
because sun4i_spi_transfer_one() takes care of this before enabling any
interrupts again.

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 | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
  

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

New issues:
- [Medium] The interrupt handler reads the interrupt status without masking it with the interrupt enable register. Combined with the removal of the status bit clearing, this exposes the driver to spurious interrupts causing destructive double-completions.

Pre-existing issues:
- [High] SMP read-modify-write data race on `SUN4I_INT_CTL_REG` between the timeout path and the IRQ handler can re-enable disabled interrupts, leading to Use-After-Free.
- [High] The SPI interrupt is requested before the completion structure is initialized, exposing the driver to a NULL pointer dereference if an early interrupt fires.
--

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

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

    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.

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 8a9dcd3b6b8f4..ea8be0170fbfd 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -333,6 +335,7 @@ 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));
> +
>  	end = jiffies;
>  	if (!time_left) {
>  		dev_warn(&host->dev,
> @@ -340,12 +343,11 @@ 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;
> -		goto out;
> +		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
> +		synchronize_irq(sspi->irq);
>  	}

[Severity: High]
This is a pre-existing issue, but does this timeout path create an SMP read-
modify-write data race with the IRQ handler?

If a timeout occurs, CPU0 executes sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0).
Concurrently on CPU1, if the TF_E34 interrupt fires, the IRQ handler calls
sun4i_spi_disable_interrupt(), which performs a read-modify-write:

drivers/spi/spi-sun4i.c:sun4i_spi_disable_interrupt() {
	u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);

	reg &= ~mask;
	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
}

If CPU1 reads the register before CPU0 writes 0, and writes its value back
afterwards, it could unintentionally re-enable the TC and RF_F34 interrupts.
Could this lead to a use-after-free if the re-enabled interrupt later fires
and writes to a freed SPI buffer?

[ ... ]

> @@ -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;
>  	}

[Severity: Medium]
Will omitting the clearing of the status bit expose the driver to spurious
interrupts causing destructive double-completions?

The interrupt handler reads the status directly from the hardware without
masking it against the enabled interrupts in SUN4I_INT_CTL_REG:

drivers/spi/spi-sun4i.c:sun4i_spi_handler() {
	u32 status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);

	/* Transfer complete */
	if (status & SUN4I_INT_CTL_TC) {
		...
}

Because the pending interrupts are cleared at the start of a transfer in
sun4i_spi_transfer_one():

drivers/spi/spi-sun4i.c:sun4i_spi_transfer_one() {
	reinit_completion(&sspi->done);
	sspi->tx_buf = tfr->tx_buf;
	sspi->rx_buf = tfr->rx_buf;
	sspi->len = tfr->len;

	/* Clear pending interrupts */
	sun4i_spi_write(sspi, SUN4I_INT_STA_REG, ~0);
	...
}

There is a narrow race window here. If a spurious interrupt fires right after
reinit_completion() but before the status register is cleared, the unmasked
status check in the handler could see an old TC flag. Could this cause
complete(&sspi->done) to be called prematurely, leading the SPI core to
tear down an active transfer and result in data corruption?

[ ... ]

> @@ -456,6 +457,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	sspi->irq = irq;
>  	sspi->host = host;
>  	host->max_speed_hz = 100 * 1000 * 1000;
>  	host->min_speed_hz = 3 * 1000;

[Severity: High]
This is a pre-existing issue, but is the IRQ requested before the completion
structure is initialized?

Earlier in sun4i_spi_probe(), the interrupt is requested:

drivers/spi/spi-sun4i.c:sun4i_spi_probe() {
	...
	ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
			       0, "sun4i-spi", sspi);
	...
}

However, init_completion(&sspi->done) is only called later in the probe
sequence. If the bootloader leaves the SPI controller active with a pending
interrupt, it will be unmasked at the GIC and fire immediately when requested.

The IRQ handler would then call complete(&sspi->done) on a zero-allocated,
uninitialized completion structure. Could this result in a NULL pointer
dereference and a kernel panic on boot?
  

Dear humans,

On 14.09.2026 15:50:02, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Medium] The interrupt handler reads the interrupt status without
>   masking it with the interrupt enable register. Combined with the
>   removal of the status bit clearing, this exposes the driver to
>   spurious interrupts causing destructive double-completions.

Spurious interrupts have not been observed with this hardware.

> Pre-existing issues:
> - [High] SMP read-modify-write data race on `SUN4I_INT_CTL_REG` between the timeout path and the IRQ handler can re-enable disabled interrupts, leading to Use-After-Free.
> - [High] The SPI interrupt is requested before the completion structure is initialized, exposing the driver to a NULL pointer dereference if an early interrupt fires.
> --
>
> commit a883667a9345430d63d8a01e0fab4daa8e1e23c6
> Author: Marc Kleine-Budde <mkl@pengutronix.de>
>
>     spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
>
>     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.
>
> > diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> > index 8a9dcd3b6b8f4..ea8be0170fbfd 100644
> > --- a/drivers/spi/spi-sun4i.c
> > +++ b/drivers/spi/spi-sun4i.c
> > @@ -333,6 +335,7 @@ 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));
> > +
> >  	end = jiffies;
> >  	if (!time_left) {
> >  		dev_warn(&host->dev,
> > @@ -340,12 +343,11 @@ 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;
> > -		goto out;
> > +		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
> > +		synchronize_irq(sspi->irq);
> >  	}
>
> [Severity: High]
> This is a pre-existing issue, but does this timeout path create an SMP read-
> modify-write data race with the IRQ handler?
>
> If a timeout occurs, CPU0 executes sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0).
> Concurrently on CPU1, if the TF_E34 interrupt fires, the IRQ handler calls
> sun4i_spi_disable_interrupt(), which performs a read-modify-write:
>
> drivers/spi/spi-sun4i.c:sun4i_spi_disable_interrupt() {
> 	u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);
>
> 	reg &= ~mask;
> 	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
> }
>
> If CPU1 reads the register before CPU0 writes 0, and writes its value back
> afterwards, it could unintentionally re-enable the TC and RF_F34 interrupts.
> Could this lead to a use-after-free if the re-enabled interrupt later fires
> and writes to a freed SPI buffer?
>
> [ ... ]
>
> > @@ -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;
> >  	}
>
> [Severity: Medium]
> Will omitting the clearing of the status bit expose the driver to spurious
> interrupts causing destructive double-completions?

Spurious interrupts have not been observed with this hardware.

> The interrupt handler reads the status directly from the hardware without
> masking it against the enabled interrupts in SUN4I_INT_CTL_REG:
>
> drivers/spi/spi-sun4i.c:sun4i_spi_handler() {
> 	u32 status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);
>
> 	/* Transfer complete */
> 	if (status & SUN4I_INT_CTL_TC) {
> 		...
> }
>
> Because the pending interrupts are cleared at the start of a transfer in
> sun4i_spi_transfer_one():
>
> drivers/spi/spi-sun4i.c:sun4i_spi_transfer_one() {
> 	reinit_completion(&sspi->done);
> 	sspi->tx_buf = tfr->tx_buf;
> 	sspi->rx_buf = tfr->rx_buf;
> 	sspi->len = tfr->len;
>
> 	/* Clear pending interrupts */
> 	sun4i_spi_write(sspi, SUN4I_INT_STA_REG, ~0);
> 	...
> }
>
> There is a narrow race window here. If a spurious interrupt fires right after
> reinit_completion() but before the status register is cleared, the unmasked
> status check in the handler could see an old TC flag. Could this cause
> complete(&sspi->done) to be called prematurely, leading the SPI core to
> tear down an active transfer and result in data corruption?

Spurious interrupts have not been observed with this hardware.

regards,
Marc