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

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

[Johan Hovold]

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.

Fixes: 307c897db762 ("spi: spi-imx: replace struct spi_imx_data::bitbang by pointer to struct spi_controller")
Cc: stable@vger.kernel.org	# 5.19
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/spi/spi-imx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 64c6c09e1e7b..a8d90c86a8a1 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -2401,6 +2401,8 @@ static void spi_imx_remove(struct platform_device *pdev)
 	struct spi_imx_data *spi_imx = spi_controller_get_devdata(controller);
 	int ret;
 
+	spi_controller_get(controller);
+
 	spi_unregister_controller(controller);
 
 	ret = pm_runtime_get_sync(spi_imx->dev);
@@ -2414,6 +2416,8 @@ static void spi_imx_remove(struct platform_device *pdev)
 	pm_runtime_disable(spi_imx->dev);
 
 	spi_imx_sdma_exit(spi_imx);
+
+	spi_controller_put(controller);
 }
 
 static int spi_imx_runtime_resume(struct device *dev)
-- 
2.52.0

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

[Marc Kleine-Budde]

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

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?

regards,
Marc

>
> Fixes: 307c897db762 ("spi: spi-imx: replace struct spi_imx_data::bitbang by pointer to struct spi_controller")
> Cc: stable@vger.kernel.org	# 5.19
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/spi/spi-imx.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 64c6c09e1e7b..a8d90c86a8a1 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -2401,6 +2401,8 @@ static void spi_imx_remove(struct platform_device *pdev)
>  	struct spi_imx_data *spi_imx = spi_controller_get_devdata(controller);
>  	int ret;
>
> +	spi_controller_get(controller);
> +
>  	spi_unregister_controller(controller);
>
>  	ret = pm_runtime_get_sync(spi_imx->dev);
> @@ -2414,6 +2416,8 @@ static void spi_imx_remove(struct platform_device *pdev)
>  	pm_runtime_disable(spi_imx->dev);
>
>  	spi_imx_sdma_exit(spi_imx);
> +
> +	spi_controller_put(controller);
>  }
>
>  static int spi_imx_runtime_resume(struct device *dev)
> --
> 2.52.0
>
>

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

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

[Johan Hovold]

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

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.

Johan

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

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

[Marc Kleine-Budde]

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

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.

Would using devm_spi_alloc_host() be an option here?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

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

[Johan Hovold]

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

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

[Marc Kleine-Budde]

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

On 23.03.2026 14:59:49, Johan Hovold wrote:
> 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).

Ah, nice! At the time I replied to the patch, the whole series was not
available on lore, yet.

Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

And thanks for taking the time in explaining me the details :)

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

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

[Johan Hovold]

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

On Mon, Mar 23, 2026 at 03:47:45PM +0100, Marc Kleine-Budde wrote:
> On 23.03.2026 14:59:49, Johan Hovold wrote:

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

This was supposed to say

	3f174274d224 ("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).
> 
> Ah, nice! At the time I replied to the patch, the whole series was not
> available on lore, yet.

Sorry, I should have CC:ed you the whole series.

> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> And thanks for taking the time in explaining me the details :)

No worries. :)

Johan

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