Re: [PATCH] iio: envelope-detector: fix use-after-free on device remove

[Peter Rosin]

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@xxxxxxxxx>
>> ---
>>  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
>>