| Message ID | 20190306174559.17362-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: proximity: as3935: fix use-after-free on device remove | expand |
On Wed, 6 Mar 2019 12:45:59 -0500 Sven Van Asbroeck <thesven73@gmail.com> wrote: > This driver's probe() uses a mix of devm_ and non-devm_ functions. This > means that the remove order will not be the exact opposite of the probe > order. > > Remove order: > 1. remove() executes: > iio_device_unregister > iio_triggered_buffer_cleanup > iio_trigger_unregister > (A) > 2. core frees devm resources in reverse order: > free_irq > iio_trigger_free > iio_device_free > > In (A) the trigger has been unregistered, but the irq handler is still > registered and active, so the trigger may still be touched via > interrupt -> as3935_event_work. This is a potential use-after-unregister. > > Given that the delayed work is never canceled explicitly, it may run even > after iio_device_free. This is a potential use-after-free. > > Solution: convert all probe functions to their devm_ equivalents. > Add a devm callback, called by the core on remove right after irq_free, > which explicitly cancels the delayed work. This will guarantee that all > resources are freed in the correct order. > > As an added bonus, some boilerplate code can be removed. > > While we're here, remove redundant &'s in front of function names when > passing a pointer-to-function. > > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> Hi Sven Your description makes it clear that there are multiple things in the patch. Don't do a 'while we were here' in a patch doing something else please. Separate patches. Content looks good. Jonathan > --- > drivers/iio/proximity/as3935.c | 53 ++++++++++++++-------------------- > 1 file changed, 22 insertions(+), 31 deletions(-) > > diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c > index f130388a16a0..e33334ea2830 100644 > --- a/drivers/iio/proximity/as3935.c > +++ b/drivers/iio/proximity/as3935.c > @@ -213,7 +213,7 @@ static int as3935_read_raw(struct iio_dev *indio_dev, > > static const struct iio_info as3935_info = { > .attrs = &as3935_attribute_group, > - .read_raw = &as3935_read_raw, > + .read_raw = as3935_read_raw, > }; > > static irqreturn_t as3935_trigger_handler(int irq, void *private) > @@ -345,6 +345,14 @@ static SIMPLE_DEV_PM_OPS(as3935_pm_ops, as3935_suspend, as3935_resume); > #define AS3935_PM_OPS NULL > #endif > > +static void as3935_stop_work(void *data) > +{ > + struct iio_dev *indio_dev = data; > + struct as3935_state *st = iio_priv(indio_dev); > + > + cancel_delayed_work_sync(&st->work); > +} > + > static int as3935_probe(struct spi_device *spi) > { > struct iio_dev *indio_dev; > @@ -368,7 +376,6 @@ static int as3935_probe(struct spi_device *spi) > > spi_set_drvdata(spi, indio_dev); > mutex_init(&st->lock); > - INIT_DELAYED_WORK(&st->work, as3935_event_work); > > ret = of_property_read_u32(np, > "ams,tuning-capacitor-pf", &st->tune_cap); > @@ -414,59 +421,44 @@ static int as3935_probe(struct spi_device *spi) > iio_trigger_set_drvdata(trig, indio_dev); > trig->ops = &iio_interrupt_trigger_ops; > > - ret = iio_trigger_register(trig); > + ret = devm_iio_trigger_register(&spi->dev, trig); > if (ret) { > dev_err(&spi->dev, "failed to register trigger\n"); > return ret; > } > > - ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time, > - &as3935_trigger_handler, NULL); > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, > + iio_pollfunc_store_time, as3935_trigger_handler, NULL); > > if (ret) { > dev_err(&spi->dev, "cannot setup iio trigger\n"); > - goto unregister_trigger; > + return ret; > } > > calibrate_as3935(st); > > + INIT_DELAYED_WORK(&st->work, as3935_event_work); > + ret = devm_add_action(&spi->dev, as3935_stop_work, indio_dev); > + if (ret) > + return ret; > + > ret = devm_request_irq(&spi->dev, spi->irq, > - &as3935_interrupt_handler, > + as3935_interrupt_handler, > IRQF_TRIGGER_RISING, > dev_name(&spi->dev), > indio_dev); > > if (ret) { > dev_err(&spi->dev, "unable to request irq\n"); > - goto unregister_buffer; > + return ret; > } > > - ret = iio_device_register(indio_dev); > + ret = devm_iio_device_register(&spi->dev, indio_dev); > if (ret < 0) { > dev_err(&spi->dev, "unable to register device\n"); > - goto unregister_buffer; > + return ret; > } > return 0; > - > -unregister_buffer: > - iio_triggered_buffer_cleanup(indio_dev); > - > -unregister_trigger: > - iio_trigger_unregister(st->trig); > - > - return ret; > -} > - > -static int as3935_remove(struct spi_device *spi) > -{ > - struct iio_dev *indio_dev = spi_get_drvdata(spi); > - struct as3935_state *st = iio_priv(indio_dev); > - > - iio_device_unregister(indio_dev); > - iio_triggered_buffer_cleanup(indio_dev); > - iio_trigger_unregister(st->trig); > - > - return 0; > } > > static const struct of_device_id as3935_of_match[] = { > @@ -488,7 +480,6 @@ static struct spi_driver as3935_driver = { > .pm = AS3935_PM_OPS, > }, > .probe = as3935_probe, > - .remove = as3935_remove, > .id_table = as3935_id, > }; > module_spi_driver(as3935_driver);
On Fri, Mar 8, 2019 at 12:13 PM Jonathan Cameron <jonathan.cameron@huawei.com> wrote: > > Your description makes it clear that there are multiple things in the patch. > Don't do a 'while we were here' in a patch doing something else please. > Separate patches. > The proposed solution (adding devm_) fixes multiple issues, so that can stay as a single patch, right? It's the 'while we're here' you'd like to see removed? Can do.
On Fri, 8 Mar 2019 12:49:30 -0500 Sven Van Asbroeck <thesven73@gmail.com> wrote: > On Fri, Mar 8, 2019 at 12:13 PM Jonathan Cameron > <jonathan.cameron@huawei.com> wrote: > > > > Your description makes it clear that there are multiple things in the patch. > > Don't do a 'while we were here' in a patch doing something else please. > > Separate patches. > > > > The proposed solution (adding devm_) fixes multiple issues, so that can stay > as a single patch, right? > > It's the 'while we're here' you'd like to see removed? Can do. That's the one. Just the & removal. The rest makes sense in one patch as you say. Jonathan
diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c index f130388a16a0..e33334ea2830 100644 --- a/drivers/iio/proximity/as3935.c +++ b/drivers/iio/proximity/as3935.c @@ -213,7 +213,7 @@ static int as3935_read_raw(struct iio_dev *indio_dev, static const struct iio_info as3935_info = { .attrs = &as3935_attribute_group, - .read_raw = &as3935_read_raw, + .read_raw = as3935_read_raw, }; static irqreturn_t as3935_trigger_handler(int irq, void *private) @@ -345,6 +345,14 @@ static SIMPLE_DEV_PM_OPS(as3935_pm_ops, as3935_suspend, as3935_resume); #define AS3935_PM_OPS NULL #endif +static void as3935_stop_work(void *data) +{ + struct iio_dev *indio_dev = data; + struct as3935_state *st = iio_priv(indio_dev); + + cancel_delayed_work_sync(&st->work); +} + static int as3935_probe(struct spi_device *spi) { struct iio_dev *indio_dev; @@ -368,7 +376,6 @@ static int as3935_probe(struct spi_device *spi) spi_set_drvdata(spi, indio_dev); mutex_init(&st->lock); - INIT_DELAYED_WORK(&st->work, as3935_event_work); ret = of_property_read_u32(np, "ams,tuning-capacitor-pf", &st->tune_cap); @@ -414,59 +421,44 @@ static int as3935_probe(struct spi_device *spi) iio_trigger_set_drvdata(trig, indio_dev); trig->ops = &iio_interrupt_trigger_ops; - ret = iio_trigger_register(trig); + ret = devm_iio_trigger_register(&spi->dev, trig); if (ret) { dev_err(&spi->dev, "failed to register trigger\n"); return ret; } - ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time, - &as3935_trigger_handler, NULL); + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, + iio_pollfunc_store_time, as3935_trigger_handler, NULL); if (ret) { dev_err(&spi->dev, "cannot setup iio trigger\n"); - goto unregister_trigger; + return ret; } calibrate_as3935(st); + INIT_DELAYED_WORK(&st->work, as3935_event_work); + ret = devm_add_action(&spi->dev, as3935_stop_work, indio_dev); + if (ret) + return ret; + ret = devm_request_irq(&spi->dev, spi->irq, - &as3935_interrupt_handler, + as3935_interrupt_handler, IRQF_TRIGGER_RISING, dev_name(&spi->dev), indio_dev); if (ret) { dev_err(&spi->dev, "unable to request irq\n"); - goto unregister_buffer; + return ret; } - ret = iio_device_register(indio_dev); + ret = devm_iio_device_register(&spi->dev, indio_dev); if (ret < 0) { dev_err(&spi->dev, "unable to register device\n"); - goto unregister_buffer; + return ret; } return 0; - -unregister_buffer: - iio_triggered_buffer_cleanup(indio_dev); - -unregister_trigger: - iio_trigger_unregister(st->trig); - - return ret; -} - -static int as3935_remove(struct spi_device *spi) -{ - struct iio_dev *indio_dev = spi_get_drvdata(spi); - struct as3935_state *st = iio_priv(indio_dev); - - iio_device_unregister(indio_dev); - iio_triggered_buffer_cleanup(indio_dev); - iio_trigger_unregister(st->trig); - - return 0; } static const struct of_device_id as3935_of_match[] = { @@ -488,7 +480,6 @@ static struct spi_driver as3935_driver = { .pm = AS3935_PM_OPS, }, .probe = as3935_probe, - .remove = as3935_remove, .id_table = as3935_id, }; module_spi_driver(as3935_driver);
This driver's probe() uses a mix of devm_ and non-devm_ functions. This means that the remove order will not be the exact opposite of the probe order. Remove order: 1. remove() executes: iio_device_unregister iio_triggered_buffer_cleanup iio_trigger_unregister (A) 2. core frees devm resources in reverse order: free_irq iio_trigger_free iio_device_free In (A) the trigger has been unregistered, but the irq handler is still registered and active, so the trigger may still be touched via interrupt -> as3935_event_work. This is a potential use-after-unregister. Given that the delayed work is never canceled explicitly, it may run even after iio_device_free. This is a potential use-after-free. Solution: convert all probe functions to their devm_ equivalents. Add a devm callback, called by the core on remove right after irq_free, which explicitly cancels the delayed work. This will guarantee that all resources are freed in the correct order. As an added bonus, some boilerplate code can be removed. While we're here, remove redundant &'s in front of function names when passing a pointer-to-function. Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> --- drivers/iio/proximity/as3935.c | 53 ++++++++++++++-------------------- 1 file changed, 22 insertions(+), 31 deletions(-)