Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] spi: Fix use-after-free with devm_spi_alloc_*
@ 2021-04-07  9:55 William A. Kennington III
  2021-04-08 13:11 ` Mark Brown
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: William A. Kennington III @ 2021-04-07  9:55 UTC (permalink / raw)
  To: broonie; +Cc: linux-spi, linux-kernel, Joel Stanley, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*
  2021-04-07  9:55 [PATCH] spi: Fix use-after-free with devm_spi_alloc_* William A. Kennington III
@ 2021-04-08 13:11 ` Mark Brown
  2021-04-08 16:54 ` Mark Brown
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Mark Brown @ 2021-04-08 13:11 UTC (permalink / raw)
  To: William A. Kennington III; +Cc: linux-spi, linux-kernel, Joel Stanley


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*
  2021-04-07  9:55 [PATCH] spi: Fix use-after-free with devm_spi_alloc_* William A. Kennington III
  2021-04-08 13:11 ` Mark Brown
@ 2021-04-08 16:54 ` Mark Brown
  2021-04-09  6:59 ` Joel Stanley
       [not found] ` <CAHp75VcRE3JOyFEMzvP9RW1du3cXx3zaTq-8KJnGt9zaJeiJZg@mail.gmail.com>
  3 siblings, 0 replies; 5+ messages in thread
From: Mark Brown @ 2021-04-08 16:54 UTC (permalink / raw)
  To: William A. Kennington III
  Cc: Mark Brown, Joel Stanley, linux-spi, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*
  2021-04-07  9:55 [PATCH] spi: Fix use-after-free with devm_spi_alloc_* William A. Kennington III
  2021-04-08 13:11 ` Mark Brown
  2021-04-08 16:54 ` Mark Brown
@ 2021-04-09  6:59 ` Joel Stanley
       [not found] ` <CAHp75VcRE3JOyFEMzvP9RW1du3cXx3zaTq-8KJnGt9zaJeiJZg@mail.gmail.com>
  3 siblings, 0 replies; 5+ messages in thread
From: Joel Stanley @ 2021-04-09  6:59 UTC (permalink / raw)
  To: William A. Kennington III
  Cc: Mark Brown, linux-spi, Linux Kernel Mailing List

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] spi: Fix use-after-free with devm_spi_alloc_*
       [not found] ` <CAHp75VcRE3JOyFEMzvP9RW1du3cXx3zaTq-8KJnGt9zaJeiJZg@mail.gmail.com>
@ 2021-04-09  9:05   ` William Kennington
  0 siblings, 0 replies; 5+ messages in thread
From: William Kennington @ 2021-04-09  9:05 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: broonie, linux-spi, linux-kernel, Joel Stanley

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-07  9:55 [PATCH] spi: Fix use-after-free with devm_spi_alloc_* William A. Kennington III
2021-04-08 13:11 ` Mark Brown
2021-04-08 16:54 ` Mark Brown
2021-04-09  6:59 ` Joel Stanley
     [not found] ` <CAHp75VcRE3JOyFEMzvP9RW1du3cXx3zaTq-8KJnGt9zaJeiJZg@mail.gmail.com>
2021-04-09  9:05   ` William Kennington

Linux-SPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-spi/0 linux-spi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-spi linux-spi/ https://lore.kernel.org/linux-spi \
		linux-spi@vger.kernel.org
	public-inbox-index linux-spi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-spi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git