Re: [PATCH v4] dmaengine: sun6i-dma: Fix memory leak in sun6i_dma_terminate_all

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless read of `vchan->phy` causes a Time-of-Check to Time-of-Use (TOCTOU) race, leading to cross-channel list corruption and improper hardware state modifications.
- [High] Lack of synchronization between `sun6i_dma_terminate_all` and the hardirq handler `sun6i_dma_interrupt` leads to an unmitigated NULL pointer dereference.
--

commit 0f302155373f5ca26acb89959d317a9f6c68ecc0
Author: Hongling Zeng <zenghongling@kylinos.cn>

dmaengine: sun6i-dma: Fix memory leak in sun6i_dma_terminate_all

This commit fixes a memory leak when terminating DMA transfers by using
vchan_terminate_vdesc() to handle both cyclic and non-cyclic descriptors.
It also prevents completed descriptors from being double-added to the list.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..f47a326dd7ffa 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -945,16 +945,13 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>  
> -	if (vchan->cyclic) {
> -		vchan->cyclic = false;
> -		if (pchan && pchan->desc) {
> -			struct virt_dma_desc *vd = &pchan->desc->vd;
> -			struct virt_dma_chan *vc = &vchan->vc;
> +	if (pchan && pchan->desc && pchan->desc != pchan->done) {
> +		struct virt_dma_desc *vd = &pchan->desc->vd;
>  
> -			list_add_tail(&vd->node, &vc->desc_completed);
> -		}
> +		vchan_terminate_vdesc(vd);

[Severity: High]
This is a pre-existing issue, but by calling vchan_terminate_vdesc() here,
does this code now trigger list corruption or a lockdep splat?

Earlier in sun6i_dma_terminate_all(), the physical channel is read locklessly:

    struct sun6i_pchan *pchan = vchan->phy;

If a concurrent DMA tasklet frees and reassigns this physical channel to a
completely different virtual channel before spin_lock_irqsave() is acquired,
we evaluate a stale pchan. Passing this foreign descriptor into
vchan_terminate_vdesc() will attempt to add it to desc_terminated while
holding the wrong lock, corrupting the list.

>  	}
>  
> +	vchan->cyclic = false;
>  	vchan_get_all_descriptors(&vchan->vc, &head);
>  
>  	if (pchan) {

[Severity: High]
This is a pre-existing issue, but does clearing the physical channel state
here lead to a NULL pointer dereference in the interrupt handler?

Just below this hunk in sun6i_dma_terminate_all(), the code sets
pchan->desc = NULL without synchronizing with the IRQ handler:

    if (pchan) {
        writel(DMA_CHAN_ENABLE_STOP, pchan->base + DMA_CHAN_ENABLE);
        writel(DMA_CHAN_PAUSE_RESUME, pchan->base + DMA_CHAN_PAUSE);
        vchan->phy = NULL;
        pchan->vchan = NULL;
        pchan->desc = NULL;
        pchan->done = NULL;
    }

If sun6i_dma_interrupt() is running concurrently on another CPU, it
could be executing vchan_cookie_complete(&pchan->desc->vd) or
vchan_cyclic_callback(&pchan->desc->vd). Without synchronize_irq()
or similar to wait for pending handlers, this will pass a NULL offset
to those helpers, which then dereference vd->tx.chan.

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