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 <[email protected]>
---
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;
+
return devm_iio_device_register(dev, indio_dev);
}
--
2.17.1
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]> 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