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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free of `chan->dev` in `at_dma_interrupt()` due to improper cleanup ordering.
- [High] Hardware polling loops without timeouts can cause infinite kernel hangs.
--

--- Patch [1]: [PATCH v6] dma: at_hdmac: Fix use-after-free by proper tasklet  cleanup ---
commit 7d3c8db7581644f08893dedd5b27d08992a22d00
Author: Hongling Zeng <zenghongling@kylinos.cn>

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

Current cleanup paths have a use-after-free vulnerability where vchan_init()
creates tasklets that access at_dma_chan memory. The fix requires careful
ordering, such as calling free_irq() first to synchronize with running IRQ
handlers, and killing tasklets before freeing other resources.

Closes: 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..09b1aedefb458 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1940,6 +1940,20 @@ static void at_dma_off(struct at_dma *atdma)
>  		cpu_relax();

[Severity: High]
This is a pre-existing issue, but does this loop risk an infinite hang if the
hardware becomes unresponsive? 

If dma_readl(atdma, CHSR) always returns a value with atdma->all_chan_mask
bits set, the loop in at_dma_off() might never exit.

Is it possible to add a timeout or limit to prevent a potential CPU lockup
here and in similar polling loops like the one for EBCISR in at_dma_probe()?

>  }
>  
> +static void at_dma_cleanup_channels(struct at_dma *atdma)

[ ... ]

> @@ -2118,23 +2134,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);

[Severity: High]
This isn't a bug introduced by this patch, but does the current cleanup order
still leave a window for a use-after-free?

In at_dma_remove(), dma_async_device_unregister() unregisters the device and
its channels, freeing chan->dev. Because free_irq() is called after this
unregister, can a concurrent interrupt arrive, get processed by
at_dma_interrupt(), and trigger atc_handle_error() which accesses the
already freed chan->dev via chan2dev()?

Could free_irq() or disable_irq() be moved earlier in the teardown path to
safeguard against this?

>  
> -	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);
> -	}
> +	at_dma_cleanup_channels(atdma);
> +	dma_pool_destroy(atdma->memset_pool);
> +	dma_pool_destroy(atdma->lli_pool);
>  
>  	clk_disable_unprepare(atdma->clk);
>  }

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