[PATCH 0/6] Fix use-after-free in SPI drivers

[PATCH 0/6] Fix use-after-free in SPI drivers

[Sascha Hauer]

Several SPI drivers continue to use private driver data after it has
been freed in spi_unregister_controller(). This results in a
use-after-free trace in KASan, see below for an example on the fsl-dspi
driver. This series fixes this on all drivers which follow the same
pattern. It is runtime tested on a Layerscape SoC with the fsl-dspi
driver, patches for the other drivers have been compile tested only.

=================================================================
BUG: KASAN: use-after-free in dspi_remove+0x48/0x1f8
Read of size 8 at addr ffff0009301b7610 by task systemd-shutdow/1

CPU: 3 PID: 1 Comm: systemd-shutdow Not tainted 5.9.0-rc2-00019-g74a89d7f9ed7 #22
Hardware name: TQ TQMLS1046A SoM on Arkona AT1130 (C300) board (DT)
Call trace:
 dump_backtrace+0x0/0x2b0
 show_stack+0x14/0x20
 dump_stack+0xe8/0x150
 print_address_description.constprop.0+0x6c/0x4e0
 kasan_report+0x130/0x1f8
 __asan_load8+0x88/0xc0
 dspi_remove+0x48/0x1f8
 dspi_shutdown+0xc/0x18
 platform_drv_shutdown+0x34/0x40
 device_shutdown+0x1ec/0x420
 kernel_restart_prepare+0x40/0x50
 kernel_restart+0x14/0x60
 __do_sys_reboot+0x294/0x2c0
 __arm64_sys_reboot+0x50/0x60
 el0_svc_common.constprop.0+0xa0/0x1d8
 do_el0_svc_compat+0x2c/0x58
 el0_sync_compat_handler+0x94/0x184
 el0_sync_compat+0x144/0x180

 Allocated by task 1:
 kasan_save_stack+0x24/0x50
 __kasan_kmalloc.isra.0+0xc0/0xe0
 kasan_kmalloc+0xc/0x18
 __kmalloc+0x208/0x360
 __spi_alloc_controller+0x2c/0xe8
 dspi_probe+0xb0/0xcc8
 platform_drv_probe+0x6c/0xc8
 really_probe+0x144/0x510
 driver_probe_device+0xc8/0xe0
 device_driver_attach+0x94/0xa0
 __driver_attach+0x70/0x110
 bus_for_each_dev+0xe4/0x158
 driver_attach+0x30/0x40
 bus_add_driver+0x21c/0x2b8
 driver_register+0xbc/0x1d0
 __platform_driver_register+0x7c/0x88
 fsl_dspi_driver_init+0x18/0x20
 do_one_initcall+0xa4/0x24c
 kernel_init_freeable+0x26c/0x2d4
 kernel_init+0x10/0x11c
 ret_from_fork+0x10/0x18

 Freed by task 1:
 kasan_save_stack+0x24/0x50
 kasan_set_track+0x24/0x38
 kasan_set_free_info+0x20/0x40
 __kasan_slab_free+0xfc/0x170
 kasan_slab_free+0x10/0x18
 kfree+0xac/0x318
 spi_controller_release+0xc/0x18
 device_release+0x7c/0xf0
 kobject_put+0xa4/0x170
 device_unregister+0x20/0x30
 spi_unregister_controller+0x104/0x178
 dspi_remove+0x40/0x1f8
 dspi_shutdown+0xc/0x18
 platform_drv_shutdown+0x34/0x40
 device_shutdown+0x1ec/0x420
 kernel_restart_prepare+0x40/0x50
 kernel_restart+0x14/0x60
 __do_sys_reboot+0x294/0x2c0
 __arm64_sys_reboot+0x50/0x60
 el0_svc_common.constprop.0+0xa0/0x1d8
 do_el0_svc_compat+0x2c/0x58
 el0_sync_compat_handler+0x94/0x184
 el0_sync_compat+0x144/0x180

The buggy address belongs to the object at ffff0009301b7000
 which belongs to the cache kmalloc-2k of size 2048
The buggy address is located 1552 bytes inside of
 2048-byte region [ffff0009301b7000, ffff0009301b7800)
The buggy address belongs to the page:
page:00000000debc463d refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x9b01b0
head:00000000debc463d order:3 compound_mapcount:0 compound_pincount:0
flags: 0x2ffff00000010200(slab|head)
raw: 2ffff00000010200 dead000000000100 dead000000000122 ffff000932003680
raw: 0000000000000000 0000000000080008 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff0009301b7500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff0009301b7580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff0009301b7600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                         ^
 ffff0009301b7680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff0009301b7700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Sascha Hauer (6):
  spi: fsl-dspi: Use devm_spi_register_controller()
  spi: atmel-quadspi: Use devm_spi_register_controller()
  spi: bcm2835: Use devm_spi_register_controller()
  spi: dw: Use devm_spi_register_controller()
  spi: pxa2xx: Use devm_spi_register_controller()
  spi: rpc-if: Use devm_spi_register_controller()

 drivers/spi/atmel-quadspi.c | 3 +--
 drivers/spi/spi-bcm2835.c   | 4 +---
 drivers/spi/spi-dw-core.c   | 4 +---
 drivers/spi/spi-fsl-dspi.c  | 5 +----
 drivers/spi/spi-pxa2xx.c    | 4 +---
 drivers/spi/spi-rpc-if.c    | 3 +--
 6 files changed, 6 insertions(+), 17 deletions(-)

-- 
2.28.0

[PATCH 1/6] spi: fsl-dspi: Use devm_spi_register_controller()

[Sascha Hauer]

Calling spi_unregister_controller() during driver remove results in
freeing the SPI controller and the associated driver data. Using it
later in dspi_remove() is a use-after-free bug. Register the controller
with devm_spi_register_controller() instead which makes calling
spi_unregister_controller() unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/spi-fsl-dspi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 91c6affe139c..a6c100b89360 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1416,7 +1416,7 @@ static int dspi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ctlr);
 
-	ret = spi_register_controller(ctlr);
+	ret = devm_spi_register_controller(&pdev->dev, ctlr);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Problem registering DSPI ctlr\n");
 		goto out_free_irq;
@@ -1440,9 +1440,6 @@ static int dspi_remove(struct platform_device *pdev)
 	struct spi_controller *ctlr = platform_get_drvdata(pdev);
 	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
 
-	/* Disconnect from the SPI framework */
-	spi_unregister_controller(dspi->ctlr);
-
 	/* Disable RX and TX */
 	regmap_update_bits(dspi->regmap, SPI_MCR,
 			   SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF,
-- 
2.28.0

[PATCH 2/6] spi: atmel-quadspi: Use devm_spi_register_controller()

[Sascha Hauer]

Calling spi_unregister_controller() during driver remove results in
freeing the SPI controller and the associated driver data. Using it
later in atmel_qspi_remove() is a use-after-free bug. Register the
controller with devm_spi_register_controller() instead which makes
calling spi_unregister_controller() unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/atmel-quadspi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 8c009c175f2c..3a5e0703bad9 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -628,7 +628,7 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 
 	atmel_qspi_init(aq);
 
-	err = spi_register_controller(ctrl);
+	err = devm_spi_register_controller(&pdev->dev, ctrl);
 	if (err)
 		goto disable_qspick;
 
@@ -649,7 +649,6 @@ static int atmel_qspi_remove(struct platform_device *pdev)
 	struct spi_controller *ctrl = platform_get_drvdata(pdev);
 	struct atmel_qspi *aq = spi_controller_get_devdata(ctrl);
 
-	spi_unregister_controller(ctrl);
 	atmel_qspi_write(QSPI_CR_QSPIDIS, aq, QSPI_CR);
 	clk_disable_unprepare(aq->qspick);
 	clk_disable_unprepare(aq->pclk);
-- 
2.28.0

[PATCH 3/6] spi: bcm2835: Use devm_spi_register_controller()

[Sascha Hauer]

Calling spi_unregister_controller() during driver remove results in
freeing the SPI controller and the associated driver data. Using it
later in bcm2835_spi_remove() is a use-after-free bug. Register the
controller with devm_spi_register_controller() instead which makes
calling spi_unregister_controller() unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/spi-bcm2835.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index c45d76c848c8..d22103f7beeb 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -1350,7 +1350,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 		goto out_dma_release;
 	}
 
-	err = spi_register_controller(ctlr);
+	err = devm_spi_register_controller(&pdev->dev, ctlr);
 	if (err) {
 		dev_err(&pdev->dev, "could not register SPI controller: %d\n",
 			err);
@@ -1377,8 +1377,6 @@ static int bcm2835_spi_remove(struct platform_device *pdev)
 
 	bcm2835_debugfs_remove(bs);
 
-	spi_unregister_controller(ctlr);
-
 	bcm2835_dma_release(ctlr, bs);
 
 	/* Clear FIFOs, and disable the HW block */
-- 
2.28.0

[PATCH 4/6] spi: dw: Use devm_spi_register_controller()

[Sascha Hauer]

Calling spi_unregister_controller() during driver remove results in
freeing the SPI controller and the associated driver data. Using it
later in dw_spi_remove_host() is a use-after-free bug. Register the
controller with devm_spi_register_controller() instead which makes
calling spi_unregister_controller() unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/spi-dw-core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 323c66c5db50..caeece6681b3 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -485,7 +485,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 		}
 	}
 
-	ret = spi_register_controller(master);
+	ret = devm_spi_register_controller(dev, master);
 	if (ret) {
 		dev_err(&master->dev, "problem registering spi master\n");
 		goto err_dma_exit;
@@ -509,8 +509,6 @@ void dw_spi_remove_host(struct dw_spi *dws)
 {
 	dw_spi_debugfs_remove(dws);
 
-	spi_unregister_controller(dws->master);
-
 	if (dws->dma_ops && dws->dma_ops->dma_exit)
 		dws->dma_ops->dma_exit(dws);
 
-- 
2.28.0

[PATCH 5/6] spi: pxa2xx: Use devm_spi_register_controller()

[Sascha Hauer]

Calling spi_unregister_controller() during driver remove results in
freeing the SPI controller and the associated driver data. Using it
later in pxa2xx_spi_remove() is a use-after-free bug. Register the
controller with devm_spi_register_controller() instead which makes
calling spi_unregister_controller() unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/spi-pxa2xx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 814268405ab0..858b9b925024 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1892,7 +1892,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 
 	/* Register with the SPI framework */
 	platform_set_drvdata(pdev, drv_data);
-	status = spi_register_controller(controller);
+	status = devm_spi_register_controller(&pdev->dev, controller);
 	if (status != 0) {
 		dev_err(&pdev->dev, "problem registering spi controller\n");
 		goto out_error_pm_runtime_enabled;
@@ -1923,8 +1923,6 @@ static int pxa2xx_spi_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(&pdev->dev);
 
-	spi_unregister_controller(drv_data->controller);
-
 	/* Disable the SSP at the peripheral and SOC level */
 	pxa2xx_spi_write(drv_data, SSCR0, 0);
 	clk_disable_unprepare(ssp->clk);
-- 
2.28.0

[PATCH 6/6] spi: rpc-if: Use devm_spi_register_controller()

[Sascha Hauer]

Calling spi_unregister_controller() during driver remove results in
freeing the SPI controller and the associated driver data. Using it
later in rpcif_spi_remove() is a use-after-free bug. Register the
controller with devm_spi_register_controller() instead which makes
calling spi_unregister_controller() unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/spi-rpc-if.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/spi/spi-rpc-if.c b/drivers/spi/spi-rpc-if.c
index ed3e548227f4..2140bb249796 100644
--- a/drivers/spi/spi-rpc-if.c
+++ b/drivers/spi/spi-rpc-if.c
@@ -156,7 +156,7 @@ static int rpcif_spi_probe(struct platform_device *pdev)
 
 	rpcif_hw_init(rpc, false);
 
-	error = spi_register_controller(ctlr);
+	error = devm_spi_register_controller(&pdev->dev, ctlr);
 	if (error) {
 		dev_err(&pdev->dev, "spi_register_controller failed\n");
 		goto err_put_ctlr;
@@ -175,7 +175,6 @@ static int rpcif_spi_remove(struct platform_device *pdev)
 	struct spi_controller *ctlr = platform_get_drvdata(pdev);
 	struct rpcif *rpc = spi_controller_get_devdata(ctlr);
 
-	spi_unregister_controller(ctlr);
 	rpcif_disable_rpm(rpc);
 
 	return 0;
-- 
2.28.0

Re: [PATCH 3/6] spi: bcm2835: Use devm_spi_register_controller()

[Nicolas Saenz Julienne]

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

On Tue, 2020-09-22 at 11:32 +0200, Sascha Hauer wrote:
> Calling spi_unregister_controller() during driver remove results in
> freeing the SPI controller and the associated driver data. Using it
> later in bcm2835_spi_remove() is a use-after-free bug. Register the
> controller with devm_spi_register_controller() instead which makes
> calling spi_unregister_controller() unnecessary.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

>  drivers/spi/spi-bcm2835.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index c45d76c848c8..d22103f7beeb 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -1350,7 +1350,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
>  		goto out_dma_release;
>  	}
>  
> -	err = spi_register_controller(ctlr);
> +	err = devm_spi_register_controller(&pdev->dev, ctlr);
>  	if (err) {
>  		dev_err(&pdev->dev, "could not register SPI controller: %d\n",
>  			err);
> @@ -1377,8 +1377,6 @@ static int bcm2835_spi_remove(struct platform_device *pdev)
>  
>  	bcm2835_debugfs_remove(bs);
>  
> -	spi_unregister_controller(ctlr);
> -
>  	bcm2835_dma_release(ctlr, bs);
>  
>  	/* Clear FIFOs, and disable the HW block */


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/6] spi: fsl-dspi: Use devm_spi_register_controller()

[Mark Brown]

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

On Tue, Sep 22, 2020 at 11:32:23AM +0200, Sascha Hauer wrote:

> @@ -1440,9 +1440,6 @@ static int dspi_remove(struct platform_device *pdev)
>  	struct spi_controller *ctlr = platform_get_drvdata(pdev);
>  	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
>  
> -	/* Disconnect from the SPI framework */
> -	spi_unregister_controller(dspi->ctlr);
> -
>  	/* Disable RX and TX */
>  	regmap_update_bits(dspi->regmap, SPI_MCR,
>  			   SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF,

Is this fix safe - what happens if we start another transaction between
disabling RX/TX and the unregistration taking effect?  Similar concerns
apply to some of the other patches.

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

Re: [PATCH 1/6] spi: fsl-dspi: Use devm_spi_register_controller()

[Sascha Hauer]

On Tue, Sep 22, 2020 at 10:44:37AM +0100, Mark Brown wrote:
> On Tue, Sep 22, 2020 at 11:32:23AM +0200, Sascha Hauer wrote:
> 
> > @@ -1440,9 +1440,6 @@ static int dspi_remove(struct platform_device *pdev)
> >  	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> >  	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
> >  
> > -	/* Disconnect from the SPI framework */
> > -	spi_unregister_controller(dspi->ctlr);
> > -
> >  	/* Disable RX and TX */
> >  	regmap_update_bits(dspi->regmap, SPI_MCR,
> >  			   SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF,
> 
> Is this fix safe - what happens if we start another transaction between
> disabling RX/TX and the unregistration taking effect?  Similar concerns
> apply to some of the other patches.

I asked myself the same question until I saw that a dozen of other drivers
also do it like this.

I don't think it's safe. Currently we only have spi_unregister_controller()
which both unregisters child SPI devices and frees the SPI controller along
with the driver data.
There are many drivers have something in their remove function like
disabling the hardware, clocks, freeing interrupts. They do it either
before calling spi_unregister_controller(), in which case messages may be
queued on a teared down hardware, or they do it after calling
spi_unregister_controller(), in which case they operate on already freed
data. IMO this only works when all drivers are fully managed with devm_*
functions and runtime pm so that the remove functions for all drivers
become empty.  Until this happens spi_unregister_controller() has to be
split up in a function actually unregistering the controller and another
one freeing the resources.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/6] spi: fsl-dspi: Use devm_spi_register_controller()

[Mark Brown]

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

On Tue, Sep 22, 2020 at 01:06:52PM +0200, Sascha Hauer wrote:
> On Tue, Sep 22, 2020 at 10:44:37AM +0100, Mark Brown wrote:
> > On Tue, Sep 22, 2020 at 11:32:23AM +0200, Sascha Hauer wrote:

> > > -	/* Disconnect from the SPI framework */
> > > -	spi_unregister_controller(dspi->ctlr);
> > > -
> > >  	/* Disable RX and TX */
> > >  	regmap_update_bits(dspi->regmap, SPI_MCR,
> > >  			   SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF,

> > Is this fix safe - what happens if we start another transaction between
> > disabling RX/TX and the unregistration taking effect?  Similar concerns
> > apply to some of the other patches.

> I asked myself the same question until I saw that a dozen of other drivers
> also do it like this.

Right, it's unfortunately common for people to just use devm without
thinking about it too hard and it doesn't always get spotted :(

> data. IMO this only works when all drivers are fully managed with devm_*
> functions and runtime pm so that the remove functions for all drivers
> become empty.  Until this happens spi_unregister_controller() has to be
> split up in a function actually unregistering the controller and another
> one freeing the resources.

It's probably easier to just not have this option for allocating driver
data as part of the controller TBH, it's probably mroe trouble to use
safely than it's worth.

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

Re: [PATCH 0/6] Fix use-after-free in SPI drivers

[Lukas Wunner]

On Tue, Sep 22, 2020 at 11:32:22AM +0200, Sascha Hauer wrote:
>   spi: bcm2835: Use devm_spi_register_controller()
>   spi: dw: Use devm_spi_register_controller()
>   spi: pxa2xx: Use devm_spi_register_controller()

Sorry but NAK at least for these three patches as they re-introduce
the issue fixed by 9dd277ff92d0, 32e5b57232c0, ca8b19d61e3f.

The use-after-free is real but it needs to be addressed differently.
Let's continue the discussion in this thread:

https://lore.kernel.org/linux-spi/20201015053829.GA2461@wunner.de/

Thanks,

Lukas