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: <20190311180042.GA17880@arch>
Date:   Mon, 11 Mar 2019 19:00:44 +0100
From:   Tomasz Duszynski <tduszyns@...il.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Peter Rosin <peda@...ntia.se>, 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-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: envelope-detector: fix use-after-free on device
 remove

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