Re: [PATCH v2 10/64] dmaengine: mmp_pdma: replace per-chan tasklet with dma_chan BH

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

Pre-existing issues:
- [High] Unconditional dereference of phy->vchan in the interrupt handler races with channel teardown.
- [High] Missing cancellation of scheduled bottom-half work during channel teardown and device removal causes UAF and NULL pointer dereferences.
- [High] Terminating cyclic transfers frees descriptors but leaves a dangling pointer in `chan->cyclic_first`, causing UAF in the bottom-half handler.
--

--- Patch [10]: [PATCH v2 10/64] dmaengine: mmp_pdma: replace per-chan tasklet with dma_chan BH ---
commit 7ee6ebc1ebbe207f60ed8c3f6afa81414e0802c3
Author: Allen Pais <[email protected]>

dmaengine: mmp_pdma: replace per-chan tasklet with dma_chan BH

This commit replaces the per-channel tasklet with the shared dma_chan BH
helper. The handler continues to run in softirq context while delegating
scheduling to the dmaengine's mechanism.

> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..3fd9ef59a1420 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
[ ... ]
> @@ -374,7 +373,7 @@ static irqreturn_t mmp_pdma_chan_handler(int irq, void *dev_id)
>  	if (clear_chan_irq(phy) != 0)
>  		return IRQ_NONE;
>  
> -	tasklet_schedule(&phy->vchan->tasklet);
> +	dma_chan_schedule_bh(&phy->vchan->chan);

[Severity: High]
This is a pre-existing issue, but does this race with channel teardown?

If mmp_pdma_free_phy() sets phy->vchan to NULL while the shared physical
interrupt remains active:

drivers/dma/mmp_pdma.c:mmp_pdma_free_phy() {
    ...
    pchan->phy->vchan = NULL;
    ...
}

Could a pending interrupt be serviced immediately afterwards?

If clear_chan_irq() successfully clears the hardware interrupt and returns
zero, it appears this code will unconditionally dereference the NULL pointer
in phy->vchan when scheduling the bottom half.

>  	return IRQ_HANDLED;
>  }
[ ... ]
> @@ -1047,13 +1046,11 @@ static void mmp_pdma_issue_pending(struct dma_chan *dchan)
>  }
>  
>  /*
> - * dma_do_tasklet
> - * Do call back
> - * Start pending list
> + * dma_do_tasklet - BH worker to process completions and kick pending work
>   */
> -static void dma_do_tasklet(struct tasklet_struct *t)
> +static void dma_do_tasklet(struct dma_chan *c)
>  {
> -	struct mmp_pdma_chan *chan = from_tasklet(chan, t, tasklet);
> +	struct mmp_pdma_chan *chan = container_of(c, struct mmp_pdma_chan, chan);

[Severity: High]
This is a pre-existing issue, but is the bottom half properly synchronized
against device removal and channel teardown?

Looking at mmp_pdma_free_chan_resources(), the descriptor pool is destroyed
and chan->desc_pool is set to NULL:

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

Since there isn't a device_synchronize() or explicit cancellation of the
pending bottom-half work during teardown or mmp_pdma_remove(), could this
background work execute asynchronously?

If it does, it looks like it would invoke dma_pool_free() on a NULL or
destroyed pool further down in this function, or access a chan structure
that was already freed by devres.

[Severity: High]
This isn't a bug introduced by this patch, but can this function access a
freed descriptor during cyclic transfers?

In mmp_pdma_terminate_all(), the driver correctly frees running descriptors
back to the DMA pool by calling mmp_pdma_free_desc_list(), but it doesn't
appear to clear chan->cyclic_first:

drivers/dma/mmp_pdma.c:mmp_pdma_terminate_all() {
    ...
    mmp_pdma_free_desc_list(chan, &chan->chain_running);
    ...
}

If a delayed or spurious bottom-half execution triggers this function,
could it evaluate chan->cyclic_first as non-null and subsequently
use-after-free the memory to invoke the callback?

>  	struct mmp_pdma_desc_sw *desc, *_desc;
>  	LIST_HEAD(chain_cleanup);
>  	unsigned long flags;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10