From: Lukas Wunner <lukas@wunner.de>
To: Mark Brown <broonie@kernel.org>
Cc: Vladimir Oltean <olteanv@gmail.com>,
Florian Fainelli <f.fainelli@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-spi <linux-spi@vger.kernel.org>,
Sascha Hauer <s.hauer@pengutronix.de>
Subject: Re: Use after free in bcm2835_spi_remove()
Date: Wed, 28 Oct 2020 10:59:46 +0100 [thread overview]
Message-ID: <20201028095946.GA25358@wunner.de> (raw)
In-Reply-To: <20201015125335.GB4390@sirena.org.uk>
On Thu, Oct 15, 2020 at 01:53:35PM +0100, Mark Brown wrote:
> 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:
> > 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.
[...]
> > + 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.
A combined memory allocation for struct spi_controller and the private
data has more benefits than just memory savings: Having them adjacent
in memory reduces the chance of cache misses. Also, one can get from
one struct to the other with a cheap subtraction (using container_of())
instead of having to chase pointers. So it helps performance. And a
lack of pointers arguably helps security.
Most subsystems embed the controller struct in the private data, but
there *is* precedence for doing it the other way round. E.g. the IIO
subsystem likewise appends the private data to the controller struct.
So I think that's fine, it need not and should not be changed.
The problem is that the ->probe() and ->remove() code is currently
asymmetric, which is unintuitive: On ->probe(), there's an allocation
step and a registration step:
spi_alloc_master()
spi_register_controller()
Whereas on ->remove(), there's no step to free the master which would
balance the prior alloc step:
spi_unregister_controller()
That's because the spi_controller struct is ref-counted and the last
ref is usually dropped by spi_unregister_controller(). If the private
data is accessed after the spi_unregister_controller() step, a ref
needs to be held.
I maintain that it would be more intuitive to automatically hold a ref.
We could offer a devm_spi_alloc_master() function which holds this ref
and automatically releases it on unbind.
There are three drivers which call spi_alloc_master() with a size of zero
for the private data. In these three cases it is fine to free the
spi_controller struct upon spi_unregister_controller(). So these drivers
can continue to use spi_alloc_master(). All other drivers could be
changed to use the new devm_spi_alloc_master(), or I could scrutinize
each of them and convert to the new function only if necessary.
Does this sound more convincing to you?
Thanks,
Lukas
next prev parent reply other threads:[~2020-10-29 2:02 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-13 23:48 Use after free in bcm2835_spi_remove() Florian Fainelli
2020-10-14 14:09 ` Lukas Wunner
2020-10-14 19:40 ` Vladimir Oltean
2020-10-14 20:25 ` Mark Brown
2020-10-14 21:20 ` Florian Fainelli
2020-10-22 12:12 ` Lukas Wunner
2020-10-15 5:38 ` Lukas Wunner
2020-10-15 12:53 ` Mark Brown
2020-10-28 9:59 ` Lukas Wunner [this message]
2020-10-29 22:24 ` Mark Brown
2020-11-11 19:07 ` [PATCH 0/4] Use-after-free be gone Lukas Wunner
2020-11-11 19:07 ` [PATCH 1/4] spi: Introduce device-managed SPI controller allocation Lukas Wunner
2020-11-11 19:07 ` [PATCH 2/4] spi: bcm2835: Fix use-after-free on unbind Lukas Wunner
2020-11-11 20:18 ` Florian Fainelli
2020-11-11 19:07 ` [PATCH 3/4] spi: bcm2835aux: " Lukas Wunner
2020-11-11 19:07 ` [PATCH 4/4] spi: bcm-qspi: " Lukas Wunner
2020-11-11 21:12 ` Florian Fainelli
2020-11-12 13:50 ` [PATCH 0/4] Use-after-free be gone Mark Brown
2020-11-12 19:39 ` Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201028095946.GA25358@wunner.de \
--to=lukas@wunner.de \
--cc=broonie@kernel.org \
--cc=f.fainelli@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=s.hauer@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).