Re: Use after free in bcm2835_spi_remove()

[Lukas Wunner <lukas@xxxxxxxxx>]

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