Re: [PATCH v3] 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:
- [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