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

New issues:
- [High] Calling list_del() on channel nodes in moxart_dma_free_channels() causes dma_async_device_unregister() to silently skip channel cleanup, permanently leaking sysfs entries, IDA tags, and per-cpu memory.

Pre-existing issues:
- [High] The DMA hardware is not halted during device removal, which can lead to memory corruption.
--

--- Patch [1]: [PATCH] dmaengine: moxart: Fix use-after-free by proper tasklet cleanup ---
commit 17dd1a2edfd46732b7bad62b5a03240b9b705283
Author: Hongling Zeng <zenghongling@kylinos.cn>

dmaengine: moxart: Fix use-after-free by proper tasklet cleanup

This commit attempts to fix a use-after-free vulnerability in the moxart
DMA driver by explicitly killing tasklets before freeing the memory. It
introduces a helper to stop tasklets and reorganizes the remove and probe
error paths.

> diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
> index 442f5aa16031f..d01eaa6b964bc 100644
> --- a/drivers/dma/moxart-dma.c
> +++ b/drivers/dma/moxart-dma.c
> @@ -553,6 +553,22 @@ static irqreturn_t moxart_dma_interrupt(int irq, void *devid)
>  	return IRQ_HANDLED;
>  }
>  
> +static void moxart_dma_free_channels(struct moxart_dmadev *mdc)
> +{
> +	struct moxart_chan *ch;
> +	int i;
> +
> +	for (i = 0; i < APB_DMA_MAX_CHANNEL; i++) {
> +		ch = &mdc->slave_chans[i];
> +		/*
> +		 * Wait for any scheduled tasklet to complete before channel
> +		 * memory is freed by devres.
> +		 */
> +		tasklet_kill(&ch->vc.task);
> +		list_del(&ch->vc.chan.device_node);

[Severity: High]
Does this code skip channel cleanup in the core DMA engine?

By calling list_del() on the channel nodes here, the channels are removed
from the DMA controller's device->channels list. When
dma_async_device_unregister() is called later in moxart_remove(), it
iterates over an empty list and silently skips calling
__dma_async_device_channel_unregister().

Won't this permanently leak sysfs entries, IDA tags, and per-cpu memory
for every DMA channel?

> +	}
> +}
> +

[ ... ]

> @@ -627,10 +647,12 @@ static int moxart_probe(struct platform_device *pdev)
>  static void moxart_remove(struct platform_device *pdev)
>  {
>  	struct moxart_dmadev *m = platform_get_drvdata(pdev);

[Severity: High]
This is a pre-existing issue, but does this remove sequence leave the DMA
hardware running?

The moxart_remove() callback does not appear to explicitly stop ongoing
hardware transfers before tearing down the driver. If a transfer is active
when the driver is unbound, the IRQ is freed and the client driver might
time out and free its DMA buffer.

Without explicitly clearing the enable bits (like APB_DMA_ENABLE) in the
hardware control registers, won't the runaway DMA hardware continue writing
asynchronously and potentially corrupt memory?

>  
>  	devm_free_irq(&pdev->dev, m->irq, m);
>  
> -	dma_async_device_unregister(&m->dma_slave);
> +	moxart_dma_free_channels(m);
>  
>  	if (pdev->dev.of_node)
>  		of_dma_controller_free(pdev->dev.of_node);
> +
> +	dma_async_device_unregister(&m->dma_slave);
>  }

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