[0/4] Use-after-free be gone

Message

Lukas Wunner Nov. 11, 2020, 7:07 p.m. UTC

Here's my proposal to fix the use-after-free bugs reported by
Sascha Hauer and Florian Fainelli:

I scrutinized all SPI drivers in the v5.10 tree:

* There are 9 drivers with a use-after-free in the ->remove() hook
  caused by accessing driver private data after spi_unregister_controller().

* There are 8 drivers which leak the spi_controller in the ->probe()
  error path because of a missing spi_controller_put().

I'm introducing devm_spi_alloc_master/slave() which automatically
calls spi_controller_put() on ->remove().  This fixes both classes
of bugs while at the same time reducing code amount and complexity
in the ->probe() hook.

I propose that spi_controller_unregister() should no longer release
a reference on the spi_controller.  Instead, drivers need to either
do it themselves or use one of the devm functions introduced herein.
The vast majority of drivers can be converted to the devm functions.
See the commit message of patch [1/4] for the rationale and details.

Enclosed are patches for 3 Broadcom drivers.
Patches for the other drivers are on this branch:
https://github.com/l1k/linux/commits/spi_fixes

@Florian Fainelli:  Could you verify that there are no KASAN splats or
leaks with these patches?  Unfortunately I do not have any SPI-capable
hardware at my disposal right now, so can only compile-test.  You may
want to augment spi_controller_release() with a printk() to log when
the spi_controller is freed.

@Mark Brown:  Patches [2/4] to [4/4] reference the SHA-1 of patch [1/4]
in their stable tags.  Because the hash is unknown to me until you apply
the patch, I've used "123456789abc" as a placeholder.  You'll have to
replace the hash if/when applying.  Alternatively, only apply patch [1/4]
and I'll repost the other patches with the hash fixed up.

Thanks!


Lukas Wunner (4):
  spi: Introduce device-managed SPI controller allocation
  spi: bcm2835: Fix use-after-free on unbind
  spi: bcm2835aux: Fix use-after-free on unbind
  spi: bcm-qspi: Fix use-after-free on unbind

 drivers/spi/spi-bcm-qspi.c   | 34 ++++++++-------------
 drivers/spi/spi-bcm2835.c    | 24 +++++----------
 drivers/spi/spi-bcm2835aux.c | 21 +++++--------
 drivers/spi/spi.c            | 58 +++++++++++++++++++++++++++++++++++-
 include/linux/spi/spi.h      | 19 ++++++++++++
 5 files changed, 103 insertions(+), 53 deletions(-)

Comments

Mark Brown Nov. 12, 2020, 1:50 p.m. UTC | #1

On Wed, Nov 11, 2020 at 08:07:00PM +0100, Lukas Wunner wrote:
> Here's my proposal to fix the use-after-free bugs reported by
> Sascha Hauer and Florian Fainelli:

Please don't send new patches in reply to old threads, it can bury them
with the old discussion and can make things get missed.

Mark Brown Nov. 12, 2020, 7:39 p.m. UTC | #2

On Wed, 11 Nov 2020 20:07:00 +0100, Lukas Wunner wrote:
> Here's my proposal to fix the use-after-free bugs reported by
> Sascha Hauer and Florian Fainelli:
> 
> I scrutinized all SPI drivers in the v5.10 tree:
> 
> * There are 9 drivers with a use-after-free in the ->remove() hook
>   caused by accessing driver private data after spi_unregister_controller().
> 
> [...]

Applied to

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

Thanks!

[1/4] spi: Introduce device-managed SPI controller allocation
      commit: 5e844cc37a5cbaa460e68f9a989d321d63088a89
[2/4] spi: bcm2835: Fix use-after-free on unbind
      commit: e1483ac030fb4c57734289742f1c1d38dca61e22
[3/4] spi: bcm2835aux: Fix use-after-free on unbind
      commit: e13ee6cc4781edaf8c7321bee19217e3702ed481
[4/4] spi: bcm-qspi: Fix use-after-free on unbind
      commit: 63c5395bb7a9777a33f0e7b5906f2c0170a23692

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