| Message ID | 20190308175935.21904-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: proximity: as3935: fix use-after-free on device remove | expand |
On Fri, Mar 08, 2019 at 12:59:35PM -0500, Sven Van Asbroeck 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. > > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> > --- > drivers/iio/proximity/as3935.c | 49 ++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 29 deletions(-) > > diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c > index f130388a16a0..6e366e772164 100644 > --- a/drivers/iio/proximity/as3935.c > +++ b/drivers/iio/proximity/as3935.c > @@ -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); Any specific reason for moving this elsewhere? > > ret = of_property_read_u32(np, > "ams,tuning-capacitor-pf", &st->tune_cap); > @@ -414,22 +421,27 @@ 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); You can fix arguments alignment while you are at it. > > 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, > IRQF_TRIGGER_RISING, > @@ -438,35 +450,15 @@ static int as3935_probe(struct spi_device *spi) > > 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); > -- > 2.17.1 >
On Fri, Mar 8, 2019 at 3:30 PM Tomasz Duszynski <tduszyns@gmail.com> wrote: > > > @@ -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); > > Any specific reason for moving this elsewhere? Yes. On the remove path, cancel_delayed_work_sync() should execute after free_irq(), but before triggered_buffer_cleanup(). So the devm_add_action() must run right before devm_request_irq(). I figured it would make sense to group the devm_add_action() and INIT_WORK() together, as they are related. This also makes it easier to understand the probe/remove order when reading the code. > > > > ret = of_property_read_u32(np, > > "ams,tuning-capacitor-pf", &st->tune_cap); > > @@ -414,22 +421,27 @@ 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); > > You can fix arguments alignment while you are at it. > What type of alignment would you prefer? This? ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, iio_pollfunc_store_time, as3935_trigger_handler, NULL);
On Fri, Mar 08, 2019 at 03:42:04PM -0500, Sven Van Asbroeck wrote: > On Fri, Mar 8, 2019 at 3:30 PM Tomasz Duszynski <tduszyns@gmail.com> wrote: > > > > > @@ -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); > > > > Any specific reason for moving this elsewhere? > > Yes. On the remove path, cancel_delayed_work_sync() should execute after > free_irq(), but before triggered_buffer_cleanup(). So the devm_add_action() > must run right before devm_request_irq(). I figured it would make sense to > group the devm_add_action() and INIT_WORK() together, as they are > related. This also makes it easier to understand the probe/remove order > when reading the code. > So perhaps that change deserves a separate patch because it smells like a code cleanup. > > > > > > ret = of_property_read_u32(np, > > > "ams,tuning-capacitor-pf", &st->tune_cap); > > > @@ -414,22 +421,27 @@ 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); > > > > You can fix arguments alignment while you are at it. > > > > What type of alignment would you prefer? This? > > ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, > iio_pollfunc_store_time, > as3935_trigger_handler, NULL); Yes, this is what I've been thinking about. Thanks.
On Fri, Mar 8, 2019 at 5:33 PM Tomasz Duszynski <tduszyns@gmail.com> wrote: > > > So perhaps that change deserves a separate patch because it smells like > a code cleanup. Not really. This patch fixes the order in which resources are torn down on release(). You do this by fixing the order by which devm-resources are allocated on probe(). So it's a fundamental use-after-free fix, and not really a cleanup. But I'm happy to split into multiple patches if you want ?
On Fri, Mar 08, 2019 at 05:46:24PM -0500, Sven Van Asbroeck wrote: > On Fri, Mar 8, 2019 at 5:33 PM Tomasz Duszynski <tduszyns@gmail.com> wrote: > > > > > > So perhaps that change deserves a separate patch because it smells like > > a code cleanup. > > Not really. This patch fixes the order in which resources are torn > down on release(). What I meant is that delayed work initialization could stay where it was. The reason you moved it is readability improvement so to me it is more of a cleanup hence I suggested a separate patch. Up to you. > You do this by fixing the order by which devm-resources are allocated > on probe(). > > So it's a fundamental use-after-free fix, and not really a cleanup. > But I'm happy to split into multiple patches if you want ?
On Sat, Mar 9, 2019 at 5:24 AM Tomasz Duszynski <tduszyns@gmail.com> wrote: > > What I meant is that delayed work initialization could stay where it was. > The reason you moved it is readability improvement so to me it is more > of a cleanup hence I suggested a separate patch. Up to you. Ah, I see what you mean now. I'm ok with splitting this up, but we should probably ask the maintainer (Jonathan) what he prefers, because he's the one who will have to take two small patches instead of one. And the second patch is a one-liner.
On Sat, 9 Mar 2019 08:03:44 -0500 Sven Van Asbroeck <thesven73@gmail.com> wrote: > On Sat, Mar 9, 2019 at 5:24 AM Tomasz Duszynski <tduszyns@gmail.com> wrote: > > > > What I meant is that delayed work initialization could stay where it was. > > The reason you moved it is readability improvement so to me it is more > > of a cleanup hence I suggested a separate patch. Up to you. > > Ah, I see what you mean now. > > I'm ok with splitting this up, but we should probably ask the maintainer > (Jonathan) what he prefers, because he's the one who will have to take > two small patches instead of one. And the second patch is a one-liner. I don't mind either way (definitely a marginal case!) but as we have this one and the alignment tidy up is trivial... Applied with the alignment tweaked to the togreg branch of iio.git and pushed out as testing. I'm not taking this the fast way or proposing it for stable as whilst the race is there, I don't think anyone has actually managed to trigger it. Thanks, Jonathan
diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c index f130388a16a0..6e366e772164 100644 --- a/drivers/iio/proximity/as3935.c +++ b/drivers/iio/proximity/as3935.c @@ -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,22 +421,27 @@ 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, IRQF_TRIGGER_RISING, @@ -438,35 +450,15 @@ static int as3935_probe(struct spi_device *spi) 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. Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> --- drivers/iio/proximity/as3935.c | 49 ++++++++++++++-------------------- 1 file changed, 20 insertions(+), 29 deletions(-)