[PATCH 1/3] iio: adc: xilinx: fix potential use-after-free on remove

[PATCH 1/3] iio: adc: xilinx: fix potential use-after-free on remove

[Sven Van Asbroeck]

When cancel_delayed_work() returns, the delayed work may still
be running. This means that the core could potentially free
the private structure (struct xadc) while the delayed work
is still using it. This is a potential use-after-free.

Fix by calling cancel_delayed_work_sync(), which waits for
any residual work to finish before returning.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 3f6be5ac049a..1960694e8007 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1320,7 +1320,7 @@ static int xadc_remove(struct platform_device *pdev)
 	}
 	free_irq(xadc->irq, indio_dev);
 	clk_disable_unprepare(xadc->clk);
-	cancel_delayed_work(&xadc->zynq_unmask_work);
+	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
 	kfree(xadc->data);
 	kfree(indio_dev->channels);
 
-- 
2.17.1

[PATCH 2/3] iio: adc: xilinx: fix potential use-after-free on probe

[Sven Van Asbroeck]

If probe errors out after request_irq(), its error path
does not explicitly cancel the delayed work, which may
have been scheduled by the interrupt handler.

This means the delayed work may still be running when
the core frees the private structure (struct xadc).
This is a potential use-after-free.

Fix by inserting cancel_delayed_work_sync() in the probe
error path.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 1960694e8007..15e1a103f37d 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1290,6 +1290,7 @@ static int xadc_probe(struct platform_device *pdev)
 
 err_free_irq:
 	free_irq(xadc->irq, indio_dev);
+	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
 err_clk_disable_unprepare:
 	clk_disable_unprepare(xadc->clk);
 err_free_samplerate_trigger:
-- 
2.17.1

[PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove

[Sven Van Asbroeck]

In remove, the clock is disabled before canceling the
delayed work. This means that the delayed work may be
touching unclocked hardware.

Fix by disabling the clock after the delayed work is
fully canceled. This is consistent with the probe error
path order.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 15e1a103f37d..1ae86e7359f7 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1320,8 +1320,8 @@ static int xadc_remove(struct platform_device *pdev)
 		iio_triggered_buffer_cleanup(indio_dev);
 	}
 	free_irq(xadc->irq, indio_dev);
-	clk_disable_unprepare(xadc->clk);
 	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
+	clk_disable_unprepare(xadc->clk);
 	kfree(xadc->data);
 	kfree(indio_dev->channels);
 
-- 
2.17.1

Re: [PATCH 1/3] iio: adc: xilinx: fix potential use-after-free on remove

[Jonathan Cameron]

On Sun, 10 Mar 2019 14:58:24 -0400
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> When cancel_delayed_work() returns, the delayed work may still
> be running. This means that the core could potentially free
> the private structure (struct xadc) while the delayed work
> is still using it. This is a potential use-after-free.
> 
> Fix by calling cancel_delayed_work_sync(), which waits for
> any residual work to finish before returning.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
This appears 'obviously correct' to me so I'll apply it to the fixes
togreg branch of iio.git and mark it for stable.

However I won't be sending a pull request for that branch until at
least next weekend, so if anyone more familiar with the hardware
has a chance to take a look that would be great.

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 3f6be5ac049a..1960694e8007 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1320,7 +1320,7 @@ static int xadc_remove(struct platform_device *pdev)
>  	}
>  	free_irq(xadc->irq, indio_dev);
>  	clk_disable_unprepare(xadc->clk);
> -	cancel_delayed_work(&xadc->zynq_unmask_work);
> +	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
>  	kfree(xadc->data);
>  	kfree(indio_dev->channels);
>  

Re: [PATCH 2/3] iio: adc: xilinx: fix potential use-after-free on probe

[Jonathan Cameron]

On Sun, 10 Mar 2019 14:58:25 -0400
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> If probe errors out after request_irq(), its error path
> does not explicitly cancel the delayed work, which may
> have been scheduled by the interrupt handler.
> 
> This means the delayed work may still be running when
> the core frees the private structure (struct xadc).
> This is a potential use-after-free.
> 
> Fix by inserting cancel_delayed_work_sync() in the probe
> error path.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
Again, seems fine to me.

Applied to the fixes-togreg branch of iio.git.

thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 1960694e8007..15e1a103f37d 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1290,6 +1290,7 @@ static int xadc_probe(struct platform_device *pdev)
>  
>  err_free_irq:
>  	free_irq(xadc->irq, indio_dev);
> +	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
>  err_clk_disable_unprepare:
>  	clk_disable_unprepare(xadc->clk);
>  err_free_samplerate_trigger:

Re: [PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove

[Jonathan Cameron]

On Sun, 10 Mar 2019 14:58:26 -0400
Sven Van Asbroeck <thesven73@gmail.com> wrote:

> In remove, the clock is disabled before canceling the
> delayed work. This means that the delayed work may be
> touching unclocked hardware.
> 
> Fix by disabling the clock after the delayed work is
> fully canceled. This is consistent with the probe error
> path order.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 15e1a103f37d..1ae86e7359f7 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1320,8 +1320,8 @@ static int xadc_remove(struct platform_device *pdev)
>  		iio_triggered_buffer_cleanup(indio_dev);
>  	}
>  	free_irq(xadc->irq, indio_dev);
> -	clk_disable_unprepare(xadc->clk);
>  	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> +	clk_disable_unprepare(xadc->clk);
>  	kfree(xadc->data);
>  	kfree(indio_dev->channels);
>  

Re: [PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove

[Sven Van Asbroeck]

On Sat, Mar 16, 2019 at 11:51 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Applied to the fixes-togreg branch of iio.git.
>

Thank you kindly for taking this patch set, Jonathan.