Re: [PATCH 1/5] spi: imx: fix use-after-free on unbind

[Johan Hovold <johan@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Mon, Mar 23, 2026 at 12:57:42PM +0100, Marc Kleine-Budde wrote:
> On 23.03.2026 12:20:08, Johan Hovold wrote:
> > On Mon, Mar 23, 2026 at 12:00:59PM +0100, Marc Kleine-Budde wrote:
> > > On 23.03.2026 11:49:44, Johan Hovold wrote:
> > > > The SPI subsystem frees the controller and any subsystem allocated
> > > > driver data as part of deregistration (unless the allocation is device
> > > > managed).
> > > >
> > > > Take another reference before deregistering the controller so that the
> > > > driver data is not freed until the driver is done with it.
> > >
> > > Would re-ordering the spi_imx_remove() function be an alternative fix?
> > > I.e. call spi_unregister_controller() last?
> >
> > No, the controller needs to be deregistered before disabling clocks and
> > releasing other resources.
> 
> I see. So the API is a bit strange to use:
> 
> Allocate with spi_alloc_host(), free with spi_controller_put() before
> spi_register_controller(), the free with spi_unregister_controller()
> afterwards.
>
> But spi_unregister_controller() shuts down the SPI interface _and_ frees
> the memory. Which is the culprit here.

Indeed, it's a known issue with the SPI API. See for example:

	68b892f1fdc4 ("spi: document odd controller reference handling")
	5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation")
	f0c35a024cce ("spi: fix misleading controller deregistration kernel-doc")

> Would using devm_spi_alloc_host() be an option here?

It can also be used, but that's more intrusive so I did that as a
follow-on cleanup to the fix (see patch 2/5).

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]