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

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

[Hongling Zeng]

When terminating a non-cyclic DMA transfer, the active descriptor
is not properly reclaimed. The descriptor is removed from the
desc_issued list in sun6i_dma_start_desc(), but in
sun6i_dma_terminate_all(), only cyclic transfer descriptors are
added to the desc_completed list before cleanup.

For non-cyclic transfers, pchan->desc is set to NULL without first
adding the descriptor back to a list that vchan_get_all_descriptors()
can collect. This causes the descriptor and its associated LLI chain
to be permanently leaked.

Fix by ensuring both cyclic and non-cyclic active descriptors are
added to the desc_completed list before setting pchan->desc to NULL.

Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Suggested-by: Frank Li <Frank.li@oss.nxp.com>

---
 Change in v2;
 -Add pchan->desc != pchan->done check to prevent race condition
  where completed descriptors could be double-added to desc_completed
  list, causing list corruption
---
 Change in v3:
 -Fix by using vchan_terminate_vdesc() as suggested by Frank Li
---
 drivers/dma/sun6i-dma.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 7a79f346250a..134ae840f176 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -946,16 +946,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;
-
-			list_add_tail(&vd->node, &vc->desc_completed);
-		}
+	if (pchan && pchan->desc && pchan->desc != pchan->done) {
+		struct virt_dma_desc *vd = &pchan->desc->vd;
+		
+		vchan_terminate_vdesc(vd);
 	}
 
+	vchan->cyclic = false;
 	vchan_get_all_descriptors(&vchan->vc, &head);
 
 	if (pchan) {
-- 
2.25.1

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] A concurrency race condition between `sun6i_dma_interrupt` and `sun6i_dma_terminate_all` leads to a NULL pointer dereference or Use-After-Free.
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race condition on `vchan->phy` in `sun6i_dma_terminate_all` leads to severe state and linked list corruption.
- [High] Use-After-Free in the `err_lli_free` error path of `sun6i_dma_prep_slave_sg` and `sun6i_dma_prep_dma_cyclic`.
- [High] Deadlock due to acquiring `sdev->lock` without disabling softirqs in process context.
--

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

    dmaengine: sun6i-dma: Fix memory leak in sun6i_dma_terminate_all
    
    When terminating a non-cyclic DMA transfer, the active descriptor
    is not properly reclaimed because only cyclic transfer descriptors
    are added to the desc_completed list before cleanup. This causes
    the descriptor and its associated LLI chain to be permanently leaked.
    This fix ensures both cyclic and non-cyclic active descriptors are
    added to the list before setting pchan->desc to NULL.

> 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)

[Severity: High]
This is a pre-existing issue, but can acquiring sdev->lock cause a deadlock
here?

Earlier in this function:

	spin_lock(&sdev->lock);
	list_del_init(&vchan->node);
	spin_unlock(&sdev->lock);

Because spin_lock() does not disable softirqs, could a hardware interrupt
occur while this lock is held, schedule sun6i_dma_tasklet(), and cause the
tasklet to spin forever on sdev->lock in softirq context on the same CPU?
This exact same locking pattern also appears in sun6i_dma_pause().


[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race on vchan->phy?

At the beginning of sun6i_dma_terminate_all(), pchan is read locklessly:

	struct sun6i_pchan *pchan = vchan->phy;

Could sun6i_dma_tasklet() free this physical channel and reallocate it to a
different virtual channel before we acquire the lock? If so, does mutating
pchan->desc and calling vchan_terminate_vdesc(vd) below corrupt the
descriptor lists of an unrelated channel?

>  
>  	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);
>  	}
>  
> +	vchan->cyclic = false;
>  	vchan_get_all_descriptors(&vchan->vc, &head);
>  
>  	if (pchan) {

[Severity: Critical]
This is a pre-existing issue, but does sun6i_dma_terminate_all() race with
sun6i_dma_interrupt()?

Later in this function, we set pchan->desc = NULL under vchan->vc.lock:

	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 currently executing on another CPU, it evaluates
pchan->desc->vd either locklessly (for cyclic) or blocks on vchan->vc.lock
(for non-cyclic):

	if (vchan->cyclic) {
		vchan_cyclic_callback(&pchan->desc->vd);
	} else {
		spin_lock(&vchan->vc.lock);
		vchan_cookie_complete(&pchan->desc->vd);
		...

Once sun6i_dma_terminate_all() drops the lock, sun6i_dma_interrupt() will
dereference the invalidated pointer. Can this cause a NULL pointer dereference
or a Use-After-Free?


[Severity: High]
This isn't a bug introduced by this patch, but is there a Use-After-Free in
the error path of sun6i_dma_prep_slave_sg() and sun6i_dma_prep_dma_cyclic()?

In the err_lli_free label of both functions:

err_lli_free:
	for (p_lli = txd->p_lli, v_lli = txd->v_lli; v_lli;
	     p_lli = v_lli->p_lli_next, v_lli = v_lli->v_lli_next)
		dma_pool_free(sdev->pool, v_lli, p_lli);

dma_pool_free() frees v_lli, but the loop update expression subsequently
evaluates v_lli = v_lli->v_lli_next. Does this dereference the freed memory?

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