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

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

[Hongling Zeng]

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 refactoring the cleanup into a helper function sun6i_dma_free_desc()
that saves both the next virtual and physical pointers before freeing the
current node, preventing the use-after-free.

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>
Reviewed-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>
---
Change in v3:
 -Further refactoring to move txd handling into the helper function
  as suggested by Jernej
---
Change in v4:
 -Add reviewed-by
---
Change in v5:
 -Correct the commit message.
---
 drivers/dma/sun6i-dma.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index fcc88a74d821..e161fc298b86 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_desc(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;
 
@@ -432,6 +428,17 @@ static void sun6i_dma_free_desc(struct virt_dma_desc *vd)
 	kfree(txd);
 }
 
+static void sun6i_dma_free_desc_virt(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_desc(sdev, txd);
+}
+
 static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
 {
 	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(vchan->vc.chan.device);
@@ -788,10 +795,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);
-	kfree(txd);
+	sun6i_dma_free_desc(sdev, txd);
 	return NULL;
 }
 
@@ -869,10 +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);
-	kfree(txd);
+	sun6i_dma_free_desc(sdev, txd);
 	return NULL;
 }
 
@@ -1432,7 +1433,7 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 		struct sun6i_vchan *vchan = &sdc->vchans[i];
 
 		INIT_LIST_HEAD(&vchan->node);
-		vchan->vc.desc_free = sun6i_dma_free_desc;
+		vchan->vc.desc_free = sun6i_dma_free_desc_virt;
 		vchan_init(&vchan->vc, &sdc->slave);
 	}
 
-- 
2.25.1

Re: [PATCH v5] dmaengine: sun6i-dma: Fix use-after-free in error handling paths

[sashiko-bot]

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

Pre-existing issues:
- [High] Deadlock in sun6i_dma_terminate_all and sun6i_dma_pause due to unsafe lock context.
- [High] Race condition in sun6i_dma_terminate_all, sun6i_dma_pause, and sun6i_dma_resume due to lockless read of vchan->phy, causing memory corruption and state inconsistency.
- [High] Race condition causing a NULL pointer dereference in sun6i_dma_interrupt.
--

--- Patch [1]: [PATCH v5] dmaengine: sun6i-dma: Fix use-after-free in error handling paths ---
commit 4eb50f2f9528d3f1d66f48feef49a56e2cf57490
Author: Hongling Zeng <zenghongling@kylinos.cn>

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 refactoring the cleanup into a helper function sun6i_dma_free_desc()
that saves both the next virtual and physical pointers before freeing the
current node, preventing the use-after-free.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..7a79f346250a5 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_desc(struct sun6i_dma_dev *sdev,
> +				struct sun6i_desc *txd)
>  {

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this cause a deadlock?

In sun6i_dma_terminate_all() and sun6i_dma_pause(), we acquire the lock
without disabling softirqs:

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

If a hardware interrupt for the DMA controller fires on the same CPU while
this lock is held, the hardirq handler will execute and schedule
sun6i_dma_tasklet. Upon return from the hardware interrupt, softirqs are
processed immediately on the same CPU, forcing the tasklet to execute.

The tasklet unconditionally attempts to acquire sdev->lock using
spin_lock_irq():

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

Because the lock is already held by the interrupted process context on the
same CPU, won't the tasklet spin forever and deadlock the CPU? Should these
use spin_lock_bh() or spin_lock_irqsave()?

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless read of
vchan->phy create a race condition?

drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
	struct sun6i_pchan *pchan = vchan->phy;
    ...
	spin_lock(&sdev->lock);
	list_del_init(&vchan->node);
	spin_unlock(&sdev->lock);

	spin_lock_irqsave(&vchan->vc.lock, flags);
    ...
	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->phy is read at the very beginning of the function, outside
of any locks.

Concurrently, sun6i_dma_tasklet() can acquire sdev->lock, assign a physical
channel to vchan->phy, and start the hardware transfer. If this race occurs,
sun6i_dma_terminate_all() sees pchan as NULL.

It subsequently acquires the necessary locks but skips the if (pchan) block,
failing to write DMA_CHAN_ENABLE_STOP to the hardware.

Will this function then return success, deceiving the caller into believing
the channel is stopped and descriptors are freed, while the hardware continues
transferring data into freed memory?

The same lockless read in sun6i_dma_resume() also seems to allow the virtual
channel to be re-added to the pending list while already having an active
physical channel.

[Severity: High]
This is a pre-existing issue, but can this sequence lead to a NULL pointer
dereference?

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
    ...
			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);
    ...
}

In sun6i_dma_interrupt(), if the channel is not cyclic, the code blocks on
spin_lock(&vchan->vc.lock).

Concurrently, a process context thread running sun6i_dma_terminate_all()
can acquire vchan->vc.lock, stop the channel, and set pchan->desc = NULL.

Once sun6i_dma_terminate_all() releases the lock, the interrupt handler
unblocks and immediately calls vchan_cookie_complete(&pchan->desc->vd).

Since pchan->desc has just been nullified, won't dereferencing
pchan->desc->vd cause a kernel oops?

Additionally, if vchan->cyclic is true, the interrupt handler calls
vchan_cyclic_callback(&pchan->desc->vd) without taking any locks, exposing
pchan->desc to concurrent nullification.

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