[v2] dmaengine: sun6i-dma: Fix use-after-free in error handling paths

In error handling paths, the for loop frees v_lli in the loop body,
then accesses v_lli->v_lli_next and v_lli->p_lli_next in the
increment expression, which is use-after-free.

Fix by saving both the next virtual and physical pointers before
freeing the current node.

Fixes: 555859308723 ("dmaengine: Add driver for Allwinner sun6i DMA")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
Suggested-by: Jernej Skrabec <jernej.skrabec@gmail.com>

---
Changes in v2:
 -Refactored the fix to avoid code duplication by creating a helper function
  sun6i_dma_free_lli_list() that handles LLI list cleanup
 -Add Suggested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/dma/sun6i-dma.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)
  

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

Pre-existing issues:
- [High] Missing validation for `sg_len == 0` in `sun6i_dma_prep_slave_sg()` allows the creation of an empty descriptor, leading to a NULL pointer dereference or wild DMA execution.
- [High] Missing parameter validation in `sun6i_dma_prep_dma_cyclic()` allows division-by-zero exceptions and NULL pointer dereferences.
- [High] Unprotected access to `pchan->desc` in the interrupt handler can race with `sun6i_dma_terminate_all()`, causing a NULL pointer dereference.
- [High] Process-context use of plain `spin_lock()` on a lock shared with a tasklet causes softirq preemption deadlocks.
- [Medium] Hardware race in `sun6i_get_chan_size()` inflates residue reports due to unverified sequential register reads.
--

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

dmaengine: sun6i-dma: Fix use-after-free in error handling paths

This commit addresses a use-after-free bug in the error handling paths of
sun6i_dma_prep_slave_sg() and sun6i_dma_prep_dma_cyclic(). The variables
v_lli and p_lli were accessed in the loop increment expression after the
loop body freed them. A new helper function, sun6i_dma_free_lli_list(), is
introduced to safely traverse and free the list.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..02dd07001d5a9 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c

[ ... ]

> @@ -788,9 +794,7 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
>  	return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
>  
>  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);
> +	sun6i_dma_free_lli_list(sdev, txd);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem earlier
in sun6i_dma_prep_slave_sg(). If the function is called with sg_len
equal to 0, the for_each_sg() loop is bypassed entirely:

	for_each_sg(sgl, sg, sg_len, i) {

This allows the creation of an empty descriptor where txd->v_lli is NULL
and txd->p_lli is 0. Does this lead to a NULL pointer dereference or wild
DMA execution when the DMA engine starts the transaction via
sun6i_dma_start_desc()?

drivers/dma/sun6i-dma.c:sun6i_dma_start_desc() {
	writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR);
	writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE);
}

Additionally, if dynamic debug is enabled, would sun6i_dma_dump_lli()
unconditionally dereference the NULL v_lli pointer before the function
returns?

>  	kfree(txd);
>  	return NULL;
>  }
> @@ -869,9 +873,7 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
>  	return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
>  
>  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);
> +	sun6i_dma_free_lli_list(sdev, txd);

[Severity: High]
This is a pre-existing issue, but I spotted a potential problem earlier in
sun6i_dma_prep_dma_cyclic() where we calculate periods:

	unsigned int i, periods = buf_len / period_len;

Is validation missing for period_len here, potentially causing a
division-by-zero exception if it is 0?

Additionally, if buf_len < period_len, the calculated periods is 0,
which bypasses the allocation loop. This leaves the prev pointer
initialized as NULL, which then causes a NULL pointer dereference
here:

	prev->p_lli_next = txd->p_lli;		/* cyclic list */

>  	kfree(txd);
>  	return NULL;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed unprotected access to
pchan->desc in the interrupt handler which might race with
sun6i_dma_terminate_all():

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
			pchan = sdev->pchans + j;
			vchan = pchan->vchan;
			if (vchan && (status & vchan->irq_type)) {
				if (vchan->cyclic) {
					vchan_cyclic_callback(&pchan->desc->vd);
				} else {
					spin_lock(&vchan->vc.lock);
					vchan_cookie_complete(&pchan->desc->vd);
}

Could the lockless read of vchan followed by dereferencing pchan->desc
allow a NULL pointer dereference if sun6i_dma_terminate_all()
concurrently runs on another CPU?

drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
		vchan->phy = NULL;
		pchan->vchan = NULL;
		pchan->desc = NULL;
}

[Severity: High]
This is a pre-existing issue, but process-context use of plain spin_lock()
on a lock shared with a tasklet seems to cause softirq preemption deadlocks.

drivers/dma/sun6i-dma.c:sun6i_dma_pause() {
		spin_lock(&sdev->lock);
		list_del_init(&vchan->node);
		spin_unlock(&sdev->lock);
}

Since sun6i_dma_tasklet() runs in softirq context and acquires sdev->lock
using spin_lock_irq():

drivers/dma/sun6i-dma.c:sun6i_dma_tasklet() {
	spin_lock_irq(&sdev->lock);
}

If a hardware interrupt fires while the lock is held in process context,
would the interrupt exit path execute pending softirqs on the same CPU,
causing the tasklet to attempt to acquire sdev->lock and spin indefinitely?

[Severity: Medium]
This is a pre-existing issue, but unverified sequential register reads
in sun6i_get_chan_size() might create a race condition.

drivers/dma/sun6i-dma.c:sun6i_get_chan_size() {
	pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
	bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
}

If the DMA hardware advances to the next Linked List Item (LLI) exactly
between these reads, can pos hold the old LLI address but bytes hold the
new LLI's remaining byte count? This might double-count the new LLI's bytes
when iterating through the software list, reporting an artificially inflated
residue.
  

Dne ponedeljek, 15. junij 2026 ob 04:23:59 Srednjeevropski poletni čas je Hongling Zeng napisal(a):
> In error handling paths, the for loop frees v_lli in the loop body,
> then accesses v_lli->v_lli_next and v_lli->p_lli_next in the
> increment expression, which is use-after-free.
> 
> Fix by saving both the next virtual and physical pointers before
> freeing the current node.
> 
> Fixes: 555859308723 ("dmaengine: Add driver for Allwinner sun6i DMA")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> Suggested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> 
> ---
> Changes in v2:
>  -Refactored the fix to avoid code duplication by creating a helper function
>   sun6i_dma_free_lli_list() that handles LLI list cleanup
>  -Add Suggested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/dma/sun6i-dma.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb..02dd07001d5a 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -406,16 +406,12 @@ static inline void sun6i_dma_dump_lli(struct sun6i_vchan *vchan,
>  		v_lli->len, v_lli->para, v_lli->p_lli_next);
>  }
>  
> -static void sun6i_dma_free_desc(struct virt_dma_desc *vd)
> +static void sun6i_dma_free_lli_list(struct sun6i_dma_dev *sdev,
> +					struct sun6i_desc *txd)
>  {
> -	struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
> -	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(vd->tx.chan->device);
>  	struct sun6i_dma_lli *v_lli, *v_next;
>  	dma_addr_t p_lli, p_next;
>  
> -	if (unlikely(!txd))
> -		return;
> -
>  	p_lli = txd->p_lli;
>  	v_lli = txd->v_lli;
>  
> @@ -428,7 +424,17 @@ static void sun6i_dma_free_desc(struct virt_dma_desc *vd)
>  		v_lli = v_next;
>  		p_lli = p_next;
>  	}
> +}
>  
> +static void sun6i_dma_free_desc(struct virt_dma_desc *vd)
> +{
> +	struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
> +	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(vd->tx.chan->device);
> +
> +	if (unlikely(!txd))
> +		return;
> +
> +	sun6i_dma_free_lli_list(sdev, txd);
>  	kfree(txd);

Why not also move txd handling? That way even more code can be put in one place.

Best regards,
Jernej

>  }
>  
> @@ -788,9 +794,7 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
>  	return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
>  
>  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);
> +	sun6i_dma_free_lli_list(sdev, txd);
>  	kfree(txd);
>  	return NULL;
>  }
> @@ -869,9 +873,7 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
>  	return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
>  
>  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);
> +	sun6i_dma_free_lli_list(sdev, txd);
>  	kfree(txd);
>  	return NULL;
>  }
>