| Message ID | 20210407095527.2771582-1-wak@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 794aaf01444d4e765e2b067cba01cc69c1c68ed9 |
| Headers | show |
| Series | spi: Fix use-after-free with devm_spi_alloc_* | expand |
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.
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
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 >
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 > >
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;
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(-)