Re: [PATCH spi-next 3/3] spi: spi-fsl-lpspi: cache content of Interrupt Enable Register (IER)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: spi-fsl-lpspi: UAF on transfer buffers due to missing synchronize_irq()
--

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

spi: spi-fsl-lpspi: cache content of Interrupt Enable Register (IER)

To improve performance avoid reading the Interrupt Enable Register (IER)
from the hardware, cache in struct fsl_lpspi_data::reg_ier.

Introduce struct fsl_lpspi_data::reg_ier, to cache the current value.
Extend fsl_lpspi_intctrl() to update the cache and replace the open coded
writing of the IRQ register in fsl_lpspi_prepare_message() with
fsl_lpspi_intctrl().

In fsl_lpspi_isr() don't read the IER from the hardware, but use the cached
value from struct fsl_lpspi_data::reg_ier.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index b25f94b2ef738..5b158b0cb5af5 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c

[ ... ]

> @@ -199,6 +200,7 @@ LPSPI_BUF_TX(u32)
>  static void fsl_lpspi_intctrl(struct fsl_lpspi_data *fsl_lpspi,
>  			      unsigned int enable)
>  {
> +	fsl_lpspi->reg_ier = enable;

[Severity: High]
Could modifying reg_ier without locks here introduce a data race with the ISR?

If an SPI PIO transfer times out or is aborted, fsl_lpspi_reset() is called to
disable interrupts, but it does not call synchronize_irq().

Because the ISR caches reg_ier into temp_IER and then later restores it by
calling fsl_lpspi_intctrl(fsl_lpspi, IER_FCIE | (temp_IER & IER_TDIE)), it can
re-enable hardware interrupts and overwrite reg_ier after fsl_lpspi_reset() has
disabled them.

This actively widens the window for a use-after-free regression on the
transfer buffers, as the SPI core frees them immediately after
fsl_lpspi_reset() returns.

>  	writel(enable, fsl_lpspi->base + IMX7ULP_IER);
>  }
>  

[ ... ]

> @@ -808,7 +810,7 @@ static irqreturn_t fsl_lpspi_isr(int irq, void *dev_id)
>  	u32 temp_SR, temp_IER;
>  	struct fsl_lpspi_data *fsl_lpspi = dev_id;
>  
> -	temp_IER = readl(fsl_lpspi->base + IMX7ULP_IER);
> +	temp_IER = fsl_lpspi->reg_ier;
>  	fsl_lpspi_intctrl(fsl_lpspi, 0);
>  	temp_SR = readl(fsl_lpspi->base + IMX7ULP_SR);
>  

[Severity: High]
Can a concurrent or delayed ISR dereference stale rx_buf and tx_buf pointers
here?

When an SPI PIO transfer times out or is aborted, the SPI core returns to the
caller and the transfer buffers (often on the stack) are freed. If the
ISR executes concurrently because synchronize_irq() is missing in the timeout
path, it will unconditionally call the FIFO helpers:

drivers/spi/spi-fsl-lpspi.c:fsl_lpspi_isr() {
    ...
	temp_IER = fsl_lpspi->reg_ier;
	fsl_lpspi_intctrl(fsl_lpspi, 0);
	temp_SR = readl(fsl_lpspi->base + IMX7ULP_SR);

	fsl_lpspi_read_rx_fifo(fsl_lpspi);

	if ((temp_SR & SR_TDF) && (temp_IER & IER_TDIE)) {
		fsl_lpspi_write_tx_fifo(fsl_lpspi);
    ...
}

This can corrupt the stack or heap by overwriting the freed rx_buf pointer.

Should synchronize_irq() be called during the timeout or abort path, and
should reg_ier modifications be protected against concurrent ISR execution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-spi-fsl-lpspi-more-cleanups-v1-0-2d4549d5205e@pengutronix.de?part=3