Possible race in spi-tegra114.ko

Possible race in spi-tegra114.ko

[Anton Volkov]

Hello.
While searching for races in Linux kernel I've come across 
drivers/spi/spi-tegra114.ko module. Here is the question that I came up 
with while analysing results. Lines are given using the info from Linux 
v4.12.

Consider the following case:
Thread 1:                                  Thread 2:
tegra_spi_probe
   -> request_threaded_irq
       (spi-tegra114.c: line 1070)
   ------------interrupt comes------------- tegra_spi_isr_thread
                                              -> handle_dma_based_xfer
                                                   -> 
tegra_spi_start_dma_based_transfer
                                                        -> 
tegra_spi_copy_client_txbuf_to_spi_txbuf
                                                             -> 
dma_sync_single_for_cpu(tspi->dev, tspi->tx_dma_phys, ...) 
(spi-tegra114.c: line 368)
   -> tspi->tx_dma_phys = ...
      (spi-tegra114.c: line 621 or 625)

If the situation above is feasible then the value of tspi->tx_dma_phys 
(which is 0) is passed to dma_sync_single_for_cpu and further down the 
callstack. This is probably not good.
Note that other fields of tspi structure can also be affected using the 
similar templates.
Similar cases can occur also for tspi structure in 
drivers/spi/spi-tegra20-slink.ko module and for tsd structure in 
drivers/spi/spi-tegra20-sflash.ko module.
So the question is: is there a possibility of interrupt triggering 
before the registration of master (spi-tegra114.c: line 1125) or a write 
to tspi->def_command1_reg (spi-tegra114.c: line 1121)?

Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avolkov-ufN2psIa012HXe+LvDLADg@public.gmane.org <mailto:avolkov-ufN2psIa012HXe+LvDLADg@public.gmane.org>

Re: Possible race in spi-tegra114.ko

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]

On Mon, Jul 24, 2017 at 03:30:36PM +0300, Anton Volkov wrote:
> Hello.
> While searching for races in Linux kernel I've come across
> drivers/spi/spi-tegra114.ko module. Here is the question that I came up with
> while analysing results. Lines are given using the info from Linux v4.12.
> 
> Consider the following case:
> Thread 1:                                  Thread 2:
> tegra_spi_probe
>   -> request_threaded_irq
>       (spi-tegra114.c: line 1070)
>   ------------interrupt comes------------- tegra_spi_isr_thread
>                                              -> handle_dma_based_xfer
>                                                   ->
> tegra_spi_start_dma_based_transfer
>                                                        ->
> tegra_spi_copy_client_txbuf_to_spi_txbuf
>                                                             ->
> dma_sync_single_for_cpu(tspi->dev, tspi->tx_dma_phys, ...) (spi-tegra114.c:
> line 368)
>   -> tspi->tx_dma_phys = ...
>      (spi-tegra114.c: line 621 or 625)
> 
> If the situation above is feasible then the value of tspi->tx_dma_phys
> (which is 0) is passed to dma_sync_single_for_cpu and further down the
> callstack. This is probably not good.
> Note that other fields of tspi structure can also be affected using the
> similar templates.
> Similar cases can occur also for tspi structure in
> drivers/spi/spi-tegra20-slink.ko module and for tsd structure in
> drivers/spi/spi-tegra20-sflash.ko module.
> So the question is: is there a possibility of interrupt triggering before
> the registration of master (spi-tegra114.c: line 1125) or a write to
> tspi->def_command1_reg (spi-tegra114.c: line 1121)?
> 
> Thank you for your time.

I suspect that it would be possible for an interrupt to be raised if the
bootloader had left it enabled and it was pending when the bootloader
passed control to the kernel. I'm not aware of any reports of this ever
happening, but that might just mean we've only dealt with reasonably
sane bootloaders so far.

I think the easiest would probably be to move the request_threaded_irq()
call to after everything required by the interrupt handler has been
initialized.

So the correct place would probably be right before the call to
pm_runtime_enable().

Laxman, any comments?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] tegra114: fix to a race condition due to early registration of interrupt handler

[Anton Volkov]

The early registration of interrupt handler made possible a race
condition leading to the usage of an incorrect physical address
obtained by reading uninitialized tspi->tx_dma_phys.

This patch moves the registration of an interrupt handler further
down the code of tegra_spi_probe to make the race infeasible.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avolkov@ispras.ru>
---
 drivers/spi/spi-tegra114.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c
index 08012ae..9a88dca 100644
--- a/drivers/spi/spi-tegra114.c
+++ b/drivers/spi/spi-tegra114.c
@@ -1065,29 +1065,18 @@ static int tegra_spi_probe(struct platform_device *pdev)
 	}
 	tspi->phys = r->start;
 
-	spi_irq = platform_get_irq(pdev, 0);
-	tspi->irq = spi_irq;
-	ret = request_threaded_irq(tspi->irq, tegra_spi_isr,
-			tegra_spi_isr_thread, IRQF_ONESHOT,
-			dev_name(&pdev->dev), tspi);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
-					tspi->irq);
-		goto exit_free_master;
-	}
-
 	tspi->clk = devm_clk_get(&pdev->dev, "spi");
 	if (IS_ERR(tspi->clk)) {
 		dev_err(&pdev->dev, "can not get clock\n");
 		ret = PTR_ERR(tspi->clk);
-		goto exit_free_irq;
+		goto exit_free_master;
 	}
 
 	tspi->rst = devm_reset_control_get(&pdev->dev, "spi");
 	if (IS_ERR(tspi->rst)) {
 		dev_err(&pdev->dev, "can not get reset\n");
 		ret = PTR_ERR(tspi->rst);
-		goto exit_free_irq;
+		goto exit_free_master;
 	}
 
 	tspi->max_buf_size = SPI_FIFO_DEPTH << 2;
@@ -1095,7 +1084,7 @@ static int tegra_spi_probe(struct platform_device *pdev)
 
 	ret = tegra_spi_init_dma_param(tspi, true);
 	if (ret < 0)
-		goto exit_free_irq;
+		goto exit_free_master;
 	ret = tegra_spi_init_dma_param(tspi, false);
 	if (ret < 0)
 		goto exit_rx_dma_free;
@@ -1105,6 +1094,17 @@ static int tegra_spi_probe(struct platform_device *pdev)
 
 	init_completion(&tspi->xfer_completion);
 
+	spi_irq = platform_get_irq(pdev, 0);
+	tspi->irq = spi_irq;
+	ret = request_threaded_irq(tspi->irq, tegra_spi_isr,
+			tegra_spi_isr_thread, IRQF_ONESHOT,
+			dev_name(&pdev->dev), tspi);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
+					tspi->irq);
+		goto exit_rx_dma_free;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = tegra_spi_runtime_resume(&pdev->dev);
@@ -1133,11 +1133,10 @@ static int tegra_spi_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_spi_runtime_suspend(&pdev->dev);
+	free_irq(spi_irq, tspi);
 	tegra_spi_deinit_dma_param(tspi, false);
 exit_rx_dma_free:
 	tegra_spi_deinit_dma_param(tspi, true);
-exit_free_irq:
-	free_irq(spi_irq, tspi);
 exit_free_master:
 	spi_master_put(master);
 	return ret;
-- 
2.7.4

[PATCH] tegra114: fix to a race condition due to early registration of interrupt handler

[Anton Volkov]

The early registration of interrupt handler made possible a race
condition leading to the usage of an incorrect physical address
obtained by reading uninitialized tspi->tx_dma_phys.

This patch moves the registration of an interrupt handler further
down the code of tegra_spi_probe to make the race infeasible.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avolkov@ispras.ru>
---
 drivers/spi/spi-tegra114.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c
index 08012ae..9a88dca 100644
--- a/drivers/spi/spi-tegra114.c
+++ b/drivers/spi/spi-tegra114.c
@@ -1065,29 +1065,18 @@ static int tegra_spi_probe(struct platform_device *pdev)
 	}
 	tspi->phys = r->start;
 
-	spi_irq = platform_get_irq(pdev, 0);
-	tspi->irq = spi_irq;
-	ret = request_threaded_irq(tspi->irq, tegra_spi_isr,
-			tegra_spi_isr_thread, IRQF_ONESHOT,
-			dev_name(&pdev->dev), tspi);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
-					tspi->irq);
-		goto exit_free_master;
-	}
-
 	tspi->clk = devm_clk_get(&pdev->dev, "spi");
 	if (IS_ERR(tspi->clk)) {
 		dev_err(&pdev->dev, "can not get clock\n");
 		ret = PTR_ERR(tspi->clk);
-		goto exit_free_irq;
+		goto exit_free_master;
 	}
 
 	tspi->rst = devm_reset_control_get(&pdev->dev, "spi");
 	if (IS_ERR(tspi->rst)) {
 		dev_err(&pdev->dev, "can not get reset\n");
 		ret = PTR_ERR(tspi->rst);
-		goto exit_free_irq;
+		goto exit_free_master;
 	}
 
 	tspi->max_buf_size = SPI_FIFO_DEPTH << 2;
@@ -1095,7 +1084,7 @@ static int tegra_spi_probe(struct platform_device *pdev)
 
 	ret = tegra_spi_init_dma_param(tspi, true);
 	if (ret < 0)
-		goto exit_free_irq;
+		goto exit_free_master;
 	ret = tegra_spi_init_dma_param(tspi, false);
 	if (ret < 0)
 		goto exit_rx_dma_free;
@@ -1105,6 +1094,17 @@ static int tegra_spi_probe(struct platform_device *pdev)
 
 	init_completion(&tspi->xfer_completion);
 
+	spi_irq = platform_get_irq(pdev, 0);
+	tspi->irq = spi_irq;
+	ret = request_threaded_irq(tspi->irq, tegra_spi_isr,
+			tegra_spi_isr_thread, IRQF_ONESHOT,
+			dev_name(&pdev->dev), tspi);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
+					tspi->irq);
+		goto exit_rx_dma_free;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = tegra_spi_runtime_resume(&pdev->dev);
@@ -1133,11 +1133,10 @@ static int tegra_spi_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_spi_runtime_suspend(&pdev->dev);
+	free_irq(spi_irq, tspi);
 	tegra_spi_deinit_dma_param(tspi, false);
 exit_rx_dma_free:
 	tegra_spi_deinit_dma_param(tspi, true);
-exit_free_irq:
-	free_irq(spi_irq, tspi);
 exit_free_master:
 	spi_master_put(master);
 	return ret;
-- 
2.7.4