[PATCH v3] dma: at_hdmac: Fix use-after-free by proper tasklet cleanup

[PATCH v3] dma: at_hdmac: Fix use-after-free by proper tasklet cleanup

[Hongling Zeng]

Current cleanup paths have a use-after-free vulnerability:
- vchan_init() creates tasklets that access at_dma_chan memory
- free_irq() only waits for IRQ handler, NOT tasklets
- atdma is devm-managed and freed after probe/remove
- Running tasklets accessing freed memory → Use-After-Free!

The fix requires careful ordering:
- free_irq() FIRST to synchronize with running IRQ handlers and prevent
  them from scheduling new tasklets
- Then kill tasklets to wait for already-scheduled ones to complete
- Only then free other resources

Fixes: ac803b56860f ("dmaengine: at_hdmac: Convert driver to use virt-dma")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260604073945.54B311F00898@smtp.kernel.org/
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
Change in v2:
- Fix NULL pointer dereference in probe error path by checking if channels                                                                       list is initialized before cleanup
- Fix race condition by calling free_irq() before tasklet_kill() to ensure
  IRQ handlers cannot reschedule tasklets after cleanup
---
Change in v3:
- Reorder probe error path to match remove() cleanup order
- Keep defensive NULL check for channels cleanup
- Remove unused variables in at_dma_remove()
- Ensure consistent order: free_irq → cleanup_channels → dma_pool_destroy
---
 drivers/dma/at_hdmac.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index e5b30a57c477..ac1ba21e3428 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -1940,6 +1940,19 @@ static void at_dma_off(struct at_dma *atdma)
 		cpu_relax();
 }
 
+static void at_dma_cleanup_channels(struct at_dma *atdma)
+{
+	struct dma_chan *chan, *_chan;
+
+	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
+			device_node) {
+		/* Disable interrupts */
+		atc_disable_chan_irq(atdma, chan->chan_id);
+		tasklet_kill(&to_at_dma_chan(chan)->vc.task);
+		list_del(&chan->device_node);
+	}
+}
+
 static int __init at_dma_probe(struct platform_device *pdev)
 {
 	struct at_dma		*atdma;
@@ -2105,11 +2118,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
 err_of_dma_controller_register:
 	dma_async_device_unregister(&atdma->dma_device);
 err_dma_async_device_register:
+	free_irq(platform_get_irq(pdev, 0), atdma);
+	at_dma_cleanup_channels(atdma);
 	dma_pool_destroy(atdma->memset_pool);
 err_memset_pool_create:
 	dma_pool_destroy(atdma->lli_pool);
 err_desc_pool_create:
 	free_irq(platform_get_irq(pdev, 0), atdma);
+	if (atdma->dma_device.channels.next != NULL)
+		at_dma_cleanup_channels(atdma);
 err_irq:
 	clk_disable_unprepare(atdma->clk);
 	return err;
@@ -2118,23 +2135,17 @@ static int __init at_dma_probe(struct platform_device *pdev)
 static void at_dma_remove(struct platform_device *pdev)
 {
 	struct at_dma		*atdma = platform_get_drvdata(pdev);
-	struct dma_chan		*chan, *_chan;
 
 	at_dma_off(atdma);
 	if (pdev->dev.of_node)
 		of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&atdma->dma_device);
 
-	dma_pool_destroy(atdma->memset_pool);
-	dma_pool_destroy(atdma->lli_pool);
 	free_irq(platform_get_irq(pdev, 0), atdma);
+	at_dma_cleanup_channels(atdma);
 
-	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
-			device_node) {
-		/* Disable interrupts */
-		atc_disable_chan_irq(atdma, chan->chan_id);
-		list_del(&chan->device_node);
-	}
+	dma_pool_destroy(atdma->memset_pool);
+	dma_pool_destroy(atdma->lli_pool);
 
 	clk_disable_unprepare(atdma->clk);
 }
-- 
2.25.1

Re: [PATCH v3] dma: at_hdmac: Fix use-after-free by proper tasklet cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The probe error path executes `free_irq()` and `at_dma_cleanup_channels()` twice due to a missing return or goto statement after the `err_dma_async_device_register` label.
- [High] `at_dma_cleanup_channels()` relies on uninitialized `chan->chan_id` values on early probe error paths, causing it to fail to disable interrupts.
--

commit 393b3802c91585b2038e624d6fb8dec3e442b9db
Author: Hongling Zeng <zenghongling@kylinos.cn>

dma: at_hdmac: Fix use-after-free by proper tasklet cleanup

This commit reorders the cleanup paths in the at_hdmac driver to fix a
use-after-free vulnerability. It ensures free_irq() is called before
killing tasklets and freeing memory to prevent running tasklets from
accessing freed memory.

Link: https://lore.kernel.org/all/20260604073945.54B311F00898@smtp.kernel.org/

> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index e5b30a57c477a..ac1ba21e34287 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1940,6 +1940,19 @@ static void at_dma_off(struct at_dma *atdma)
>  		cpu_relax();
>  }
>  
> +static void at_dma_cleanup_channels(struct at_dma *atdma)
> +{
> +	struct dma_chan *chan, *_chan;
> +
> +	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
> +			device_node) {
> +		/* Disable interrupts */
> +		atc_disable_chan_irq(atdma, chan->chan_id);

[Severity: High]
Will this successfully disable interrupts for all channels if probe fails
early?

It looks like chan->chan_id is populated by the DMA engine core inside
dma_async_device_register(). If probe fails before registration is complete,
chan->chan_id will still be zero from the allocation.

This would result in repeatedly attempting to disable the interrupt for
channel 0, leaving the other channels enabled. Since the global IRQ handler
was just removed by free_irq(), could pending interrupts on these active
channels cause an unhandled interrupt storm?

> +		tasklet_kill(&to_at_dma_chan(chan)->vc.task);
> +		list_del(&chan->device_node);
> +	}
> +}

[ ... ]

> @@ -2105,11 +2118,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  err_of_dma_controller_register:
>  	dma_async_device_unregister(&atdma->dma_device);
>  err_dma_async_device_register:
> +	free_irq(platform_get_irq(pdev, 0), atdma);
> +	at_dma_cleanup_channels(atdma);

[Severity: High]
Is a goto or return statement missing here?

If execution reaches err_dma_async_device_register, it appears it will fall
through to err_desc_pool_create below, where free_irq() and
at_dma_cleanup_channels() are called a second time.

Could calling free_irq() twice for the same dev_id trigger a kernel warning
for trying to free an already-free IRQ?

>  	dma_pool_destroy(atdma->memset_pool);
>  err_memset_pool_create:
>  	dma_pool_destroy(atdma->lli_pool);
>  err_desc_pool_create:
>  	free_irq(platform_get_irq(pdev, 0), atdma);
> +	if (atdma->dma_device.channels.next != NULL)
> +		at_dma_cleanup_channels(atdma);
>  err_irq:
>  	clk_disable_unprepare(atdma->clk);
>  	return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608072635.148011-1-zenghongling@kylinos.cn?part=1