linux-kernel - Re: Use after free in bcm2835_spi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201015125335.GB4390@sirena.org.uk>
Date:   Thu, 15 Oct 2020 13:53:35 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Vladimir Oltean <olteanv@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>
Subject: Re: Use after free in bcm2835_spi_remove()

On Thu, Oct 15, 2020 at 07:38:29AM +0200, Lukas Wunner wrote:
> On Wed, Oct 14, 2020 at 09:25:05PM +0100, Mark Brown wrote:

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

> How about holding a ref on the controller as long as the SPI driver
> is bound to the controller's parent device?  See below for a patch,
> compile-tested only for lack of an SPI-equipped machine.

> Makes sense or dumb idea?

> If this approach is deemed to be a case of "midlayer fallacy",
> we could alternatively do this in a library function which drivers
> opt-in to.  Or, given that the majority of drivers seems to be affected,
> make it the default and allow drivers to opt-out.

...

> +	spi_controller_get(ctlr);
> +	ret = devm_add_action(dev, __spi_controller_put, ctlr);
> +	if (ret) {
> +		kfree(ctlr);

This feels a bit icky - we're masking a standard use after free bug that
affects devm in general, not just this instance, and so while it will
work it doesn't feel great.  If we did do this it'd need more comments
and should probably be conditional on using the feature.  TBH I'm just
thinking it's better to just remove the feature, it's clearly error
prone and pretty redundant with devm.  I'm not sure any memory savings
it's delivering are worth the sharp edges.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)