Re: Use after free in bcm2835_spi_remove()

[Mark Brown]

On Wed, Oct 28, 2020 at 10:59:46AM +0100, Lukas Wunner wrote:
> On Thu, Oct 15, 2020 at 01:53:35PM +0100, Mark Brown wrote:

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

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

The performance arguments don't seem super compelling either way TBH
given what SPI does, cache misses accessing the private data seem
unlikely to be perceptible when operations boil down to accesses on the
SPI bus.

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

Given their ages I suspect IIO copied SPI; I do think it's this reversal
that's confusing things.

> 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 agree that it's the asymmetry here, the disagreement is about how to
fix it.  If we keep the allocations combined then that probably makes
sense but I'm at best unclear on the merit of keeping the allocations
combined.

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

I don't know that it's super intuitive to have to have an explicit free
in the driver - you could equally expect that having registered the
thing allocated with the core's custom allocation function with the core
that the core is now taking ownership of it (which is how SPI devices as
opposed to controllers work).  That's what makes me lean towards just
doing separate allocations, there's no possibility of expectations about
transferring ownership.  If it's *always* done with devm it kind of gets
hidden though so perhaps it's not so bad and my concern goes away...

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

It's only things that explicitly unregister the controller (rather than
using devm) that are going to be affected here, that's a *much* smaller
subset.  Everything else will be done with driver specific code and
hence private data usage before the controler goes away, though it looks
like a bunch (though not all) of them have other issues and are using
devm when they shouldn't.  That's a separate problem which ought to be
fixed anyway though.  The removal paths aren't exactly heavily stressed,
especially not under load.

In any case for your proposal your plan makes sense, I mostly just want
to avoid ending up with people getting confused in the other direction
and introducing another set of bugs.

Attachment: signature.asc
Description: PGP signature