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

[sashiko-bot@kernel.org]

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?

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