* [PATCH 1/3] iio: adc: xilinx: fix potential use-after-free on remove
@ 2019-03-10 18:58 ` Sven Van Asbroeck
0 siblings, 0 replies; 14+ messages in thread
From: Sven Van Asbroeck @ 2019-03-10 18:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, linux-iio, Michal Simek, linux-kernel,
Manish Narani, Peter Meerwald-Stadler, Hartmut Knaack,
linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] iio: adc: xilinx: fix potential use-after-free on probe
2019-03-10 18:58 ` Sven Van Asbroeck
@ 2019-03-10 18:58 ` Sven Van Asbroeck
-1 siblings, 0 replies; 14+ messages in thread
From: Sven Van Asbroeck @ 2019-03-10 18:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek, Manish Narani, linux-iio, linux-arm-kernel,
linux-kernel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] iio: adc: xilinx: fix potential use-after-free on probe
@ 2019-03-10 18:58 ` Sven Van Asbroeck
0 siblings, 0 replies; 14+ messages in thread
From: Sven Van Asbroeck @ 2019-03-10 18:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, linux-iio, Michal Simek, linux-kernel,
Manish Narani, Peter Meerwald-Stadler, Hartmut Knaack,
linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: adc: xilinx: fix potential use-after-free on probe
2019-03-10 18:58 ` Sven Van Asbroeck
@ 2019-03-16 15:49 ` Jonathan Cameron
-1 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2019-03-16 15:49 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek, Manish Narani, linux-iio, linux-arm-kernel,
linux-kernel
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:
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] iio: adc: xilinx: fix potential use-after-free on probe
@ 2019-03-16 15:49 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2019-03-16 15:49 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Lars-Peter Clausen, linux-iio, Michal Simek, linux-kernel,
Manish Narani, Peter Meerwald-Stadler, Hartmut Knaack,
linux-arm-kernel
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:
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove
2019-03-10 18:58 ` Sven Van Asbroeck
@ 2019-03-10 18:58 ` Sven Van Asbroeck
-1 siblings, 0 replies; 14+ messages in thread
From: Sven Van Asbroeck @ 2019-03-10 18:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek, Manish Narani, linux-iio, linux-arm-kernel,
linux-kernel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove
@ 2019-03-10 18:58 ` Sven Van Asbroeck
0 siblings, 0 replies; 14+ messages in thread
From: Sven Van Asbroeck @ 2019-03-10 18:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, linux-iio, Michal Simek, linux-kernel,
Manish Narani, Peter Meerwald-Stadler, Hartmut Knaack,
linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove
2019-03-10 18:58 ` Sven Van Asbroeck
@ 2019-03-16 15:51 ` Jonathan Cameron
-1 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2019-03-16 15:51 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek, Manish Narani, linux-iio, linux-arm-kernel,
linux-kernel
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);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove
@ 2019-03-16 15:51 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2019-03-16 15:51 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Lars-Peter Clausen, linux-iio, Michal Simek, linux-kernel,
Manish Narani, Peter Meerwald-Stadler, Hartmut Knaack,
linux-arm-kernel
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);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] iio: adc: xilinx: prevent touching unclocked h/w on remove
2019-03-16 15:51 ` Jonathan Cameron
@ 2019-03-17 16:07 ` Sven Van Asbroeck
-1 siblings, 0 replies; 14+ messages in thread
From: Sven Van Asbroeck @ 2019-03-17 16:07 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-arm-kernel, Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] iio: adc: xilinx: fix potential use-after-free on remove
2019-03-10 18:58 ` Sven Van Asbroeck
@ 2019-03-16 15:47 ` Jonathan Cameron
-1 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2019-03-16 15:47 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek, Manish Narani, linux-iio, linux-arm-kernel,
linux-kernel
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);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] iio: adc: xilinx: fix potential use-after-free on remove
@ 2019-03-16 15:47 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2019-03-16 15:47 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Lars-Peter Clausen, linux-iio, Michal Simek, linux-kernel,
Manish Narani, Peter Meerwald-Stadler, Hartmut Knaack,
linux-arm-kernel
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);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread