Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 11 Mar 2019 19:00:44 +0100
From:   Tomasz Duszynski <tduszyns@gmail.com>
To:     Sven Van Asbroeck <thesven73@gmail.com>
Cc:     Peter Rosin <peda@axentia.se>, Jonathan Cameron <jic23@kernel.org>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: envelope-detector: fix use-after-free on device
 remove
Message-ID: <20190311180042.GA17880@arch>
References: <20190310193246.31761-1-TheSven73@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20190310193246.31761-1-TheSven73@gmail.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>