| Message ID | 20190310193246.31761-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: envelope-detector: fix use-after-free on device remove | expand |
On Sun, Mar 10, 2019 at 03:32:46PM -0400, Sven Van Asbroeck wrote: > This driver's remove path never explicitly cancels the > delayed work. So it is possible for the delayed work to > run after the core has freed the private structure > (struct envelope). This is a potential use-after-free. > > Fix by adding a devm_add_action callback to the remove > path, called right after iio_device_unregister(), which > explicitly cancels the delayed work. > > This issue was detected with the help of Coccinelle. > > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> > --- > drivers/iio/adc/envelope-detector.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iio/adc/envelope-detector.c b/drivers/iio/adc/envelope-detector.c > index 2f2b563c1162..2f1c78b3ff44 100644 > --- a/drivers/iio/adc/envelope-detector.c > +++ b/drivers/iio/adc/envelope-detector.c > @@ -321,6 +321,14 @@ static const struct iio_info envelope_detector_info = { > .read_raw = &envelope_detector_read_raw, > }; > > +static void envelope_detector_stop_work(void *data) > +{ > + struct iio_dev *indio_dev = data; > + struct envelope *env = iio_priv(indio_dev); > + > + cancel_delayed_work_sync(&env->comp_timeout); > +} > + > static int envelope_detector_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -395,6 +403,10 @@ static int envelope_detector_probe(struct platform_device *pdev) > return ret; > } > > + ret = devm_add_action(dev, envelope_detector_stop_work, indio_dev); > + if (ret) > + return ret; Just a random thought. Wouldn't devm_add_action_or_reset() be a better fit? In case adding action results in failure we will not get the chance to cancel work. > + > return devm_iio_device_register(dev, indio_dev); > } > > -- > 2.17.1 >
On 2019-03-11 19:00, Tomasz Duszynski wrote: > On Sun, Mar 10, 2019 at 03:32:46PM -0400, Sven Van Asbroeck wrote: >> This driver's remove path never explicitly cancels the >> delayed work. So it is possible for the delayed work to >> run after the core has freed the private structure >> (struct envelope). This is a potential use-after-free. >> >> Fix by adding a devm_add_action callback to the remove >> path, called right after iio_device_unregister(), which >> explicitly cancels the delayed work. >> >> This issue was detected with the help of Coccinelle. Hi Sven, (I didn't get the original msg for some reason, so I'm responding to this reply instead) This is false positive, AFAICT. The delayed work must have finished while envelope_detector_read_raw() holds the read_lock mutex, and it would be highly surprising if the device can go away while it is handling an IIO ->read_raw call. (THAT would be an interesting bug...) That said, I might be mistaken, but you need to explain a sequence of events that leads to the claimed dangling delayed work. I.e., how do you propose that env->done completes while there is work scheduled? Short of that, and since this is all theory without empirical evidence, this patch gets a NACK from me. Cheers, Peter >> >> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> >> --- >> drivers/iio/adc/envelope-detector.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iio/adc/envelope-detector.c b/drivers/iio/adc/envelope-detector.c >> index 2f2b563c1162..2f1c78b3ff44 100644 >> --- a/drivers/iio/adc/envelope-detector.c >> +++ b/drivers/iio/adc/envelope-detector.c >> @@ -321,6 +321,14 @@ static const struct iio_info envelope_detector_info = { >> .read_raw = &envelope_detector_read_raw, >> }; >> >> +static void envelope_detector_stop_work(void *data) >> +{ >> + struct iio_dev *indio_dev = data; >> + struct envelope *env = iio_priv(indio_dev); >> + >> + cancel_delayed_work_sync(&env->comp_timeout); >> +} >> + >> static int envelope_detector_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -395,6 +403,10 @@ static int envelope_detector_probe(struct platform_device *pdev) >> return ret; >> } >> >> + ret = devm_add_action(dev, envelope_detector_stop_work, indio_dev); >> + if (ret) >> + return ret; > > Just a random thought. Wouldn't devm_add_action_or_reset() be a better fit? > In case adding action results in failure we will not get the chance to cancel > work. > >> + >> return devm_iio_device_register(dev, indio_dev); >> } >> >> -- >> 2.17.1 >>
Hi Peter, On Mon, Mar 11, 2019 at 2:41 PM Peter Rosin <peda@axentia.se> wrote: > > This is false positive, AFAICT. The delayed work must have > finished while envelope_detector_read_raw() holds the read_lock > mutex, and it would be highly surprising if the device can go > away while it is handling an IIO ->read_raw call. (THAT would be > an interesting bug...) Quite right. I had completely overlooked the fact that wait_for_completion() cannot finish unless explicitly completed. Unlike wait_for_completion_interruptible(). So yes, this looks like a false positive. My apologies. Sven
diff --git a/drivers/iio/adc/envelope-detector.c b/drivers/iio/adc/envelope-detector.c index 2f2b563c1162..2f1c78b3ff44 100644 --- a/drivers/iio/adc/envelope-detector.c +++ b/drivers/iio/adc/envelope-detector.c @@ -321,6 +321,14 @@ static const struct iio_info envelope_detector_info = { .read_raw = &envelope_detector_read_raw, }; +static void envelope_detector_stop_work(void *data) +{ + struct iio_dev *indio_dev = data; + struct envelope *env = iio_priv(indio_dev); + + cancel_delayed_work_sync(&env->comp_timeout); +} + static int envelope_detector_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -395,6 +403,10 @@ static int envelope_detector_probe(struct platform_device *pdev) return ret; } + ret = devm_add_action(dev, envelope_detector_stop_work, indio_dev); + if (ret) + return ret; + return devm_iio_device_register(dev, indio_dev); }
This driver's remove path never explicitly cancels the delayed work. So it is possible for the delayed work to run after the core has freed the private structure (struct envelope). This is a potential use-after-free. Fix by adding a devm_add_action callback to the remove path, called right after iio_device_unregister(), which explicitly cancels the delayed work. This issue was detected with the help of Coccinelle. Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> --- drivers/iio/adc/envelope-detector.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)