FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 4.9-stable tree

PATCH [email protected] 3 participants 11 messages Nov 23, 2020 – Dec 5, 2020

Subsystems (3)

Stable branch SPI Broadcom SPI driver

lore

Share & tools

lore

[email protected]

author maintainer

2020-11-23 08:51 UTC #0 lore raw

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 63c5395bb7a9777a33f0e7b5906f2c0170a23692 Mon Sep 17 00:00:00 2001
From: Lukas Wunner <[email protected]>
Date: Wed, 11 Nov 2020 20:07:40 +0100
Subject: [PATCH] spi: bcm-qspi: Fix use-after-free on unbind

bcm_qspi_remove() calls spi_unregister_master() even though bcm_qspi_probe() calls devm_spi_register_master(). The spi_master is therefore unregistered and freed twice on unbind.

Moreover, since commit 0392727c261b ("spi: bcm-qspi: Handle clock probe deferral"), bcm_qspi_probe() leaks the spi_master allocation if the call to devm_clk_get_optional() fails.

Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound and also avoids the spi_master leak on probe.

While at it, fix an ordering issue in bcm_qspi_remove() wherein spi_unregister_master() is called after uninitializing the hardware, disabling the clock and freeing an IRQ data structure. The correct order is to call spi_unregister_master() *before* those teardown steps because bus accesses may still be ongoing until that function returns.

Fixes fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")

Signed-off-by Lukas Wunner <[email protected]>

Cc <[email protected]> # v4.9+: 123456789abc: spi: Introduce device-managed SPI controller allocation

Cc <[email protected]> # v4.9+

Cc Kamal Dasu <[email protected]>

Acked-by Florian Fainelli <[email protected]>

Tested-by Florian Fainelli <[email protected]>

Link https://lore.kernel.org/r/5e31a9a59fd1c0d0b795b2fe219f25e5ee855f9d.1605121038.git.lukas@wunner.de open here

Signed-off-by Mark Brown <[email protected]>

drivers/spi/spi-bcm-qspi.c +12 ~~-22~~

index 14c9d0133bce..c028446c7460 100644
@@ -1327,7 +1327,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 
 	data = of_id->data;
 
-	master = spi_alloc_master(dev, sizeof(struct bcm_qspi));
+	master = devm_spi_alloc_master(dev, sizeof(struct bcm_qspi));
 	if (!master) {
 		dev_err(dev, "error allocating spi_master\n");
 		return -ENOMEM;

@@ -1367,21 +1367,17 @@ int bcm_qspi_probe(struct platform_device *pdev,
 
 	if (res) {
 		qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[MSPI])) {
-			ret = PTR_ERR(qspi->base[MSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[MSPI]))
+			return PTR_ERR(qspi->base[MSPI]);
 	} else {
-		goto qspi_resource_err;
+		return 0;
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi");
 	if (res) {
 		qspi->base[BSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[BSPI])) {
-			ret = PTR_ERR(qspi->base[BSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[BSPI]))
+			return PTR_ERR(qspi->base[BSPI]);
 		qspi->bspi_mode = true;
 	} else {
 		qspi->bspi_mode = false;

@@ -1392,18 +1388,14 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg");
 	if (res) {
 		qspi->base[CHIP_SELECT]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[CHIP_SELECT])) {
-			ret = PTR_ERR(qspi->base[CHIP_SELECT]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[CHIP_SELECT]))
+			return PTR_ERR(qspi->base[CHIP_SELECT]);
 	}
 
 	qspi->dev_ids = kcalloc(num_irqs, sizeof(struct bcm_qspi_dev_id),
 				GFP_KERNEL);
-	if (!qspi->dev_ids) {
-		ret = -ENOMEM;
-		goto qspi_resource_err;
-	}
+	if (!qspi->dev_ids)
+		return -ENOMEM;
 
 	for (val = 0; val < num_irqs; val++) {
 		irq = -1;

@@ -1484,7 +1476,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	qspi->xfer_mode.addrlen = -1;
 	qspi->xfer_mode.hp = -1;
 
-	ret = devm_spi_register_master(&pdev->dev, master);
+	ret = spi_register_master(master);
 	if (ret < 0) {
 		dev_err(dev, "can't register master\n");
 		goto qspi_reg_err;

@@ -1497,8 +1489,6 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	clk_disable_unprepare(qspi->clk);
 qspi_probe_err:
 	kfree(qspi->dev_ids);
-qspi_resource_err:
-	spi_master_put(master);
 	return ret;
 }
 /* probe function to be called by SoC specific platform driver probe */

@@ -1508,10 +1498,10 @@ int bcm_qspi_remove(struct platform_device *pdev)
 {
 	struct bcm_qspi *qspi = platform_get_drvdata(pdev);
 
+	spi_unregister_master(qspi->master);
 	bcm_qspi_hw_uninit(qspi);
 	clk_disable_unprepare(qspi->clk);
 	kfree(qspi->dev_ids);
-	spi_unregister_master(qspi->master);
 
 	return 0;
 }

[email protected]

author maintainer

2020-11-23 09:15 UTC #1 lore raw

FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

bcm_qspi_remove() calls spi_unregister_master() even though bcm_qspi_probe() calls devm_spi_register_master(). The spi_master is therefore unregistered and freed twice on unbind.

Moreover, since commit 0392727c261b ("spi: bcm-qspi: Handle clock probe deferral"), bcm_qspi_probe() leaks the spi_master allocation if the call to devm_clk_get_optional() fails.

Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound and also avoids the spi_master leak on probe.

Fixes fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")

Signed-off-by Lukas Wunner <[email protected]>

Cc <[email protected]> # v4.9+: 123456789abc: spi: Introduce device-managed SPI controller allocation

Cc <[email protected]> # v4.9+

Cc Kamal Dasu <[email protected]>

Acked-by Florian Fainelli <[email protected]>

Tested-by Florian Fainelli <[email protected]>

Link https://lore.kernel.org/r/5e31a9a59fd1c0d0b795b2fe219f25e5ee855f9d.1605121038.git.lukas@wunner.de open here

Signed-off-by Mark Brown <[email protected]>

drivers/spi/spi-bcm-qspi.c +12 ~~-22~~

index 14c9d0133bce..c028446c7460 100644
@@ -1327,7 +1327,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 
 	data = of_id->data;
 
-	master = spi_alloc_master(dev, sizeof(struct bcm_qspi));
+	master = devm_spi_alloc_master(dev, sizeof(struct bcm_qspi));
 	if (!master) {
 		dev_err(dev, "error allocating spi_master\n");
 		return -ENOMEM;

@@ -1367,21 +1367,17 @@ int bcm_qspi_probe(struct platform_device *pdev,
 
 	if (res) {
 		qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[MSPI])) {
-			ret = PTR_ERR(qspi->base[MSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[MSPI]))
+			return PTR_ERR(qspi->base[MSPI]);
 	} else {
-		goto qspi_resource_err;
+		return 0;
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi");
 	if (res) {
 		qspi->base[BSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[BSPI])) {
-			ret = PTR_ERR(qspi->base[BSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[BSPI]))
+			return PTR_ERR(qspi->base[BSPI]);
 		qspi->bspi_mode = true;
 	} else {
 		qspi->bspi_mode = false;

@@ -1392,18 +1388,14 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg");
 	if (res) {
 		qspi->base[CHIP_SELECT]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[CHIP_SELECT])) {
-			ret = PTR_ERR(qspi->base[CHIP_SELECT]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[CHIP_SELECT]))
+			return PTR_ERR(qspi->base[CHIP_SELECT]);
 	}
 
 	qspi->dev_ids = kcalloc(num_irqs, sizeof(struct bcm_qspi_dev_id),
 				GFP_KERNEL);
-	if (!qspi->dev_ids) {
-		ret = -ENOMEM;
-		goto qspi_resource_err;
-	}
+	if (!qspi->dev_ids)
+		return -ENOMEM;
 
 	for (val = 0; val < num_irqs; val++) {
 		irq = -1;

@@ -1484,7 +1476,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	qspi->xfer_mode.addrlen = -1;
 	qspi->xfer_mode.hp = -1;
 
-	ret = devm_spi_register_master(&pdev->dev, master);
+	ret = spi_register_master(master);
 	if (ret < 0) {
 		dev_err(dev, "can't register master\n");
 		goto qspi_reg_err;

@@ -1497,8 +1489,6 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	clk_disable_unprepare(qspi->clk);
 qspi_probe_err:
 	kfree(qspi->dev_ids);
-qspi_resource_err:
-	spi_master_put(master);
 	return ret;
 }
 /* probe function to be called by SoC specific platform driver probe */

@@ -1508,10 +1498,10 @@ int bcm_qspi_remove(struct platform_device *pdev)
 {
 	struct bcm_qspi *qspi = platform_get_drvdata(pdev);
 
+	spi_unregister_master(qspi->master);
 	bcm_qspi_hw_uninit(qspi);
 	clk_disable_unprepare(qspi->clk);
 	kfree(qspi->dev_ids);
-	spi_unregister_master(qspi->master);
 
 	return 0;
 }

Sudip Mukherjee

Acked-by Florian Fainelli Tested-by Florian Fainelli

2020-11-24 13:41 UTC #2 lore raw

Re: FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

Hi Greg,

Quoted 5 lines · On Mon, Nov 23, 2020 at 10:16:47AM +0100, [email protected] wrote:

The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Here is the backport for all the stable tree from v4.9.y to v5.4.y.

--
Regards
Sudip

From 230cfe3365d13ae6144c7fd0321a9f10fa9fc7f4 Mon Sep 17 00:00:00 2001
From: Lukas Wunner <[email protected]>
Date: Wed, 11 Nov 2020 20:07:40 +0100
Subject: [PATCH] spi: bcm-qspi: Fix use-after-free on unbind

commit 63c5395bb7a9777a33f0e7b5906f2c0170a23692 upstream

bcm_qspi_remove() calls spi_unregister_master() even though bcm_qspi_probe() calls devm_spi_register_master(). The spi_master is therefore unregistered and freed twice on unbind.

Moreover, since commit 0392727c261b ("spi: bcm-qspi: Handle clock probe deferral"), bcm_qspi_probe() leaks the spi_master allocation if the call to devm_clk_get_optional() fails.

Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound and also avoids the spi_master leak on probe.

Fixes fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")

Signed-off-by Lukas Wunner <[email protected]>

Cc <[email protected]> # v4.9+: 123456789abc: spi: Introduce device-managed SPI controller allocation

Cc <[email protected]> # v4.9+

Cc Kamal Dasu <[email protected]>

Acked-by Florian Fainelli <[email protected]>

Tested-by Florian Fainelli <[email protected]>

Link https://lore.kernel.org/r/5e31a9a59fd1c0d0b795b2fe219f25e5ee855f9d.1605121038.git.lukas@wunner.de open here

Signed-off-by Mark Brown <[email protected]>

[sudip: adjust context]

Signed-off-by Sudip Mukherjee <[email protected]>

---
 drivers/spi/spi-bcm-qspi.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

drivers/spi/spi-bcm-qspi.c +12 ~~-22~~

index d0afe0b1599f..8a4be34bccfd 100644
@@ -1213,7 +1213,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	if (!of_match_node(bcm_qspi_of_match, dev->of_node))
 		return -ENODEV;
 
-	master = spi_alloc_master(dev, sizeof(struct bcm_qspi));
+	master = devm_spi_alloc_master(dev, sizeof(struct bcm_qspi));
 	if (!master) {
 		dev_err(dev, "error allocating spi_master\n");
 		return -ENOMEM;

@@ -1252,21 +1252,17 @@ int bcm_qspi_probe(struct platform_device *pdev,
 
 	if (res) {
 		qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[MSPI])) {
-			ret = PTR_ERR(qspi->base[MSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[MSPI]))
+			return PTR_ERR(qspi->base[MSPI]);
 	} else {
-		goto qspi_resource_err;
+		return 0;
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi");
 	if (res) {
 		qspi->base[BSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[BSPI])) {
-			ret = PTR_ERR(qspi->base[BSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[BSPI]))
+			return PTR_ERR(qspi->base[BSPI]);
 		qspi->bspi_mode = true;
 	} else {
 		qspi->bspi_mode = false;

@@ -1277,18 +1273,14 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg");
 	if (res) {
 		qspi->base[CHIP_SELECT]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[CHIP_SELECT])) {
-			ret = PTR_ERR(qspi->base[CHIP_SELECT]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[CHIP_SELECT]))
+			return PTR_ERR(qspi->base[CHIP_SELECT]);
 	}
 
 	qspi->dev_ids = kcalloc(num_irqs, sizeof(struct bcm_qspi_dev_id),
 				GFP_KERNEL);
-	if (!qspi->dev_ids) {
-		ret = -ENOMEM;
-		goto qspi_resource_err;
-	}
+	if (!qspi->dev_ids)
+		return -ENOMEM;
 
 	for (val = 0; val < num_irqs; val++) {
 		irq = -1;

@@ -1357,7 +1349,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	qspi->xfer_mode.addrlen = -1;
 	qspi->xfer_mode.hp = -1;
 
-	ret = devm_spi_register_master(&pdev->dev, master);
+	ret = spi_register_master(master);
 	if (ret < 0) {
 		dev_err(dev, "can't register master\n");
 		goto qspi_reg_err;

@@ -1370,8 +1362,6 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	clk_disable_unprepare(qspi->clk);
 qspi_probe_err:
 	kfree(qspi->dev_ids);
-qspi_resource_err:
-	spi_master_put(master);
 	return ret;
 }
 /* probe function to be called by SoC specific platform driver probe */

@@ -1381,10 +1371,10 @@ int bcm_qspi_remove(struct platform_device *pdev)
 {
 	struct bcm_qspi *qspi = platform_get_drvdata(pdev);
 
+	spi_unregister_master(qspi->master);
 	bcm_qspi_hw_uninit(qspi);
 	clk_disable_unprepare(qspi->clk);
 	kfree(qspi->dev_ids);
-	spi_unregister_master(qspi->master);
 
 	return 0;
 }

Signature · 1 line

2.11.0

Greg KH

author maintainer

2020-11-24 18:06 UTC #3 lore raw

Re: FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

Quoted 10 lines · Sudip Mukherjee

Hi Greg,
Quoted 5 lines · On Mon, Nov 23, 2020 at 10:16:47AM +0100, [email protected] wrote:
The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Here is the backport for all the stable tree from v4.9.y to v5.4.y.

THis didn't apply to 4.9.y, are you sure you tried that?

Anyway, queued up for all other branches, thanks!

greg k-h

Sudip Mukherjee

2020-11-24 18:54 UTC #4 lore raw

Re: FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

Hi Greg,

Quoted 11 lines · Greg KH

Quoted 9 lines · Sudip Mukherjee
Hi Greg,
Quoted 5 lines · On Mon, Nov 23, 2020 at 10:16:47AM +0100, [email protected] wrote:
The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Quoted 5 lines

Quoted 1 line
Here is the backport for all the stable tree from v4.9.y to v5.4.y.
THis didn't apply to 4.9.y, are you sure you tried that?
Anyway, queued up for all other branches, thanks!

I was modifying one of my script which pulls in the stablerc and stable-queue and I have completely messed up today :( Please drop this from v4.14.y. It will fail to build there. I had been working on the version for v4.14.y and v4.9.y, but I will keep it aside for today.
Really sorry for the confusion.

Signature · 2 lines

Regards
Sudip

Sudip Mukherjee

2020-11-24 19:29 UTC #5 lore raw

Re: FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

Quoted 19 lines · Sudip Mukherjee

Hi Greg,
Quoted 15 lines · Greg KH
Quoted 10 lines · Sudip Mukherjee
Hi Greg,
Quoted 5 lines · On Mon, Nov 23, 2020 at 10:16:47AM +0100, [email protected] wrote:
The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Here is the backport for all the stable tree from v4.9.y to v5.4.y.
THis didn't apply to 4.9.y, are you sure you tried that?

Quoted 8 lines

Quoted 1 line
Anyway, queued up for all other branches, thanks!
I was modifying one of my script which pulls in the stablerc and stable-queue and I have completely messed up today :( Please drop this from v4.14.y. It will fail to build there. I had been working on the version for v4.14.y and v4.9.y, but I will keep it aside for today.
Really sorry for the confusion.

v4.19.y also. :(
I have rechecked and only v5.4.y is ok.

Signature · 2 lines

Regards
Sudip

Greg KH

author maintainer

2020-11-24 20:12 UTC #6 lore raw

Re: FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

Quoted 30 lines · Sudip Mukherjee

Quoted 19 lines · Sudip Mukherjee
Hi Greg,
Quoted 15 lines · Greg KH
Quoted 10 lines · Sudip Mukherjee
Hi Greg,
Quoted 5 lines · On Mon, Nov 23, 2020 at 10:16:47AM +0100, [email protected] wrote:
The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Here is the backport for all the stable tree from v4.9.y to v5.4.y.
THis didn't apply to 4.9.y, are you sure you tried that?
Quoted 8 lines
Quoted 1 line
Anyway, queued up for all other branches, thanks!
I was modifying one of my script which pulls in the stablerc and stable-queue and I have completely messed up today :( Please drop this from v4.14.y. It will fail to build there. I had been working on the version for v4.14.y and v4.9.y, but I will keep it aside for today.
Really sorry for the confusion.

Quoted 2 lines

v4.19.y also. :(
I have rechecked and only v5.4.y is ok.

Now dropped from both trees, my CI testing also just hit this :)

thanks,

gre gk-h

Lukas Wunner

Acked-by Florian Fainelli Tested-by Florian Fainelli

2020-12-05 09:34 UTC #7 lore raw

Re: FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

In reply to Sudip Mukherjee

Quoted 18 lines · Sudip Mukherjee

Quoted 8 lines · Sudip Mukherjee
Quoted 7 lines · Greg KH
Quoted 6 lines · Sudip Mukherjee
Quoted 4 lines · On Mon, Nov 23, 2020 at 10:16:47AM +0100, [email protected] wrote:
The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Here is the backport for all the stable tree from v4.9.y to v5.4.y.
I was modifying one of my script which pulls in the stablerc and stable-queue and I have completely messed up today :( Please drop this from v4.14.y. It will fail to build there. I had been working on the version for v4.14.y and v4.9.y, but I will keep it aside for today.
Really sorry for the confusion.

Quoted 2 lines

v4.19.y also. :(
I have rechecked and only v5.4.y is ok.

Sudip's patch for 4.19 is actually correct, but it depends on commit 5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation"). If that commit is cherry-picked to 4.19 (applies cleanly), Sudip's patch compiles without errors.

I'm attaching Sudip's patch for re-application, barring any objections against applying 5e844cc37a5c to older stable kernels.

I had tried to make the patch dependency explicit by including the following tag in the commit message:

Cc <[email protected]> # v4.9+: 123456789abc: spi: Introduce device-managed SPI controller allocation

I didn't know the SHA-1 hash the commit would get, so I used 123456789abc as a placeholder. (Both 5e844cc37a5c and the $subject patch were part of the same series.) Sorry for the confusion.

Thanks,

Lukas

commit 63c5395bb7a9777a33f0e7b5906f2c0170a23692 upstream

bcm_qspi_remove() calls spi_unregister_master() even though bcm_qspi_probe() calls devm_spi_register_master(). The spi_master is therefore unregistered and freed twice on unbind.

Moreover, since commit 0392727c261b ("spi: bcm-qspi: Handle clock probe deferral"), bcm_qspi_probe() leaks the spi_master allocation if the call to devm_clk_get_optional() fails.

Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound and also avoids the spi_master leak on probe.

Fixes fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")

Signed-off-by Lukas Wunner <[email protected]>

Cc <[email protected]> # v4.9+: 123456789abc: spi: Introduce device-managed SPI controller allocation

Cc <[email protected]> # v4.9+

Cc Kamal Dasu <[email protected]>

Acked-by Florian Fainelli <[email protected]>

Tested-by Florian Fainelli <[email protected]>

Link https://lore.kernel.org/r/5e31a9a59fd1c0d0b795b2fe219f25e5ee855f9d.1605121038.git.lukas@wunner.de open here

Signed-off-by Mark Brown <[email protected]>

[sudip: adjust context]

Signed-off-by Sudip Mukherjee <[email protected]>

---
 drivers/spi/spi-bcm-qspi.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

drivers/spi/spi-bcm-qspi.c +12 ~~-22~~

index d0afe0b1599f..8a4be34bccfd 100644
@@ -1213,7 +1213,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	if (!of_match_node(bcm_qspi_of_match, dev->of_node))
 		return -ENODEV;
 
-	master = spi_alloc_master(dev, sizeof(struct bcm_qspi));
+	master = devm_spi_alloc_master(dev, sizeof(struct bcm_qspi));
 	if (!master) {
 		dev_err(dev, "error allocating spi_master\n");
 		return -ENOMEM;

@@ -1252,21 +1252,17 @@ int bcm_qspi_probe(struct platform_device *pdev,
 
 	if (res) {
 		qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[MSPI])) {
-			ret = PTR_ERR(qspi->base[MSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[MSPI]))
+			return PTR_ERR(qspi->base[MSPI]);
 	} else {
-		goto qspi_resource_err;
+		return 0;
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi");
 	if (res) {
 		qspi->base[BSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[BSPI])) {
-			ret = PTR_ERR(qspi->base[BSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[BSPI]))
+			return PTR_ERR(qspi->base[BSPI]);
 		qspi->bspi_mode = true;
 	} else {
 		qspi->bspi_mode = false;

@@ -1277,18 +1273,14 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg");
 	if (res) {
 		qspi->base[CHIP_SELECT]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[CHIP_SELECT])) {
-			ret = PTR_ERR(qspi->base[CHIP_SELECT]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[CHIP_SELECT]))
+			return PTR_ERR(qspi->base[CHIP_SELECT]);
 	}
 
 	qspi->dev_ids = kcalloc(num_irqs, sizeof(struct bcm_qspi_dev_id),
 				GFP_KERNEL);
-	if (!qspi->dev_ids) {
-		ret = -ENOMEM;
-		goto qspi_resource_err;
-	}
+	if (!qspi->dev_ids)
+		return -ENOMEM;
 
 	for (val = 0; val < num_irqs; val++) {
 		irq = -1;

@@ -1357,7 +1349,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	qspi->xfer_mode.addrlen = -1;
 	qspi->xfer_mode.hp = -1;
 
-	ret = devm_spi_register_master(&pdev->dev, master);
+	ret = spi_register_master(master);
 	if (ret < 0) {
 		dev_err(dev, "can't register master\n");
 		goto qspi_reg_err;

@@ -1370,8 +1362,6 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	clk_disable_unprepare(qspi->clk);
 qspi_probe_err:
 	kfree(qspi->dev_ids);
-qspi_resource_err:
-	spi_master_put(master);
 	return ret;
 }
 /* probe function to be called by SoC specific platform driver probe */

@@ -1381,10 +1371,10 @@ int bcm_qspi_remove(struct platform_device *pdev)
 {
 	struct bcm_qspi *qspi = platform_get_drvdata(pdev);
 
+	spi_unregister_master(qspi->master);
 	bcm_qspi_hw_uninit(qspi);
 	clk_disable_unprepare(qspi->clk);
 	kfree(qspi->dev_ids);
-	spi_unregister_master(qspi->master);
 
 	return 0;
 }

Signature · 1 line

2.11.0

Lukas Wunner

2020-12-05 16:57 UTC #8 lore raw

Re: FAILED: patch "[PATCH] spi: bcm-qspi: Fix use-after-free on unbind" failed to apply to 5.4-stable tree

Quoted 19 lines · Lukas Wunner

Quoted 18 lines · Sudip Mukherjee
Quoted 16 lines · Sudip Mukherjee
Quoted 8 lines · Greg KH
Quoted 7 lines · Sudip Mukherjee
Quoted 4 lines · On Mon, Nov 23, 2020 at 10:16:47AM +0100, [email protected] wrote:
The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Here is the backport for all the stable tree from v4.9.y to v5.4.y.
I was modifying one of my script which pulls in the stablerc and stable-queue and I have completely messed up today :( Please drop this from v4.14.y. It will fail to build there. I had been working on the version for v4.14.y and v4.9.y, but I will keep it aside for today.
Really sorry for the confusion.

Quoted 7 lines

Quoted 2 lines
v4.19.y also. :(
I have rechecked and only v5.4.y is ok.
Sudip's patch for 4.19 is actually correct, but it depends on commit 5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation"). If that commit is cherry-picked to 4.19 (applies cleanly), Sudip's patch compiles without errors.

Same situation for 4.14:

Sudip's patch applies and compiles cleanly if 5e844cc37a5c is applied before, as do all the backports for spi-bcm2835.c and spi-bcm2835aux.c I sent out earlier today.

Thanks,

Lukas

Lukas Wunner

2020-12-05 18:44 UTC #9 lore raw

Quoted 4 lines · On Mon, Nov 23, 2020 at 09:52:50AM +0100, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Below please find the backport of 5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation") to the 4.9-stable tree.

This commit is a prerequisite to fix use-after-free bugs in a number of SPI drivers, including spi-bcm-qspi.c, for which I'll send a 4.9 backport in a minute.

Thanks!

-- >8 --
Subject: [PATCH] spi: Introduce device-managed SPI controller allocation

[ Upstream commit 5e844cc37a5cbaa460e68f9a989d321d63088a89 ]

SPI driver probing currently comprises two steps, whereas removal comprises only one step:

spi_alloc_master()
spi_register_master()

spi_unregister_master()

That's because spi_unregister_master() calls device_unregister() instead of device_del(), thereby releasing the reference on the spi_master which was obtained by spi_alloc_master().

An SPI driver's private data is contained in the same memory allocation as the spi_master struct. Thus, once spi_unregister_master() has been called, the private data is inaccessible. But some drivers need to access it after spi_unregister_master() to perform further teardown steps.

Introduce devm_spi_alloc_master(), which releases a reference on the spi_master struct only after the driver has unbound, thereby keeping the memory allocation accessible. Change spi_unregister_master() to not release a reference if the spi_master was allocated by the new devm function.

The present commit is small enough to be backportable to stable. It allows fixing drivers which use the private data in their ->remove() hook after it's been freed. It also allows fixing drivers which neglect to release a reference on the spi_master in the probe error path.

Long-term, most SPI drivers shall be moved over to the devm function introduced herein. The few that can't shall be changed in a treewide commit to explicitly release the last reference on the master. That commit shall amend spi_unregister_master() to no longer release a reference, thereby completing the migration.

As a result, the behaviour will be less surprising and more consistent with subsystems such as IIO, which also includes the private data in the allocation of the generic iio_dev struct, but calls device_del() in iio_device_unregister().

Signed-off-by Lukas Wunner <[email protected]>

Link https://lore.kernel.org/r/272bae2ef08abd21388c98e23729886663d19192.1605121038.git.lukas@wunner.de open here

Signed-off-by Mark Brown <[email protected]>

---
 drivers/spi/spi.c       | 54 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/spi/spi.h |  2 ++
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/spi/spi.c +53 -1

index 3fadc564d781..c7c9ca3178ad 100644
@@ -1827,6 +1827,46 @@ struct spi_master *spi_alloc_master(struct device *dev, unsigned size)
 }
 EXPORT_SYMBOL_GPL(spi_alloc_master);
 
+static void devm_spi_release_master(struct device *dev, void *master)
+{
+	spi_master_put(*(struct spi_master **)master);
+}
+
+/**
+ * devm_spi_alloc_master - resource-managed spi_alloc_master()
+ * @dev: physical device of SPI master
+ * @size: how much zeroed driver-private data to allocate
+ * Context: can sleep
+ *
+ * Allocate an SPI master and automatically release a reference on it
+ * when @dev is unbound from its driver.  Drivers are thus relieved from
+ * having to call spi_master_put().
+ *
+ * The arguments to this function are identical to spi_alloc_master().
+ *
+ * Return: the SPI master structure on success, else NULL.
+ */
+struct spi_master *devm_spi_alloc_master(struct device *dev, unsigned int size)
+{
+	struct spi_master **ptr, *master;
+
+	ptr = devres_alloc(devm_spi_release_master, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return NULL;
+
+	master = spi_alloc_master(dev, size);
+	if (master) {
+		*ptr = master;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return master;
+}
+EXPORT_SYMBOL_GPL(devm_spi_alloc_master);
+
 #ifdef CONFIG_OF
 static int of_spi_register_master(struct spi_master *master)
 {

@@ -2007,6 +2047,11 @@ int devm_spi_register_master(struct device *dev, struct spi_master *master)
 }
 EXPORT_SYMBOL_GPL(devm_spi_register_master);
 
+static int devm_spi_match_master(struct device *dev, void *res, void *master)
+{
+	return *(struct spi_master **)res == master;
+}
+
 static int __unregister(struct device *dev, void *null)
 {
 	spi_unregister_device(to_spi_device(dev));

@@ -2036,7 +2081,14 @@ void spi_unregister_master(struct spi_master *master)
 	list_del(&master->list);
 	mutex_unlock(&board_lock);
 
-	device_unregister(&master->dev);
+	device_del(&master->dev);
+
+	/* Release the last reference on the master if its driver
+	 * has not yet been converted to devm_spi_alloc_master().
+	 */
+	if (!devres_find(master->dev.parent, devm_spi_release_master,
+			 devm_spi_match_master, master))
+		put_device(&master->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_master);

include/linux/spi/spi.h +2 -0

index 4b743ac35396..8470695e5dd7 100644
@@ -601,6 +601,8 @@ extern void spi_finalize_current_transfer(struct spi_master *master);
 /* the spi driver core manages memory for the spi_master classdev */
 extern struct spi_master *
 spi_alloc_master(struct device *host, unsigned size);
+extern struct spi_master *
+devm_spi_alloc_master(struct device *dev, unsigned int size);
 
 extern int spi_register_master(struct spi_master *master);
 extern int devm_spi_register_master(struct device *dev,

Signature · 1 line

2.29.2

Lukas Wunner

Acked-by Florian Fainelli Tested-by Florian Fainelli

2020-12-05 18:44 UTC #10 lore raw

Quoted 4 lines · On Mon, Nov 23, 2020 at 09:52:50AM +0100, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Below please find the backport of 63c5395bb7a9 to the 4.9-stable tree. It depends on the backport of 5e844cc37a5c I just sent out.

Thanks!

-- >8 --
Subject: [PATCH] spi: bcm-qspi: Fix use-after-free on unbind

[ Upstream commit 63c5395bb7a9777a33f0e7b5906f2c0170a23692 ]

bcm_qspi_remove() calls spi_unregister_master() even though bcm_qspi_probe() calls devm_spi_register_master(). The spi_master is therefore unregistered and freed twice on unbind.

Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound.

Fixes fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")

Signed-off-by Lukas Wunner <[email protected]>

Cc <[email protected]> # v4.9+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation

Cc <[email protected]> # v4.9+

Cc Kamal Dasu <[email protected]>

Acked-by Florian Fainelli <[email protected]>

Tested-by Florian Fainelli <[email protected]>

Link https://lore.kernel.org/r/5e31a9a59fd1c0d0b795b2fe219f25e5ee855f9d.1605121038.git.lukas@wunner.de open here

Signed-off-by Mark Brown <[email protected]>

---
 drivers/spi/spi-bcm-qspi.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

drivers/spi/spi-bcm-qspi.c +12 ~~-22~~

index 1906b2319e5b..5453910d8abc 100644
@@ -1185,7 +1185,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	if (!of_match_node(bcm_qspi_of_match, dev->of_node))
 		return -ENODEV;
 
-	master = spi_alloc_master(dev, sizeof(struct bcm_qspi));
+	master = devm_spi_alloc_master(dev, sizeof(struct bcm_qspi));
 	if (!master) {
 		dev_err(dev, "error allocating spi_master\n");
 		return -ENOMEM;

@@ -1218,21 +1218,17 @@ int bcm_qspi_probe(struct platform_device *pdev,
 
 	if (res) {
 		qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[MSPI])) {
-			ret = PTR_ERR(qspi->base[MSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[MSPI]))
+			return PTR_ERR(qspi->base[MSPI]);
 	} else {
-		goto qspi_resource_err;
+		return 0;
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi");
 	if (res) {
 		qspi->base[BSPI]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[BSPI])) {
-			ret = PTR_ERR(qspi->base[BSPI]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[BSPI]))
+			return PTR_ERR(qspi->base[BSPI]);
 		qspi->bspi_mode = true;
 	} else {
 		qspi->bspi_mode = false;

@@ -1243,18 +1239,14 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg");
 	if (res) {
 		qspi->base[CHIP_SELECT]  = devm_ioremap_resource(dev, res);
-		if (IS_ERR(qspi->base[CHIP_SELECT])) {
-			ret = PTR_ERR(qspi->base[CHIP_SELECT]);
-			goto qspi_resource_err;
-		}
+		if (IS_ERR(qspi->base[CHIP_SELECT]))
+			return PTR_ERR(qspi->base[CHIP_SELECT]);
 	}
 
 	qspi->dev_ids = kcalloc(num_irqs, sizeof(struct bcm_qspi_dev_id),
 				GFP_KERNEL);
-	if (!qspi->dev_ids) {
-		ret = -ENOMEM;
-		goto qspi_resource_err;
-	}
+	if (!qspi->dev_ids)
+		return -ENOMEM;
 
 	for (val = 0; val < num_irqs; val++) {
 		irq = -1;

@@ -1330,7 +1322,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	qspi->xfer_mode.addrlen = -1;
 	qspi->xfer_mode.hp = -1;
 
-	ret = devm_spi_register_master(&pdev->dev, master);
+	ret = spi_register_master(master);
 	if (ret < 0) {
 		dev_err(dev, "can't register master\n");
 		goto qspi_reg_err;

@@ -1343,8 +1335,6 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	clk_disable_unprepare(qspi->clk);
 qspi_probe_err:
 	kfree(qspi->dev_ids);
-qspi_resource_err:
-	spi_master_put(master);
 	return ret;
 }
 /* probe function to be called by SoC specific platform driver probe */

@@ -1355,10 +1345,10 @@ int bcm_qspi_remove(struct platform_device *pdev)
 	struct bcm_qspi *qspi = platform_get_drvdata(pdev);
 
 	platform_set_drvdata(pdev, NULL);
+	spi_unregister_master(qspi->master);
 	bcm_qspi_hw_uninit(qspi);
 	clk_disable_unprepare(qspi->clk);
 	kfree(qspi->dev_ids);
-	spi_unregister_master(qspi->master);
 
 	return 0;
 }

Signature · 1 line

2.29.2