Re: [PATCH] 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:
- [Critical] The patch introduces a NULL pointer dereference in the error handling paths of `at_dma_probe()` by calling `at_dma_cleanup_channels()` before the channel list is initialized.
- [High] The patch introduces a flawed teardown order by calling `tasklet_kill()` before `free_irq()`, leaving a race condition open that allows an interrupt handler to schedule a tasklet after it has been killed.
--

commit 616bebad1ba493e9e69b6cbd4fcc4afc4e20c2f9
Author: Hongling Zeng <zenghongling@kylinos.cn>

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

This commit aims to fix a use-after-free vulnerability by introducing
a cleanup function that explicitly kills tasklets during device teardown.
It modifies both the probe error path and the remove function.

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
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c

[ ... ]

> @@ -2109,6 +2122,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  err_memset_pool_create:
>  	dma_pool_destroy(atdma->lli_pool);
>  err_desc_pool_create:
> +	at_dma_cleanup_channels(atdma);
>  	free_irq(platform_get_irq(pdev, 0), atdma);

[Severity: Critical]
Does this cause a NULL pointer dereference during a probe failure?

The atdma structure is allocated earlier via devm_kzalloc(), meaning the
dma_device.channels list head is initially zeroed out.

If dma_pool_create() fails, we jump to err_desc_pool_create and call
at_dma_cleanup_channels(). However, INIT_LIST_HEAD() hasn't been called yet.

When at_dma_cleanup_channels() executes list_for_each_entry_safe() on
the uninitialized list, it will dereference the NULL pointers, resulting in
a kernel panic.

[ ... ]

> @@ -2125,17 +2139,12 @@ static void at_dma_remove(struct platform_device *pdev)
>  		of_dma_controller_free(pdev->dev.of_node);
>  	dma_async_device_unregister(&atdma->dma_device);
>  
> +	at_dma_cleanup_channels(atdma);
> +
>  	dma_pool_destroy(atdma->memset_pool);
>  	dma_pool_destroy(atdma->lli_pool);
>  	free_irq(platform_get_irq(pdev, 0), atdma);

[Severity: High]
Is it safe to place at_dma_cleanup_channels() before free_irq() here?

The commit message indicates we need to disable interrupts, kill tasklets,
and then free the IRQ. However, atc_disable_chan_irq() inside the cleanup
function only disables the hardware interrupt; it doesn't synchronize with
an already-executing IRQ handler on another CPU.

If an interrupt handler is currently running and delayed, it can schedule
a tasklet after tasklet_kill() has already returned. 

Because free_irq() is the function that actually synchronizes with running
handlers, wouldn't free_irq() need to be called before tasklet_kill() to
prevent the handler from re-queueing the tasklet and triggering the exact
use-after-free this patch is trying to fix?

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