[PATCH v2] dmaengine: moxart: Fix use-after-free by proper tasklet cleanup

[PATCH v2] dmaengine: moxart: Fix use-after-free by proper tasklet cleanup

[Hongling Zeng]

The moxart DMA driver has a use-after-free vulnerability:
- vchan_init() creates tasklets that access moxart_chan memory
- Neither probe error paths nor remove() call tasklet_kill()
- devm_free_irq() only waits for IRQ handler, NOT tasklets
- mdc is devm-managed and freed after probe/remove
- Running tasklets accessing freed memory → Use-After-Free!

Fix by adding moxart_dma_free_channels() helper that calls
tasklet_kill() for each channel, and ensuring proper teardown order:

In remove():
- devm_free_irq() FIRST to stop the IRQ handler (implies
  synchronize_irq(), preventing new tasklets from being scheduled)
- moxart_dma_free_channels() to kill already-scheduled tasklets
- Then of_dma_controller_free() and dma_async_device_unregister()
  to safely unregister the device

In probe error path:
- moxart_dma_free_channels() to kill tasklets created by vchan_init()
- devm_request_irq() is automatically released by devres, so no
  explicit devm_free_irq() is needed

Fixes: 5f9e685a0d46 ("dmaengine: Add MOXA ART DMA engine driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
 Change in v2:
 - The DMA core's dma_async_device_unregister() handles all channel
   cleanup including sysfs entries, IDA tags, and per-cpu memory, so
   we don't call list_del() on the channel nodes.
---
 drivers/dma/moxart-dma.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
index 442f5aa16031..a2481960e870 100644
--- a/drivers/dma/moxart-dma.c
+++ b/drivers/dma/moxart-dma.c
@@ -553,6 +553,21 @@ 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);
+	}
+}
+
 static int moxart_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -609,19 +624,23 @@ static int moxart_probe(struct platform_device *pdev)
 	ret = dma_async_device_register(&mdc->dma_slave);
 	if (ret) {
 		dev_err(dev, "dma_async_device_register failed\n");
-		return ret;
+		goto err_dma_register;
 	}
 
 	ret = of_dma_controller_register(node, moxart_of_xlate, mdc);
 	if (ret) {
 		dev_err(dev, "of_dma_controller_register failed\n");
 		dma_async_device_unregister(&mdc->dma_slave);
-		return ret;
+		goto err_dma_register;
 	}
 
 	dev_dbg(dev, "%s: IRQ=%u\n", __func__, irq);
 
 	return 0;
+
+err_dma_register:
+	moxart_dma_free_channels(mdc);
+	return ret;
 }
 
 static void moxart_remove(struct platform_device *pdev)
@@ -630,10 +649,12 @@ static void moxart_remove(struct platform_device *pdev)
 
 	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);
 }
 
 static const struct of_device_id moxart_dma_match[] = {
-- 
2.25.1

Re: [PATCH v2] dmaengine: moxart: Fix use-after-free by proper tasklet cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free of DMA device structure (`mdc`) on driver unbind due to `devm_kzalloc`.
- [High] Interrupt storm and system lockup due to unacknowledged pending interrupts on unallocated channels.
- [High] NULL pointer dereference due to a Time-of-Check to Time-of-Use (TOCTOU) race condition in the interrupt handler.
- [High] Hardware DMA channels are not disabled during driver removal, risking silent memory corruption.
--

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

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

The moxart DMA driver has a use-after-free vulnerability:
- vchan_init() creates tasklets that access moxart_chan memory
- Neither probe error paths nor remove() call tasklet_kill()

> diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
> index 442f5aa16031f..a2481960e8700 100644
> --- a/drivers/dma/moxart-dma.c
> +++ b/drivers/dma/moxart-dma.c
> @@ -553,6 +553,21 @@ static irqreturn_t moxart_dma_interrupt(int irq, void *devid)
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
potentially cause an interrupt storm?

If a hardware interrupt asserts on an unallocated channel, the handler skips
reading the control register and fails to clear the interrupt:

drivers/dma/moxart-dma.c:moxart_dma_interrupt() {
    ...
	for (i = 0; i < APB_DMA_MAX_CHANNEL; i++, ch++) {
		if (!ch->allocated)
			continue;
    ...
}

Since the handler unconditionally returns IRQ_HANDLED at the end, could this
cause the kernel to re-enter the interrupt handler immediately and
indefinitely?

[Severity: High]
This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use
(TOCTOU) race condition in the interrupt handler?

The descriptor is checked locklessly before acquiring the spinlock:

drivers/dma/moxart-dma.c:moxart_dma_interrupt() {
    ...
			if (ch->desc) {
				spin_lock(&ch->vc.lock);
				if (++ch->sgidx < ch->desc->sglen) {
    ...
}

If a client driver concurrently terminates the channel and sets ch->desc to
NULL, could this result in a NULL pointer dereference when accessing
ch->desc->sglen inside the locked section?

[ ... ]

> @@ -609,19 +624,23 @@ static int moxart_probe(struct platform_device *pdev)
>  	ret = dma_async_device_register(&mdc->dma_slave);
>  	if (ret) {

[Severity: High]
This is a pre-existing issue, but does using devm_kzalloc() for the DMA
device structure cause a use-after-free on driver unbind?

drivers/dma/moxart-dma.c:moxart_probe() {
    ...
	mdc = devm_kzalloc(dev, sizeof(*mdc), GFP_KERNEL);
    ...
}

If the driver is unbound via sysfs while a client holds a reference to a
DMA channel, devres will immediately free mdc.  When the client later drops
its reference, will the dmaengine core access the freed memory?

[ ... ]

> @@ -630,10 +649,12 @@ static void moxart_remove(struct platform_device *pdev)
>  {
>  	struct moxart_dmadev *m = platform_get_drvdata(pdev);
>  
>  	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);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but are the hardware DMA channels
left enabled during driver removal?

If the driver is unbound while a DMA transfer is actively running, the
software resources are freed, but the hardware control registers are never
updated to stop the transfers.

If the client driver subsequently frees its DMA buffers, could the
continuing hardware transfer corrupt memory?

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