[PATCH] dmaengine: mmp_pdma: Check virtual channel before scheduling tasklet

[PATCH] dmaengine: mmp_pdma: Check virtual channel before scheduling tasklet

[Ruoyu Wang]

mmp_pdma_chan_handler() clears a physical-channel interrupt and then
unconditionally schedules phy->vchan->tasklet. The physical channel can
be detached from its virtual channel when the channel is terminated or
when no pending work remains, so a late or shared interrupt can reach the
handler with phy->vchan already NULL.

Snapshot phy->vchan in the interrupt path, skip tasklet scheduling when
there is no virtual channel, and use the same snapshot for the BUSERR
warning. Use WRITE_ONCE() for the matching attach/detach stores because
the IRQ path reads this pointer without taking phy_lock.

This issue was found by a static analysis checker and confirmed by
manual source review.

Fixes: c8acd6aa6bed ("dmaengine: mmp-pdma support")
Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 drivers/dma/mmp_pdma.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 386e85cd4882a..6f379e9f10017 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -351,6 +351,7 @@ static void disable_chan(struct mmp_pdma_phy *phy)
 
 static int clear_chan_irq(struct mmp_pdma_phy *phy)
 {
+	struct mmp_pdma_chan *vchan;
 	u32 dcsr;
 	u32 dint = readl(phy->base + DINT);
 	u32 reg = (phy->idx << 2) + DCSR;
@@ -361,8 +362,9 @@ static int clear_chan_irq(struct mmp_pdma_phy *phy)
 	/* clear irq */
 	dcsr = readl(phy->base + reg);
 	writel(dcsr, phy->base + reg);
-	if ((dcsr & DCSR_BUSERR) && (phy->vchan))
-		dev_warn(phy->vchan->dev, "DCSR_BUSERR\n");
+	vchan = READ_ONCE(phy->vchan);
+	if ((dcsr & DCSR_BUSERR) && vchan)
+		dev_warn(vchan->dev, "DCSR_BUSERR\n");
 
 	return 0;
 }
@@ -370,11 +372,16 @@ static int clear_chan_irq(struct mmp_pdma_phy *phy)
 static irqreturn_t mmp_pdma_chan_handler(int irq, void *dev_id)
 {
 	struct mmp_pdma_phy *phy = dev_id;
+	struct mmp_pdma_chan *vchan;
 
 	if (clear_chan_irq(phy) != 0)
 		return IRQ_NONE;
 
-	tasklet_schedule(&phy->vchan->tasklet);
+	vchan = READ_ONCE(phy->vchan);
+	if (!vchan)
+		return IRQ_HANDLED;
+
+	tasklet_schedule(&vchan->tasklet);
 	return IRQ_HANDLED;
 }
 
@@ -427,7 +434,7 @@ static struct mmp_pdma_phy *lookup_phy(struct mmp_pdma_chan *pchan)
 				continue;
 			phy = &pdev->phy[i];
 			if (!phy->vchan) {
-				phy->vchan = pchan;
+				WRITE_ONCE(phy->vchan, pchan);
 				found = phy;
 				goto out_unlock;
 			}
@@ -453,7 +460,7 @@ static void mmp_pdma_free_phy(struct mmp_pdma_chan *pchan)
 	writel(0, pchan->phy->base + reg);
 
 	spin_lock_irqsave(&pdev->phy_lock, flags);
-	pchan->phy->vchan = NULL;
+	WRITE_ONCE(pchan->phy->vchan, NULL);
 	pchan->phy = NULL;
 	spin_unlock_irqrestore(&pdev->phy_lock, flags);
 }
-- 
2.51.0

Re: [PATCH] dmaengine: mmp_pdma: Check virtual channel before scheduling tasklet

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch leaves a Time-Of-Check to Time-Of-Use (TOCTOU) race open by taking two separate snapshots of `phy->vchan`.
- [High] Time-of-check to time-of-use (TOCTOU) races on `chan->phy` can lead to NULL pointer dereferences when racing with channel termination.
- [High] Missing `tasklet_kill()` during channel teardown and driver removal leads to use-after-free vulnerabilities.
- [High] A use-after-free write occurs on the error paths of the `prep_*` functions when freeing the descriptor list.
- [High] The DMA channel hangs indefinitely if multiple transactions are submitted before `issue_pending` is called.
- [Medium] Stale hardware interrupts are not reliably cleared when a physical channel is disabled, risking state corruption upon reallocation.
--

--- Patch [1]: [PATCH] dmaengine: mmp_pdma: Check virtual channel before scheduling tasklet ---
commit 4969ecc3a7a7a671c042835c34adbdf0229a2d70
Author: Ruoyu Wang <ruoyuw560@gmail.com>

dmaengine: mmp_pdma: Check virtual channel before scheduling tasklet

mmp_pdma_chan_handler() clears a physical-channel interrupt and then
unconditionally schedules phy->vchan->tasklet. The physical channel can
be detached from its virtual channel when the channel is terminated or
when no pending work remains, so a late or shared interrupt can reach the
handler with phy->vchan already NULL.

Snapshot phy->vchan in the interrupt path, skip tasklet scheduling when
there is no virtual channel, and use the same snapshot for the BUSERR
warning. Use WRITE_ONCE() for the matching attach/detach stores because
the IRQ path reads this pointer without taking phy_lock.

Fixes: c8acd6aa6bed ("dmaengine: mmp-pdma support")
Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>

> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..6f379e9f10017 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c

[ ... ]

> @@ -370,11 +372,16 @@ static int clear_chan_irq(struct mmp_pdma_phy *phy)
>  static irqreturn_t mmp_pdma_chan_handler(int irq, void *dev_id)
>  {
>  	struct mmp_pdma_phy *phy = dev_id;
> +	struct mmp_pdma_chan *vchan;
>  
>  	if (clear_chan_irq(phy) != 0)
>  		return IRQ_NONE;
>  
> -	tasklet_schedule(&phy->vchan->tasklet);
> +	vchan = READ_ONCE(phy->vchan);

[Severity: High]
This isn't a bug introduced by this patch, but does taking two separate
snapshots of phy->vchan leave a time-of-check to time-of-use race open?

The commit message mentions using the same snapshot, but the implementation
takes one snapshot in clear_chan_irq() and a second one here in
mmp_pdma_chan_handler(). If an interrupt fires for an old virtual channel
exactly as the physical channel is reassigned to a new one, the two
READ_ONCE(phy->vchan) calls could fetch different virtual channels.

The interrupt would be cleared for the old channel, but the tasklet would be
scheduled for the new channel, prematurely completing its pending descriptors.

Could this be fixed by returning the vchan pointer from clear_chan_irq()
instead of reading it twice?

> +	if (!vchan)
> +		return IRQ_HANDLED;
> +
> +	tasklet_schedule(&vchan->tasklet);
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
due to a time-of-check to time-of-use race in mmp_pdma_residue()?

In mmp_pdma_residue(), chan->phy is checked and used locklessly:

drivers/dma/mmp_pdma.c:mmp_pdma_residue() {
    ...
    if (!chan->phy)
        return 0;

    if (chan->dir == DMA_DEV_TO_MEM)
        curr = pdev->ops->read_dst_addr(chan->phy);
    ...
}

Concurrently, mmp_pdma_terminate_all() can call mmp_pdma_free_phy(), which
sets chan->phy = NULL. Because chan->phy is accessed without READ_ONCE() or
locks, the compiler might reload the variable from memory between the check
and its use, resulting in a NULL dereference.

[Severity: High]
This is a pre-existing issue, but does the channel teardown path miss a
tasklet_kill() call, potentially leading to a use-after-free?

In mmp_pdma_free_chan_resources(), the descriptor pool is destroyed without
synchronizing or killing active tasklets:

drivers/dma/mmp_pdma.c:mmp_pdma_free_chan_resources() {
    ...
    dma_pool_destroy(chan->desc_pool);
    chan->desc_pool = NULL;
    ...
}

Similarly, in mmp_pdma_remove(), the driver unregisters the device and allows
devres to free chan memory without first killing the tasklet. If a tasklet is
scheduled just before the DMA channel is freed, dma_do_tasklet() could run
concurrently with or after teardown, causing it to dereference chan after it
is freed or operate on chan->desc_pool after it is destroyed.

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in the
prep_* functions cause a use-after-free write when freeing the descriptor list?

In mmp_pdma_prep_memcpy(), newly allocated descriptors are appended using
list_add_tail(&new->node, &first->tx_list). If an allocation fails, the error
path passes the dynamically embedded list head to the cleanup function:

drivers/dma/mmp_pdma.c:mmp_pdma_prep_memcpy() {
    ...
fail:
    if (first)
        mmp_pdma_free_desc_list(chan, &first->tx_list);
    ...
}

During cleanup in mmp_pdma_free_desc_list():

drivers/dma/mmp_pdma.c:mmp_pdma_free_desc_list() {
    ...
    list_for_each_entry_safe(desc, _desc, list, node) {
        list_del(&desc->node);
        dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
    }
    ...
}

Since first->tx_list is embedded in first, and first is freed during the
first iteration, subsequent iterations perform list_del(&desc->node). This
modifies the list head inside the already freed first descriptor, corrupting
the memory of the DMA descriptor pool.

[Severity: High]
This is a pre-existing issue, but does the DMA channel hang indefinitely if
multiple transactions are submitted before issue_pending() is called?

In start_pending_queue(), all pending transactions are moved to chain_running,
but only the physical address of the first descriptor is programmed to the
hardware:

drivers/dma/mmp_pdma.c:start_pending_queue() {
    ...
    list_splice_tail_init(&chan->chain_pending, &chan->chain_running);

    /*
     * Program the descriptor's address into the DMA controller,
     * then start the DMA transaction
     */
    pdev->ops->write_next_addr(chan->phy, desc->async_tx.phys);
    ...
}

Since descriptors from different transactions are not linked, the controller
will stop after the first transaction. The tasklet in dma_do_tasklet() sees
that chain_running is not empty and leaves chan->idle = false:

drivers/dma/mmp_pdma.c:dma_do_tasklet() {
    ...
    chan->idle = list_empty(&chan->chain_running);

    /* Start any pending transactions automatically */
    start_pending_queue(chan);
    ...
}

However, start_pending_queue() returns immediately if chan->idle is false,
meaning the remaining transactions in chain_running are never started.

[Severity: Medium]
This isn't a bug introduced by this patch, but can stale hardware interrupts be
incompletely cleared when a physical channel is disabled or enabled?

In disable_chan(), the read-modify-write sequence might miss W1C
(write-1-to-clear) interrupt bits that become asserted during the window:

drivers/dma/mmp_pdma.c:disable_chan() {
    ...
    dcsr = readl(phy->base + reg);

    if (phy->vchan) {
        struct mmp_pdma_device *pdev;

        pdev = to_mmp_pdma_dev(phy->vchan->chan.device);
        writel(dcsr & ~pdev->ops->run_bits, phy->base + reg);
    ...
}

If a hardware interrupt fires between readl() and writel(), the snapshot's
W1C interrupt bit is 0, so writel() writes 0, failing to clear the pending
interrupt. 

Similarly, enable_chan() performs an unlocked read-modify-write on a W1C
register, blindly clearing any pending W1C bits without processing them. When
the physical channel is later reassigned, this stale interrupt can trigger
the IRQ handler for the new channel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707150356.2257833-1-ruoyuw560@gmail.com?part=1