Re: Use after free in bcm2835_spi_remove()

From: Lukas Wunner
Date: Wed Oct 28 2020 - 22:03:28 EST


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