[PATCH] spi: davinci: Fix use-after-free on unbind

[PATCH] spi: davinci: Fix use-after-free on unbind

[Lukas Wunner]

davinci_spi_remove() accesses the driver's private data after it's been
freed with spi_master_put().

Fix by moving the spi_master_put() to the end of the function.

Fixes: fe5fd2540947 ("spi: davinci: Use dma_request_chan() for requesting DMA channel")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v4.7+
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/spi/spi-davinci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 818f2b22875d..7453a1dbbc06 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -1040,13 +1040,13 @@ static int davinci_spi_remove(struct platform_device *pdev)
 	spi_bitbang_stop(&dspi->bitbang);
 
 	clk_disable_unprepare(dspi->clk);
-	spi_master_put(master);
 
 	if (dspi->dma_rx) {
 		dma_release_channel(dspi->dma_rx);
 		dma_release_channel(dspi->dma_tx);
 	}
 
+	spi_master_put(master);
 	return 0;
 }
 
-- 
2.28.0

[PATCH] spi: st-ssc4: Fix unbalanced pm_runtime_disable() in probe error path

[Lukas Wunner]

If the calls to devm_platform_ioremap_resource(), irq_of_parse_and_map()
or devm_request_irq() fail on probe of the ST SSC4 SPI driver, the
runtime PM disable depth is incremented even though it was not
decremented before.  Fix it.

Fixes: cd050abeba2a ("spi: st-ssc4: add missed pm_runtime_disable")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v5.5+
Cc: Chuhong Yuan <hslester96@gmail.com>
---
 drivers/spi/spi-st-ssc4.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-st-ssc4.c b/drivers/spi/spi-st-ssc4.c
index 77d26d64541a..6c44dda9ee8c 100644
--- a/drivers/spi/spi-st-ssc4.c
+++ b/drivers/spi/spi-st-ssc4.c
@@ -375,13 +375,14 @@ static int spi_st_probe(struct platform_device *pdev)
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register master\n");
-		goto clk_disable;
+		goto rpm_disable;
 	}
 
 	return 0;
 
-clk_disable:
+rpm_disable:
 	pm_runtime_disable(&pdev->dev);
+clk_disable:
 	clk_disable_unprepare(spi_st->clk);
 put_master:
 	spi_master_put(master);
-- 
2.28.0

Re: [PATCH] spi: st-ssc4: Fix unbalanced pm_runtime_disable() in probe error path

[Mark Brown]

On Sun, 8 Nov 2020 23:41:00 +0100, Lukas Wunner wrote:
> If the calls to devm_platform_ioremap_resource(), irq_of_parse_and_map()
> or devm_request_irq() fail on probe of the ST SSC4 SPI driver, the
> runtime PM disable depth is incremented even though it was not
> decremented before.  Fix it.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: st-ssc4: Fix unbalanced pm_runtime_disable() in probe error path
      commit: 5ef76dac0f2c26aeae4ee79eb830280f16d5aceb

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

[PATCH] spi: synquacer: Disable clock in probe error path

[Lukas Wunner]

If the calls to platform_get_irq() or devm_request_irq() fail on probe
of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not
unprepared and disabled.

If the clock rate "master->max_speed_hz" cannot be determined, the same
happens and in addition the spi_master struct is not freed.

Fix it.

Fixes: b0823ee35cf9 ("spi: Add spi driver for Socionext SynQuacer platform")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v5.3+
Cc: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 drivers/spi/spi-synquacer.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-synquacer.c b/drivers/spi/spi-synquacer.c
index 42e82dbe3d41..8cdca6ab8098 100644
--- a/drivers/spi/spi-synquacer.c
+++ b/drivers/spi/spi-synquacer.c
@@ -657,7 +657,8 @@ static int synquacer_spi_probe(struct platform_device *pdev)
 
 	if (!master->max_speed_hz) {
 		dev_err(&pdev->dev, "missing clock source\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto disable_clk;
 	}
 	master->min_speed_hz = master->max_speed_hz / 254;
 
@@ -670,7 +671,7 @@ static int synquacer_spi_probe(struct platform_device *pdev)
 	rx_irq = platform_get_irq(pdev, 0);
 	if (rx_irq <= 0) {
 		ret = rx_irq;
-		goto put_spi;
+		goto disable_clk;
 	}
 	snprintf(sspi->rx_irq_name, SYNQUACER_HSSPI_IRQ_NAME_MAX, "%s-rx",
 		 dev_name(&pdev->dev));
@@ -678,13 +679,13 @@ static int synquacer_spi_probe(struct platform_device *pdev)
 				0, sspi->rx_irq_name, sspi);
 	if (ret) {
 		dev_err(&pdev->dev, "request rx_irq failed (%d)\n", ret);
-		goto put_spi;
+		goto disable_clk;
 	}
 
 	tx_irq = platform_get_irq(pdev, 1);
 	if (tx_irq <= 0) {
 		ret = tx_irq;
-		goto put_spi;
+		goto disable_clk;
 	}
 	snprintf(sspi->tx_irq_name, SYNQUACER_HSSPI_IRQ_NAME_MAX, "%s-tx",
 		 dev_name(&pdev->dev));
@@ -692,7 +693,7 @@ static int synquacer_spi_probe(struct platform_device *pdev)
 				0, sspi->tx_irq_name, sspi);
 	if (ret) {
 		dev_err(&pdev->dev, "request tx_irq failed (%d)\n", ret);
-		goto put_spi;
+		goto disable_clk;
 	}
 
 	master->dev.of_node = np;
@@ -710,7 +711,7 @@ static int synquacer_spi_probe(struct platform_device *pdev)
 
 	ret = synquacer_spi_enable(master);
 	if (ret)
-		goto fail_enable;
+		goto disable_clk;
 
 	pm_runtime_set_active(sspi->dev);
 	pm_runtime_enable(sspi->dev);
@@ -723,7 +724,7 @@ static int synquacer_spi_probe(struct platform_device *pdev)
 
 disable_pm:
 	pm_runtime_disable(sspi->dev);
-fail_enable:
+disable_clk:
 	clk_disable_unprepare(sspi->clk);
 put_spi:
 	spi_master_put(master);
-- 
2.28.0

Re: [PATCH] spi: synquacer: Disable clock in probe error path

[Andy Shevchenko]

On Mon, Nov 9, 2020 at 12:52 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> If the calls to platform_get_irq() or devm_request_irq() fail on probe
> of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not
> unprepared and disabled.
>
> If the clock rate "master->max_speed_hz" cannot be determined, the same
> happens and in addition the spi_master struct is not freed.

Wouldn't be better to switch over devm_add_action_or_reset() in such cases?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] spi: synquacer: Disable clock in probe error path

[Lukas Wunner]

On Mon, Nov 09, 2020 at 04:22:38PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 9, 2020 at 12:52 AM Lukas Wunner <lukas@wunner.de> wrote:
> > If the calls to platform_get_irq() or devm_request_irq() fail on probe
> > of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not
> > unprepared and disabled.
> >
> > If the clock rate "master->max_speed_hz" cannot be determined, the same
> > happens and in addition the spi_master struct is not freed.
> 
> Wouldn't be better to switch over devm_add_action_or_reset() in such cases?

I'd rather prefer a devm_clk_prepare_enable().  This is common enough
to merit a function of its own.

As for the spi_master struct being leaked:  I'm about to submit
a series which introduces devm_spi_alloc_master/slave() and uses
that to fix a use-after-free in 9 drivers and an spi_master leak
in 8 drivers.  Patches are on this development branch:

https://github.com/l1k/linux/commits/spi_fixes

I'm too busy with this series to also look into adding a
devm_clk_prepare_enable().  Hopefully someone else can do that.

Thanks,

Lukas

Re: [PATCH] spi: synquacer: Disable clock in probe error path

[Mark Brown]

On Sun, 8 Nov 2020 23:41:00 +0100, Lukas Wunner wrote:
> If the calls to platform_get_irq() or devm_request_irq() fail on probe
> of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not
> unprepared and disabled.
> 
> If the clock rate "master->max_speed_hz" cannot be determined, the same
> happens and in addition the spi_master struct is not freed.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: synquacer: Disable clock in probe error path
      commit: 8853b2503014aca5c793d2c7f0aabc990b32bdad

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

[PATCH] spi: lpspi: Fix use-after-free on unbind

[Lukas Wunner]

Normally the last reference on an spi_controller is released by
spi_unregister_controller().  In the case of the i.MX lpspi driver,
the spi_controller is registered with devm_spi_register_controller(),
so spi_unregister_controller() is invoked automatically after the driver
has unbound.

However the driver already releases the last reference in
fsl_lpspi_remove() through a gratuitous call to spi_master_put(),
causing a use-after-free when spi_unregister_controller() is
subsequently invoked by the devres framework.

Fix by dropping the superfluous spi_master_put().

Fixes: 944c01a889d9 ("spi: lpspi: enable runtime pm for lpspi")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v5.2+
Cc: Han Xu <han.xu@nxp.com>
---
 drivers/spi/spi-fsl-lpspi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 986b9793fd3c..a2886ee44e4c 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -938,9 +938,6 @@ static int fsl_lpspi_remove(struct platform_device *pdev)
 				spi_controller_get_devdata(controller);
 
 	pm_runtime_disable(fsl_lpspi->dev);
-
-	spi_master_put(controller);
-
 	return 0;
 }
 
-- 
2.28.0

Re: [PATCH] spi: lpspi: Fix use-after-free on unbind

[Mark Brown]

On Sun, 8 Nov 2020 23:41:00 +0100, Lukas Wunner wrote:
> Normally the last reference on an spi_controller is released by
> spi_unregister_controller().  In the case of the i.MX lpspi driver,
> the spi_controller is registered with devm_spi_register_controller(),
> so spi_unregister_controller() is invoked automatically after the driver
> has unbound.
> 
> However the driver already releases the last reference in
> fsl_lpspi_remove() through a gratuitous call to spi_master_put(),
> causing a use-after-free when spi_unregister_controller() is
> subsequently invoked by the devres framework.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: lpspi: Fix use-after-free on unbind
      commit: 4def49da620c84a682d9361d6bef0a97eed46fe0

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

[PATCH] spi: atmel-quadspi: Disable clock in probe error path

[Lukas Wunner]

If the call to of_device_get_match_data() fails on probe of the Atmel
QuadSPI driver, the clock "aq->pclk" is erroneously not unprepared and
disabled.  Fix it.

Fixes: 2e5c88887358 ("spi: atmel-quadspi: add support for sam9x60 qspi controller")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v5.1+
Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/spi/atmel-quadspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 8c009c175f2c..b44521d4a245 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -594,7 +594,7 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	if (!aq->caps) {
 		dev_err(&pdev->dev, "Could not retrieve QSPI caps\n");
 		err = -EINVAL;
-		goto exit;
+		goto disable_pclk;
 	}
 
 	if (aq->caps->has_qspick) {
-- 
2.28.0

Re: [PATCH] spi: atmel-quadspi: Disable clock in probe error path

[Tudor.Ambarus]

On 11/9/20 12:41 AM, Lukas Wunner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> If the call to of_device_get_match_data() fails on probe of the Atmel
> QuadSPI driver, the clock "aq->pclk" is erroneously not unprepared and
> disabled.  Fix it.
> 
> Fixes: 2e5c88887358 ("spi: atmel-quadspi: add support for sam9x60 qspi controller")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: <stable@vger.kernel.org> # v5.1+
> Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Thanks!

> ---
>  drivers/spi/atmel-quadspi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index 8c009c175f2c..b44521d4a245 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -594,7 +594,7 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>         if (!aq->caps) {
>                 dev_err(&pdev->dev, "Could not retrieve QSPI caps\n");
>                 err = -EINVAL;
> -               goto exit;
> +               goto disable_pclk;
>         }
> 
>         if (aq->caps->has_qspick) {
> --
> 2.28.0
> 

Re: [PATCH] spi: atmel-quadspi: Disable clock in probe error path

[Mark Brown]

On Sun, 8 Nov 2020 23:41:00 +0100, Lukas Wunner wrote:
> If the call to of_device_get_match_data() fails on probe of the Atmel
> QuadSPI driver, the clock "aq->pclk" is erroneously not unprepared and
> disabled.  Fix it.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: atmel-quadspi: Disable clock in probe error path
      commit: 0e685017c7ba1a2fe9f6f1e7a9302890747d934c

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

[PATCH] spi: pic32: Don't leak DMA channels in probe error path

[Lukas Wunner]

If the calls to devm_request_irq() or devm_spi_register_master() fail
on probe of the PIC32 SPI driver, the DMA channels requested by
pic32_spi_dma_prep() are erroneously not released.  Plug the leak.

Fixes: 1bcb9f8ceb67 ("spi: spi-pic32: Add PIC32 SPI master driver")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: <stable@vger.kernel.org> # v4.7+
Cc: Purna Chandra Mandal <purna.mandal@microchip.com>
---
 drivers/spi/spi-pic32.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c
index 156961b4ca86..104bde153efd 100644
--- a/drivers/spi/spi-pic32.c
+++ b/drivers/spi/spi-pic32.c
@@ -839,6 +839,7 @@ static int pic32_spi_probe(struct platform_device *pdev)
 	return 0;
 
 err_bailout:
+	pic32_spi_dma_unprep(pic32s);
 	clk_disable_unprepare(pic32s->clk);
 err_master:
 	spi_master_put(master);
-- 
2.28.0

Re: [PATCH] spi: pic32: Don't leak DMA channels in probe error path

[Mark Brown]

On Sun, 8 Nov 2020 23:41:00 +0100, Lukas Wunner wrote:
> If the calls to devm_request_irq() or devm_spi_register_master() fail
> on probe of the PIC32 SPI driver, the DMA channels requested by
> pic32_spi_dma_prep() are erroneously not released.  Plug the leak.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: pic32: Don't leak DMA channels in probe error path
      commit: c575e9113bff5e024d75481613faed5ef9d465b2

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] spi: davinci: Fix use-after-free on unbind

[Peter Ujfalusi]

Hi Lukas,

On 09/11/2020 0.41, Lukas Wunner wrote:
> davinci_spi_remove() accesses the driver's private data after it's been
> freed with spi_master_put().
> 
> Fix by moving the spi_master_put() to the end of the function.

Thanks for spotting it,

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> Fixes: fe5fd2540947 ("spi: davinci: Use dma_request_chan() for requesting DMA channel")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: <stable@vger.kernel.org> # v4.7+
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/spi/spi-davinci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index 818f2b22875d..7453a1dbbc06 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -1040,13 +1040,13 @@ static int davinci_spi_remove(struct platform_device *pdev)
>  	spi_bitbang_stop(&dspi->bitbang);
>  
>  	clk_disable_unprepare(dspi->clk);
> -	spi_master_put(master);
>  
>  	if (dspi->dma_rx) {
>  		dma_release_channel(dspi->dma_rx);
>  		dma_release_channel(dspi->dma_tx);
>  	}
>  
> +	spi_master_put(master);
>  	return 0;
>  }
>  
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki