users of spi_unregister_controller() broken?

users of spi_unregister_controller() broken?

[Sascha Hauer]

Hi All,

I see the following KASan use-after-free messages from the fsl-dspi
driver. It seems that after spi_unregister_controller() has been called
no references to the SPI controller device are left and the device is
freed in spi_controller_release(). This also frees the driver data
structure which is allocated with spi_alloc_master(). Nevertheless all
users of spi_unregister_controller() still use their driver data after
having called spi_unregister_controller().

Any idea what to do about this?

Sascha

==================================================================
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
==================================================================

-- 
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: users of spi_unregister_controller() broken?

[Mark Brown]

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

On Mon, Sep 21, 2020 at 01:08:05PM +0200, Sascha Hauer wrote:

> I see the following KASan use-after-free messages from the fsl-dspi
> driver. It seems that after spi_unregister_controller() has been called
> no references to the SPI controller device are left and the device is
> freed in spi_controller_release(). This also frees the driver data
> structure which is allocated with spi_alloc_master(). Nevertheless all
> users of spi_unregister_controller() still use their driver data after
> having called spi_unregister_controller().

> Any idea what to do about this?

Drivers that need to use their driver_data in remove() should either use
devm_spi_register_controller() or not use spi_register_controller() to
allocate driver_data.  Depending on what the remove function does it may
not be practical to do the former, most likely it won't be.

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

Re: users of spi_unregister_controller() broken?

[Vladimir Oltean]

On Mon, Sep 21, 2020 at 01:00:29PM +0100, Mark Brown wrote:
> On Mon, Sep 21, 2020 at 01:08:05PM +0200, Sascha Hauer wrote:
> 
> > I see the following KASan use-after-free messages from the fsl-dspi
> > driver. It seems that after spi_unregister_controller() has been called
> > no references to the SPI controller device are left and the device is
> > freed in spi_controller_release(). This also frees the driver data
> > structure which is allocated with spi_alloc_master(). Nevertheless all
> > users of spi_unregister_controller() still use their driver data after
> > having called spi_unregister_controller().
> 
> > Any idea what to do about this?
> 
> Drivers that need to use their driver_data in remove() should either use
> devm_spi_register_controller() or not use spi_register_controller() to
> allocate driver_data.  Depending on what the remove function does it may
> not be practical to do the former, most likely it won't be.

Please think of the consequences of making such a change, especially to
so many drivers.

For example, in spi-fsl-dspi.c, the reason why
spi_unregister_controller() is first is:

From 7684580d45bd3d84ed9b453a4cadf7a9a5605a3f Mon Sep 17 00:00:00 2001
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: Mon, 22 Jun 2020 13:05:40 +0200
Subject: [PATCH] spi: spi-fsl-dspi: Fix lockup if device is removed during SPI
 transfer

During device removal, the driver should unregister the SPI controller
and stop the hardware.  Otherwise the dspi_transfer_one_message() could
wait on completion infinitely.

Additionally, calling spi_unregister_controller() first in device
removal reverse-matches the probe function, where SPI controller is
registered at the end.

Fixes: 05209f457069 ("spi: fsl-dspi: add missing clk_disable_unprepare() in dspi_remove()")
Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200622110543.5035-1-krzk@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>

We want to make sure there is no SPI transaction left in flight when we
remove the driver. So we need to unregister from the SPI core first, to
make it stop sending them.

I understand there is no spi_unregister_controller / spi_free_controller
API as there is for unregister_netdev / free_netdev.

Does it help if you call spi_controller_get(dspi->ctlr) at the beginning
of dspi_remove, and spi_controller_put(dspi->ctlr) at the end of it?

Thanks,
-Vladimir

Re: users of spi_unregister_controller() broken?

[Mark Brown]

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

On Tue, Sep 22, 2020 at 01:07:15PM +0300, Vladimir Oltean wrote:

> Does it help if you call spi_controller_get(dspi->ctlr) at the beginning
> of dspi_remove, and spi_controller_put(dspi->ctlr) at the end of it?

That'd be another way to do it but TBH it feels pretty contorted.  Just
allocating separate driver data seems a lot less trouble, or taking
copies of fields that are needed to clean up before freeing the
controller.

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