[PATCH] spi: Fix use-after-free with devm_spi_alloc_*

[PATCH] spi: Fix use-after-free with devm_spi_alloc_*

[William A. Kennington III]

We can't rely on the contents of the devres list during
spi_unregister_controller(), as the list is already torn down at the
time we perform devres_find() for devm_spi_release_controller. This
causes devices registered with devm_spi_alloc_{master,slave}() to be
mistakenly identified as legacy, non-devm managed devices and have their
reference counters decremented below 0.

------------[ cut here ]------------
WARNING: CPU: 1 PID: 660 at lib/refcount.c:28 refcount_warn_saturate+0x108/0x174
[<b0396f04>] (refcount_warn_saturate) from [<b03c56a4>] (kobject_put+0x90/0x98)
[<b03c5614>] (kobject_put) from [<b0447b4c>] (put_device+0x20/0x24)
 r4:b6700140
[<b0447b2c>] (put_device) from [<b07515e8>] (devm_spi_release_controller+0x3c/0x40)
[<b07515ac>] (devm_spi_release_controller) from [<b045343c>] (release_nodes+0x84/0xc4)
 r5:b6700180 r4:b6700100
[<b04533b8>] (release_nodes) from [<b0454160>] (devres_release_all+0x5c/0x60)
 r8:b1638c54 r7:b117ad94 r6:b1638c10 r5:b117ad94 r4:b163dc10
[<b0454104>] (devres_release_all) from [<b044e41c>] (__device_release_driver+0x144/0x1ec)
 r5:b117ad94 r4:b163dc10
[<b044e2d8>] (__device_release_driver) from [<b044f70c>] (device_driver_detach+0x84/0xa0)
 r9:00000000 r8:00000000 r7:b117ad94 r6:b163dc54 r5:b1638c10 r4:b163dc10
[<b044f688>] (device_driver_detach) from [<b044d274>] (unbind_store+0xe4/0xf8)

Instead, determine the devm allocation state as a flag on the
controller which is guaranteed to be stable during cleanup.

Fixes: 5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation")
Signed-off-by: William A. Kennington III <wak@google.com>
---
 drivers/spi/spi.c       | 9 ++-------
 include/linux/spi/spi.h | 3 +++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b08efe88ccd6..904a353798b6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2496,6 +2496,7 @@ struct spi_controller *__devm_spi_alloc_controller(struct device *dev,
 
 	ctlr = __spi_alloc_controller(dev, size, slave);
 	if (ctlr) {
+		ctlr->devm_allocated = true;
 		*ptr = ctlr;
 		devres_add(dev, ptr);
 	} else {
@@ -2842,11 +2843,6 @@ int devm_spi_register_controller(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_spi_register_controller);
 
-static int devm_spi_match_controller(struct device *dev, void *res, void *ctlr)
-{
-	return *(struct spi_controller **)res == ctlr;
-}
-
 static int __unregister(struct device *dev, void *null)
 {
 	spi_unregister_device(to_spi_device(dev));
@@ -2893,8 +2889,7 @@ void spi_unregister_controller(struct spi_controller *ctlr)
 	/* Release the last reference on the controller if its driver
 	 * has not yet been converted to devm_spi_alloc_master/slave().
 	 */
-	if (!devres_find(ctlr->dev.parent, devm_spi_release_controller,
-			 devm_spi_match_controller, ctlr))
+	if (!ctlr->devm_allocated)
 		put_device(&ctlr->dev);
 
 	/* free bus id */
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 592897fa4f03..643139b1eafe 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -510,6 +510,9 @@ struct spi_controller {
 
 #define SPI_MASTER_GPIO_SS		BIT(5)	/* GPIO CS must select slave */
 
+	/* flag indicating this is a non-devres managed controller */
+	bool			devm_allocated;
+
 	/* flag indicating this is an SPI slave controller */
 	bool			slave;
 
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*

[Mark Brown]

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

On Wed, Apr 07, 2021 at 02:55:27AM -0700, William A. Kennington III wrote:

> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 660 at lib/refcount.c:28 refcount_warn_saturate+0x108/0x174
> [<b0396f04>] (refcount_warn_saturate) from [<b03c56a4>] (kobject_put+0x90/0x98)
> [<b03c5614>] (kobject_put) from [<b0447b4c>] (put_device+0x20/0x24)
>  r4:b6700140

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative (it often is
for search engines if nothing else) then it's usually better to pull out
the relevant sections.

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

Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*

[Mark Brown]

On Wed, 7 Apr 2021 02:55:27 -0700, William A. Kennington III wrote:
> We can't rely on the contents of the devres list during
> spi_unregister_controller(), as the list is already torn down at the
> time we perform devres_find() for devm_spi_release_controller. This
> causes devices registered with devm_spi_alloc_{master,slave}() to be
> mistakenly identified as legacy, non-devm managed devices and have their
> reference counters decremented below 0.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: Fix use-after-free with devm_spi_alloc_*
      commit: 794aaf01444d4e765e2b067cba01cc69c1c68ed9

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*

[Joel Stanley]

On Wed, 7 Apr 2021 at 09:55, William A. Kennington III <wak@google.com> wrote:
>
> We can't rely on the contents of the devres list during
> spi_unregister_controller(), as the list is already torn down at the
> time we perform devres_find() for devm_spi_release_controller. This
> causes devices registered with devm_spi_alloc_{master,slave}() to be
> mistakenly identified as legacy, non-devm managed devices and have their
> reference counters decremented below 0.

Thanks for spending the time to track down the bug and sending a fix
for it. I appreciate it!

Reviewed-by: Joel Stnaley <joel@jms.id.au>

Cheers,

Joel

>
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 660 at lib/refcount.c:28 refcount_warn_saturate+0x108/0x174
> [<b0396f04>] (refcount_warn_saturate) from [<b03c56a4>] (kobject_put+0x90/0x98)
> [<b03c5614>] (kobject_put) from [<b0447b4c>] (put_device+0x20/0x24)
>  r4:b6700140
> [<b0447b2c>] (put_device) from [<b07515e8>] (devm_spi_release_controller+0x3c/0x40)
> [<b07515ac>] (devm_spi_release_controller) from [<b045343c>] (release_nodes+0x84/0xc4)
>  r5:b6700180 r4:b6700100
> [<b04533b8>] (release_nodes) from [<b0454160>] (devres_release_all+0x5c/0x60)
>  r8:b1638c54 r7:b117ad94 r6:b1638c10 r5:b117ad94 r4:b163dc10
> [<b0454104>] (devres_release_all) from [<b044e41c>] (__device_release_driver+0x144/0x1ec)
>  r5:b117ad94 r4:b163dc10
> [<b044e2d8>] (__device_release_driver) from [<b044f70c>] (device_driver_detach+0x84/0xa0)
>  r9:00000000 r8:00000000 r7:b117ad94 r6:b163dc54 r5:b1638c10 r4:b163dc10
> [<b044f688>] (device_driver_detach) from [<b044d274>] (unbind_store+0xe4/0xf8)
>
> Instead, determine the devm allocation state as a flag on the
> controller which is guaranteed to be stable during cleanup.
>
> Fixes: 5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation")
> Signed-off-by: William A. Kennington III <wak@google.com>
> ---
>  drivers/spi/spi.c       | 9 ++-------
>  include/linux/spi/spi.h | 3 +++
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index b08efe88ccd6..904a353798b6 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2496,6 +2496,7 @@ struct spi_controller *__devm_spi_alloc_controller(struct device *dev,
>
>         ctlr = __spi_alloc_controller(dev, size, slave);
>         if (ctlr) {
> +               ctlr->devm_allocated = true;
>                 *ptr = ctlr;
>                 devres_add(dev, ptr);
>         } else {
> @@ -2842,11 +2843,6 @@ int devm_spi_register_controller(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_register_controller);
>
> -static int devm_spi_match_controller(struct device *dev, void *res, void *ctlr)
> -{
> -       return *(struct spi_controller **)res == ctlr;
> -}
> -
>  static int __unregister(struct device *dev, void *null)
>  {
>         spi_unregister_device(to_spi_device(dev));
> @@ -2893,8 +2889,7 @@ void spi_unregister_controller(struct spi_controller *ctlr)
>         /* Release the last reference on the controller if its driver
>          * has not yet been converted to devm_spi_alloc_master/slave().
>          */
> -       if (!devres_find(ctlr->dev.parent, devm_spi_release_controller,
> -                        devm_spi_match_controller, ctlr))
> +       if (!ctlr->devm_allocated)
>                 put_device(&ctlr->dev);
>
>         /* free bus id */
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 592897fa4f03..643139b1eafe 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -510,6 +510,9 @@ struct spi_controller {
>
>  #define SPI_MASTER_GPIO_SS             BIT(5)  /* GPIO CS must select slave */
>
> +       /* flag indicating this is a non-devres managed controller */
> +       bool                    devm_allocated;
> +
>         /* flag indicating this is an SPI slave controller */
>         bool                    slave;
>
> --
> 2.31.0.208.g409f899ff0-goog
>

Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*

[William Kennington]

For the header file comment? I think it's actually backwards since
`devm_allocated = true` would indicate the device is managed with
devres. Should I send a followup change or v2?


On Fri, Apr 9, 2021 at 12:20 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
>
> On Wednesday, April 7, 2021, William A. Kennington III <wak@google.com> wrote:
>>
>> We can't rely on the contents of the devres list during
>> spi_unregister_controller(), as the list is already torn down at the
>> time we perform devres_find() for devm_spi_release_controller. This
>> causes devices registered with devm_spi_alloc_{master,slave}() to be
>> mistakenly identified as legacy, non-devm managed devices and have their
>> reference counters decremented below 0.
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 660 at lib/refcount.c:28 refcount_warn_saturate+0x108/0x174
>> [<b0396f04>] (refcount_warn_saturate) from [<b03c56a4>] (kobject_put+0x90/0x98)
>> [<b03c5614>] (kobject_put) from [<b0447b4c>] (put_device+0x20/0x24)
>>  r4:b6700140
>> [<b0447b2c>] (put_device) from [<b07515e8>] (devm_spi_release_controller+0x3c/0x40)
>> [<b07515ac>] (devm_spi_release_controller) from [<b045343c>] (release_nodes+0x84/0xc4)
>>  r5:b6700180 r4:b6700100
>> [<b04533b8>] (release_nodes) from [<b0454160>] (devres_release_all+0x5c/0x60)
>>  r8:b1638c54 r7:b117ad94 r6:b1638c10 r5:b117ad94 r4:b163dc10
>> [<b0454104>] (devres_release_all) from [<b044e41c>] (__device_release_driver+0x144/0x1ec)
>>  r5:b117ad94 r4:b163dc10
>> [<b044e2d8>] (__device_release_driver) from [<b044f70c>] (device_driver_detach+0x84/0xa0)
>>  r9:00000000 r8:00000000 r7:b117ad94 r6:b163dc54 r5:b1638c10 r4:b163dc10
>> [<b044f688>] (device_driver_detach) from [<b044d274>] (unbind_store+0xe4/0xf8)
>>
>> Instead, determine the devm allocation state as a flag on the
>> controller which is guaranteed to be stable during cleanup.
>>
>> Fixes: 5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation")
>> Signed-off-by: William A. Kennington III <wak@google.com>
>> ---
>>  drivers/spi/spi.c       | 9 ++-------
>>  include/linux/spi/spi.h | 3 +++
>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index b08efe88ccd6..904a353798b6 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -2496,6 +2496,7 @@ struct spi_controller *__devm_spi_alloc_controller(struct device *dev,
>>
>>         ctlr = __spi_alloc_controller(dev, size, slave);
>>         if (ctlr) {
>> +               ctlr->devm_allocated = true;
>>                 *ptr = ctlr;
>>                 devres_add(dev, ptr);
>>         } else {
>> @@ -2842,11 +2843,6 @@ int devm_spi_register_controller(struct device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(devm_spi_register_controller);
>>
>> -static int devm_spi_match_controller(struct device *dev, void *res, void *ctlr)
>> -{
>> -       return *(struct spi_controller **)res == ctlr;
>> -}
>> -
>>  static int __unregister(struct device *dev, void *null)
>>  {
>>         spi_unregister_device(to_spi_device(dev));
>> @@ -2893,8 +2889,7 @@ void spi_unregister_controller(struct spi_controller *ctlr)
>>         /* Release the last reference on the controller if its driver
>>          * has not yet been converted to devm_spi_alloc_master/slave().
>>          */
>> -       if (!devres_find(ctlr->dev.parent, devm_spi_release_controller,
>> -                        devm_spi_match_controller, ctlr))
>> +       if (!ctlr->devm_allocated)
>>                 put_device(&ctlr->dev);
>>
>>         /* free bus id */
>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>> index 592897fa4f03..643139b1eafe 100644
>> --- a/include/linux/spi/spi.h
>> +++ b/include/linux/spi/spi.h
>> @@ -510,6 +510,9 @@ struct spi_controller {
>>
>>  #define SPI_MASTER_GPIO_SS             BIT(5)  /* GPIO CS must select slave */
>>
>> +       /* flag indicating this is a non-devres managed controller */
>
>
>
> Isn’t “non-“ part confusing a lot?
>
>>
>> +       bool                    devm_allocated;
>> +
>>         /* flag indicating this is an SPI slave controller */
>>         bool                    slave;
>>
>> --
>> 2.31.0.208.g409f899ff0-goog
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>