Re: Use after free in bcm2835_spi_remove()

[Florian Fainelli <f.fainelli@gmail.com>]

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

On 10/14/20 1:25 PM, Mark Brown wrote:
> On Wed, Oct 14, 2020 at 10:40:35PM +0300, Vladimir Oltean wrote:
>> On Wed, Oct 14, 2020 at 04:09:12PM +0200, Lukas Wunner wrote:
> 
>>> Apparently the problem is that spi_unregister_controller() drops the
>>> last ref on the controller, causing it to be freed, and afterwards we
>>> access the controller's private data, which is part of the same
>>> allocation as struct spi_controller:
> 
>>> bcm2835_spi_remove()
>>>   spi_unregister_controller()
>>>     device_unregister()
>>>       put_device()
>>>         spi_controller_release()  #  spi_master_class.dev_release()
>>> 	  kfree(ctlr)
>>>   bcm2835_dma_release(ctlr, bs)
> 
>> Also see these threads:
>> https://lore.kernel.org/linux-spi/20200922112241.GO4792@sirena.org.uk/T/#t
>> https://lore.kernel.org/linux-spi/270b94fd1e546d0c17a735c1f55500e58522da04.camel@suse.de/T/#u
> 
> Right, the proposed patch is yet another way to fix the issue - it all
> comes back to the fact that you shouldn't be using the driver data after
> unregistering if it was allocated as part of allocating the controller.
> This framework feature is unfortunately quite error prone.

Lukas, your patch works fine for me and is only two lines, so maybe
better suited for stable. How about the attached patch?
-- 
Florian

[-- Attachment #2: 0001-spi-bcm2835-Fix-use-after-free-in-bcm2835_spi_remove.patch --]
[-- Type: text/x-patch, Size: 4973 bytes --]

From a4ee9da1ef09f9ddb04060e644b9c34fd532c189 Mon Sep 17 00:00:00 2001
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 14 Oct 2020 14:15:28 -0700
Subject: [PATCH] spi: bcm2835: Fix use-after-free in bcm2835_spi_remove()

In bcm2835_spi_remove(), spi_controller_unregister() will free the ctlr
reference which will lead to an use after free in bcm2835_release_dma().

To avoid this use after free, allocate the bcm2835_spi structure with a
different lifecycle than the spi_controller structure such that we
unregister the SPI controller, free up all the resources and finally let
device managed allocations free the bcm2835_spi structure.

Fixes: 05897c710e8e ("spi: bcm2835: Tear down DMA before turning off SPI controller")
Fixes: 3ecd37edaa2a ("spi: bcm2835: enable dma modes for transfers meeting certain conditions")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/spi/spi-bcm2835.c | 46 +++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 41986ac0fbfb..d66358e6b5cd 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -847,42 +847,41 @@ static bool bcm2835_spi_can_dma(struct spi_controller *ctlr,
 	return true;
 }
 
-static void bcm2835_dma_release(struct spi_controller *ctlr,
-				struct bcm2835_spi *bs)
+static void bcm2835_dma_release(struct bcm2835_spi *bs,
+				struct dma_chan *dma_tx,
+				struct dma_chan *dma_rx)
 {
 	int i;
 
-	if (ctlr->dma_tx) {
-		dmaengine_terminate_sync(ctlr->dma_tx);
+	if (dma_tx) {
+		dmaengine_terminate_sync(dma_tx);
 
 		if (bs->fill_tx_desc)
 			dmaengine_desc_free(bs->fill_tx_desc);
 
 		if (bs->fill_tx_addr)
-			dma_unmap_page_attrs(ctlr->dma_tx->device->dev,
+			dma_unmap_page_attrs(dma_tx->device->dev,
 					     bs->fill_tx_addr, sizeof(u32),
 					     DMA_TO_DEVICE,
 					     DMA_ATTR_SKIP_CPU_SYNC);
 
-		dma_release_channel(ctlr->dma_tx);
-		ctlr->dma_tx = NULL;
+		dma_release_channel(dma_tx);
 	}
 
-	if (ctlr->dma_rx) {
-		dmaengine_terminate_sync(ctlr->dma_rx);
+	if (dma_rx) {
+		dmaengine_terminate_sync(dma_rx);
 
 		for (i = 0; i < BCM2835_SPI_NUM_CS; i++)
 			if (bs->clear_rx_desc[i])
 				dmaengine_desc_free(bs->clear_rx_desc[i]);
 
 		if (bs->clear_rx_addr)
-			dma_unmap_single(ctlr->dma_rx->device->dev,
+			dma_unmap_single(dma_rx->device->dev,
 					 bs->clear_rx_addr,
 					 sizeof(bs->clear_rx_cs),
 					 DMA_TO_DEVICE);
 
-		dma_release_channel(ctlr->dma_rx);
-		ctlr->dma_rx = NULL;
+		dma_release_channel(dma_rx);
 	}
 }
 
@@ -1010,7 +1009,7 @@ static int bcm2835_dma_init(struct spi_controller *ctlr, struct device *dev,
 	dev_err(dev, "issue configuring dma: %d - not using DMA mode\n",
 		ret);
 err_release:
-	bcm2835_dma_release(ctlr, bs);
+	bcm2835_dma_release(bs, ctlr->dma_tx, ctlr->dma_rx);
 err:
 	/*
 	 * Only report error for deferred probing, otherwise fall back to
@@ -1291,12 +1290,17 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	struct bcm2835_spi *bs;
 	int err;
 
+	bs = devm_kzalloc(&pdev->dev, sizeof(*bs), GFP_KERNEL);
+	if (!bs)
+		return -ENOMEM;
+
 	ctlr = spi_alloc_master(&pdev->dev, ALIGN(sizeof(*bs),
 						  dma_get_cache_alignment()));
 	if (!ctlr)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, ctlr);
+	spi_controller_set_devdata(ctlr, bs);
+	platform_set_drvdata(pdev, bs);
 
 	ctlr->use_gpio_descriptors = true;
 	ctlr->mode_bits = BCM2835_SPI_MODE_BITS;
@@ -1308,7 +1312,6 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	ctlr->prepare_message = bcm2835_spi_prepare_message;
 	ctlr->dev.of_node = pdev->dev.of_node;
 
-	bs = spi_controller_get_devdata(ctlr);
 	bs->ctlr = ctlr;
 
 	bs->regs = devm_platform_ioremap_resource(pdev, 0);
@@ -1362,7 +1365,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	return 0;
 
 out_dma_release:
-	bcm2835_dma_release(ctlr, bs);
+	bcm2835_dma_release(bs, ctlr->dma_tx, ctlr->dma_rx);
 out_clk_disable:
 	clk_disable_unprepare(bs->clk);
 out_controller_put:
@@ -1372,14 +1375,19 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 
 static int bcm2835_spi_remove(struct platform_device *pdev)
 {
-	struct spi_controller *ctlr = platform_get_drvdata(pdev);
-	struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
+	struct bcm2835_spi *bs = platform_get_drvdata(pdev);
+	struct spi_controller *ctlr = bs->ctlr;
+	struct dma_chan *tx_chan = ctlr->dma_tx;
+	struct dma_chan *rx_chan = ctlr->dma_rx;
 
 	bcm2835_debugfs_remove(bs);
 
 	spi_unregister_controller(ctlr);
 
-	bcm2835_dma_release(ctlr, bs);
+	/* ctlr is freed by spi_unregister_controller() use the cached dma_chan
+	 * references.
+	 */
+	bcm2835_dma_release(bs, tx_chan, rx_chan);
 
 	/* Clear FIFOs, and disable the HW block */
 	bcm2835_wr(bs, BCM2835_SPI_CS,
-- 
2.25.1