[PATCH spi-next 0/3] spi: spi-fsl-lpspi: various cleanup and enhancement patches - part 2

[PATCH spi-next 0/3] spi: spi-fsl-lpspi: various cleanup and enhancement patches - part 2

[Marc Kleine-Budde]

While optimizing the spi-fsl-lpspi driver, I created some cleanup and
enhacement patches.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (3):
      spi: spi-fsl-lpspi: pass struct fsl_lpspi_data instead of struct spi_controller if possible
      spi: spi-fsl-lpspi: fsl_lpspi_transfer_one(): directly return ret
      spi: spi-fsl-lpspi: cache content of Interrupt Enable Register (IER)

 drivers/spi/spi-fsl-lpspi.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)
---
base-commit: 85b0e19de168d82cbd879a4c8e13fabad5ef4dd8
change-id: 20260916-spi-fsl-lpspi-more-cleanups-d2c934a372a1

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

[PATCH spi-next 1/3] spi: spi-fsl-lpspi: pass struct fsl_lpspi_data instead of struct spi_controller if possible

[Marc Kleine-Budde]

Instead of passing a spi_controller pointer to functions and using
spi_controller_get_devdata() to get the struct fsl_lpspi_data pointer,
directly pass a struct fsl_lpspi_data pointer.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 5aa0a75b4e57..a3c5bab1acc7 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -550,11 +550,8 @@ static int fsl_lpspi_target_abort(struct spi_controller *controller)
 	return 0;
 }
 
-static int fsl_lpspi_wait_for_completion(struct spi_controller *controller)
+static int fsl_lpspi_wait_for_completion(struct fsl_lpspi_data *fsl_lpspi)
 {
-	struct fsl_lpspi_data *fsl_lpspi =
-				spi_controller_get_devdata(controller);
-
 	if (fsl_lpspi->is_target) {
 		if (wait_for_completion_interruptible(&fsl_lpspi->xfer_done) ||
 			fsl_lpspi->target_aborted) {
@@ -758,11 +755,9 @@ static int fsl_lpspi_dma_init(struct device *dev,
 	return ret;
 }
 
-static int fsl_lpspi_pio_transfer(struct spi_controller *controller,
+static int fsl_lpspi_pio_transfer(struct fsl_lpspi_data *fsl_lpspi,
 				  struct spi_transfer *t)
 {
-	struct fsl_lpspi_data *fsl_lpspi =
-				spi_controller_get_devdata(controller);
 	int ret;
 
 	fsl_lpspi->tx_buf = t->tx_buf;
@@ -774,7 +769,7 @@ static int fsl_lpspi_pio_transfer(struct spi_controller *controller,
 
 	fsl_lpspi_write_tx_fifo(fsl_lpspi);
 
-	ret = fsl_lpspi_wait_for_completion(controller);
+	ret = fsl_lpspi_wait_for_completion(fsl_lpspi);
 
 	fsl_lpspi_reset(fsl_lpspi);
 
@@ -803,7 +798,7 @@ static int fsl_lpspi_transfer_one(struct spi_controller *controller,
 	if (fsl_lpspi->usedma)
 		ret = fsl_lpspi_dma_transfer(controller, fsl_lpspi, t);
 	else
-		ret = fsl_lpspi_pio_transfer(controller, t);
+		ret = fsl_lpspi_pio_transfer(fsl_lpspi, t);
 	if (ret < 0)
 		return ret;
 

-- 
2.53.0

[PATCH spi-next 2/3] spi: spi-fsl-lpspi: fsl_lpspi_transfer_one(): directly return ret

[Marc Kleine-Budde]

Both fsl_lpspi_dma_transfer() and fsl_lpspi_pio_transfer() only return
negative error values in case of error or 0 in case of success.

Directly return ret, there is no need to check for positive error values
and return them as 0.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index a3c5bab1acc7..b25f94b2ef73 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -799,10 +799,8 @@ static int fsl_lpspi_transfer_one(struct spi_controller *controller,
 		ret = fsl_lpspi_dma_transfer(controller, fsl_lpspi, t);
 	else
 		ret = fsl_lpspi_pio_transfer(fsl_lpspi, t);
-	if (ret < 0)
-		return ret;
 
-	return 0;
+	return ret;
 }
 
 static irqreturn_t fsl_lpspi_isr(int irq, void *dev_id)

-- 
2.53.0

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

[Marc Kleine-Budde]

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>
---
 drivers/spi/spi-fsl-lpspi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index b25f94b2ef73..5b158b0cb5af 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -122,6 +122,7 @@ struct fsl_lpspi_data {
 	void (*rx)(struct fsl_lpspi_data *fsl_lpspi);
 
 	u32 remain;
+	u32 reg_ier;
 	u8 watermark;
 	u8 txfifosize;
 	u8 rxfifosize;
@@ -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;
 	writel(enable, fsl_lpspi->base + IMX7ULP_IER);
 }
 
@@ -525,7 +527,7 @@ static int fsl_lpspi_prepare_message(struct spi_controller *controller,
 	fsl_lpspi_set_cmd(fsl_lpspi);
 
 	/* No IRQs */
-	writel(0, fsl_lpspi->base + IMX7ULP_IER);
+	fsl_lpspi_intctrl(fsl_lpspi, 0);
 
 	/* Controller disable, clear FIFOs, clear status */
 	writel(CR_RRF | CR_RTF, fsl_lpspi->base + IMX7ULP_CR);
@@ -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);
 

-- 
2.53.0

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

[sashiko-bot]

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