The SPI API is asymmetric and the controller is freed as part of
deregistration (unless it has been allocated using
devm_spi_alloc_host/target()).

A recent change converting the managed registration function to use
devm_add_action_or_reset() inadvertently introduced a (mostly
theoretical) regression where a non-devres managed controller could be
freed as part of failed registration. This in turn would lead to
use-after-free in controller driver error paths.

Fix this by taking another reference before calling
devm_add_action_or_reset() and not releasing it on errors for
non-devres allocated controllers.

An alternative would be a partial revert of the offending commit, but
it is better to handle this explicitly until the API has been fixed
(e.g. see 5e844cc37a5c ("spi: Introduce device-managed SPI controller
allocation")).

Fixes: b6376dbed8e1 ("spi: Simplify devm_spi_*_controller()")
Reported-by: Felix Gu <ustc.gu@gmail.com>
Link: https://lore.kernel.org/all/20260324145548.139952-1-ustc.gu@gmail.com/
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/spi/spi.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index cb00619864cf..aac378e668a8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3537,8 +3537,19 @@ int devm_spi_register_controller(struct device *dev,
 	if (ret)
 		return ret;
 
-	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
+	/*
+	 * Prevent controller from being freed by spi_unregister_controller()
+	 * if devm_add_action_or_reset() fails for a non-devres allocated
+	 * controller.
+	 */
+	spi_controller_get(ctlr);
+
+	ret = devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
 
+	if (ret == 0 || ctlr->devm_allocated)
+		spi_controller_put(ctlr);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(devm_spi_register_controller);
 
-- 
2.52.0

On Wed, Mar 25, 2026 at 03:53:19PM +0100, Johan Hovold wrote:
> The SPI API is asymmetric and the controller is freed as part of
> deregistration (unless it has been allocated using
> devm_spi_alloc_host/target()).
> 
> A recent change converting the managed registration function to use
> devm_add_action_or_reset() inadvertently introduced a (mostly
> theoretical) regression where a non-devres managed controller could be
> freed as part of failed registration. This in turn would lead to
> use-after-free in controller driver error paths.
> 
> Fix this by taking another reference before calling
> devm_add_action_or_reset() and not releasing it on errors for
> non-devres allocated controllers.
> 
> An alternative would be a partial revert of the offending commit, but
> it is better to handle this explicitly until the API has been fixed
> (e.g. see 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> allocation")).

Thank you!
This is good TODO list now.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

On Thu, Mar 26, 2026 at 11:35:56AM +0200, Andy Shevchenko wrote:
> On Wed, Mar 25, 2026 at 03:53:19PM +0100, Johan Hovold wrote:
> > The SPI API is asymmetric and the controller is freed as part of
> > deregistration (unless it has been allocated using
> > devm_spi_alloc_host/target()).
> > 
> > A recent change converting the managed registration function to use
> > devm_add_action_or_reset() inadvertently introduced a (mostly
> > theoretical) regression where a non-devres managed controller could be
> > freed as part of failed registration. This in turn would lead to
> > use-after-free in controller driver error paths.
> > 
> > Fix this by taking another reference before calling
> > devm_add_action_or_reset() and not releasing it on errors for
> > non-devres allocated controllers.
> > 
> > An alternative would be a partial revert of the offending commit, but
> > it is better to handle this explicitly until the API has been fixed
> > (e.g. see 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> > allocation")).
> 
> Thank you!
> This is good TODO list now.
> 
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Seems Mark dropped the original change, so this is not needed.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Mar 26, 2026 at 09:19:22PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 26, 2026 at 11:35:56AM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 25, 2026 at 03:53:19PM +0100, Johan Hovold wrote:
> > > The SPI API is asymmetric and the controller is freed as part of
> > > deregistration (unless it has been allocated using
> > > devm_spi_alloc_host/target()).

...

> > > An alternative would be a partial revert of the offending commit, but
> > > it is better to handle this explicitly until the API has been fixed
> > > (e.g. see 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> > > allocation")).

> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Seems Mark dropped the original change, so this is not needed.

Not intentionally?

On Thu, Mar 26, 2026 at 07:25:14PM +0000, Mark Brown wrote:
> On Thu, Mar 26, 2026 at 09:19:22PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 26, 2026 at 11:35:56AM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 25, 2026 at 03:53:19PM +0100, Johan Hovold wrote:

...

> > > > An alternative would be a partial revert of the offending commit, but
> > > > it is better to handle this explicitly until the API has been fixed
> > > > (e.g. see 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> > > > allocation")).
> 
> > > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> > Seems Mark dropped the original change, so this is not needed.
> 
> Not intentionally?

I don't know, but it allows to start from fixing the users to avoid regressions.
I'm fine with the current state of affairs.

-- 
With Best Regards,
Andy Shevchenko

On Fri, Mar 27, 2026 at 09:20:08AM +0200, Andy Shevchenko wrote:
> On Thu, Mar 26, 2026 at 07:25:14PM +0000, Mark Brown wrote:
> > On Thu, Mar 26, 2026 at 09:19:22PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 26, 2026 at 11:35:56AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 25, 2026 at 03:53:19PM +0100, Johan Hovold wrote:
> 
> ...
> 
> > > > > An alternative would be a partial revert of the offending commit, but
> > > > > it is better to handle this explicitly until the API has been fixed
> > > > > (e.g. see 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> > > > > allocation")).
> > 
> > > > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > > Seems Mark dropped the original change, so this is not needed.
> > 
> > Not intentionally?
> 
> I don't know, but it allows to start from fixing the users to avoid regressions.
> I'm fine with the current state of affairs.

The offending commit is already in mainline and hasn't been reverted in
Mark's tree. This fix has been merged there though so nothing more is
needed for now.

Johan

On Fri, Mar 27, 2026 at 09:47:11AM +0100, Johan Hovold wrote:
> On Fri, Mar 27, 2026 at 09:20:08AM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 26, 2026 at 07:25:14PM +0000, Mark Brown wrote:
> > > On Thu, Mar 26, 2026 at 09:19:22PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Mar 26, 2026 at 11:35:56AM +0200, Andy Shevchenko wrote:
> > > > > On Wed, Mar 25, 2026 at 03:53:19PM +0100, Johan Hovold wrote:

...

> > > > > > An alternative would be a partial revert of the offending commit, but
> > > > > > it is better to handle this explicitly until the API has been fixed
> > > > > > (e.g. see 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> > > > > > allocation")).
> > > 
> > > > > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 
> > > > Seems Mark dropped the original change, so this is not needed.
> > > 
> > > Not intentionally?
> > 
> > I don't know, but it allows to start from fixing the users to avoid regressions.
> > I'm fine with the current state of affairs.
> 
> The offending commit is already in mainline and hasn't been reverted in
> Mark's tree. This fix has been merged there though so nothing more is
> needed for now.

Ah, I only looked into Linux Next (without looking into the base, which is
v7.0-rc5 as of today). Thanks for clarification.

-- 
With Best Regards,
Andy Shevchenko

On Fri, Mar 27, 2026 at 10:53:48AM +0200, Andy Shevchenko wrote:
> On Fri, Mar 27, 2026 at 09:47:11AM +0100, Johan Hovold wrote:
> > On Fri, Mar 27, 2026 at 09:20:08AM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 26, 2026 at 07:25:14PM +0000, Mark Brown wrote:

> > > > Not intentionally?

> > > I don't know, but it allows to start from fixing the users to avoid regressions.
> > > I'm fine with the current state of affairs.

> > The offending commit is already in mainline and hasn't been reverted in
> > Mark's tree. This fix has been merged there though so nothing more is
> > needed for now.

> Ah, I only looked into Linux Next (without looking into the base, which is
> v7.0-rc5 as of today). Thanks for clarification.

Oh, that's a relief!  I thought I'd managed to throw some commits away
by mistake.