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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe70de0a-9134-8942-c729-e014807d101b@axentia.se>
Date:   Mon, 11 Mar 2019 18:41:51 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     Tomasz Duszynski <tduszyns@...il.com>,
        Sven Van Asbroeck <thesven73@...il.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: envelope-detector: fix use-after-free on device
 remove

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@...il.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
>>