Re: [PATCH v2] dmaengine: sun6i: Fix potential deadlock on sdev->lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The DMA interrupt handler erroneously uses the relative inner loop index `j` to resolve physical channels, causing false transfer completions and silent memory corruption for channels >= 8.
- [High] Unprotected read of `vchan->phy` before acquiring locks creates severe TOCTOU races, risking memory corruption and crashes.
- [High] Concurrent execution of `sun6i_dma_interrupt` and `sun6i_dma_terminate_all` causes a NULL pointer dereference on `pchan->desc`.
- [High] Calling `sun6i_dma_resume` on an already pending channel corrupts the `sdev->pending` list.
- [High] `sun6i_dma_terminate_all()` permanently leaks the active descriptor and its DMA pool entries for non-cyclic transfers.
- [High] Use-after-free when traversing the LLI linked list in the error handling paths of `sun6i_dma_prep_slave_sg()` and `sun6i_dma_prep_dma_cyclic()`.
--

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

dmaengine: sun6i: Fix potential deadlock on sdev->lock

sun6i_dma_terminate_all() and sun6i_dma_pause() acquire sdev->lock
with plain spin_lock() from process context. If a DMA interrupt fires
on the same CPU while the lock is held, the interrupt handler schedules
sun6i_dma_tasklet(), which runs in softirq context and attempts to
acquire the same lock with spin_lock_irq(), causing a deadlock.

Cc: stable@vger.kernel.org

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..a6487971ad09e 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -891,6 +891,7 @@ static int sun6i_dma_pause(struct dma_chan *chan)
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
>  	struct sun6i_pchan *pchan = vchan->phy;

[Severity: High]
This isn't a bug introduced by this patch, but vchan->phy is read without
holding vchan->vc.lock. Can this allow pchan to become stale if the
tasklet completes the transfer and reassigns the physical channel to a
different virtual channel before the locks are acquired?

> +	unsigned long flags;
>  
>  	dev_dbg(chan2dev(chan), "vchan %p: pause\n", &vchan->vc);
>  
> @@ -898,9 +899,9 @@ static int sun6i_dma_pause(struct dma_chan *chan)
>  		writel(DMA_CHAN_PAUSE_PAUSE,
>  		       pchan->base + DMA_CHAN_PAUSE);
>  	} else {
> -		spin_lock(&sdev->lock);
> +		spin_lock_irqsave(&sdev->lock, flags);
>  		list_del_init(&vchan->node);
> -		spin_unlock(&sdev->lock);
> +		spin_unlock_irqrestore(&sdev->lock, flags);
>  	}
>  
>  	return 0;

[ ... ]

> @@ -939,9 +940,9 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)

[Severity: High]
This is a pre-existing issue, but vchan->phy is read unlocked in this
function as well. Could the stale pchan be used to modify hardware registers
and descriptor lists for the wrong transfer inside the lock?

>  	unsigned long flags;
>  	LIST_HEAD(head);
>  
> -	spin_lock(&sdev->lock);
> +	spin_lock_irqsave(&sdev->lock, flags);
>  	list_del_init(&vchan->node);
> -	spin_unlock(&sdev->lock);
> +	spin_unlock_irqrestore(&sdev->lock, flags);
>  
>  	spin_lock_irqsave(&vchan->vc.lock, flags);


[Severity: High]
This isn't a bug introduced by this patch, but does sun6i_dma_terminate_all()
permanently leak the active descriptor and its DMA pool entries for non-cyclic
transfers?

When a transfer is aborted, the code clears pchan->desc:

drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
	...
	if (vchan->cyclic) {
		vchan->cyclic = false;
		if (pchan && pchan->desc) {
			...
			list_add_tail(&vd->node, &vc->desc_completed);
		}
	}

	vchan_get_all_descriptors(&vchan->vc, &head);

	if (pchan) {
		...
		pchan->desc = NULL;
	}
	...
}

It doesn't append the active descriptor to any cleanup list for non-cyclic
transfers before clearing it, which means vchan_get_all_descriptors() fails
to capture it.


[Severity: Critical]
This is a pre-existing issue, but does the DMA interrupt handler erroneously
use the relative inner loop index 'j' to resolve physical channels?

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
	...
	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
		...
		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
			pchan = sdev->pchans + j;
	...
}

On SoCs with more than 8 channels, shouldn't this use the absolute index
(i * DMA_IRQ_CHAN_NR + j)? Using 'j' always resolves to channels 0-7, which
could falsely complete unrelated channels and cause silent memory corruption
for channels >= 8.


[Severity: High]
This isn't a bug introduced by this patch, but can concurrent execution of
sun6i_dma_interrupt() and sun6i_dma_terminate_all() cause a NULL pointer
dereference?

In the hard IRQ handler, sun6i_dma_interrupt() acquires the lock and
unconditionally dereferences pchan->desc:

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);
			pchan->done = pchan->desc;
			spin_unlock(&vchan->vc.lock);
		}
	}
	...
}

If sun6i_dma_terminate_all() runs concurrently, it acquires vchan->vc.lock,
forcefully stops the channel, and sets pchan->desc = NULL. When the interrupt
handler subsequently acquires the lock, wouldn't it dereference the NULL
pchan->desc?


[Severity: High]
This is a pre-existing issue, but does calling sun6i_dma_resume() on an
already pending channel corrupt the sdev->pending list?

drivers/dma/sun6i-dma.c:sun6i_dma_resume() {
	...
	} else if (!list_empty(&vchan->vc.desc_issued)) {
		spin_lock(&sdev->lock);
		list_add_tail(&vchan->node, &sdev->pending);
		spin_unlock(&sdev->lock);
	}
	...
}

Unlike sun6i_dma_issue_pending(), this fails to check
!list_empty(&vchan->node) before adding the node. If a channel is already
waiting in the sdev->pending list, couldn't this re-addition corrupt the
list pointers?


[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
when traversing the LLI linked list in the error handling paths?

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

The loop body frees v_lli immediately, but the loop's update expression
then reads v_lli->p_lli_next and v_lli->v_lli_next from the just-freed
memory. This same pattern also appears in sun6i_dma_prep_dma_cyclic().

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