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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch unconditionally adds the active descriptor `pchan->desc` to `vc->desc_completed` in `sun6i_dma_terminate_all()`, causing list corruption and a potential use-after-free for non-cyclic transfers.

Pre-existing issues:
- [High] A TOCTOU race condition between `sun6i_dma_interrupt()` and `sun6i_dma_terminate_all()` allows the interrupt handler to dereference a NULL `pchan->desc` pointer.
- [High] Use-after-free when calculating residue for a recently completed transfer in `sun6i_dma_tx_status()`.
- [High] Deadlock risk due to acquiring `sdev->lock` with plain `spin_lock()` in process context, which is vulnerable to softirq preemption.
- [High] The interrupt handler locklessly reads `pchan->vchan`, which races with channel reallocation and can prematurely complete a newly started transfer.
--

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

dmaengine: sun6i-dma: Fix memory leak in sun6i_dma_terminate_all

This commit fixes a memory leak in sun6i_dma_terminate_all() for non-cyclic
DMA transfers. It ensures that both cyclic and non-cyclic active descriptors
are added to the desc_completed list before setting pchan->desc to NULL.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..7f9414e3a3814 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -945,16 +945,14 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  
>  	spin_lock_irqsave(&vchan->vc.lock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but just above this line in
sun6i_dma_terminate_all(), there is a plain spin_lock() used in process
context:

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

Does this risk an infinite spinlock deadlock? Since sun6i_dma_terminate_all()
is callable from process context, a hardirq can preempt execution while
sdev->lock is held. When the hardirq finishes, the pending softirqs would run
sun6i_dma_tasklet on the same CPU, which attempts to acquire the lock using
spin_lock_irqsave(&sdev->lock, flags), potentially deadlocking.

> -	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) {
> +		struct virt_dma_desc *vd = &pchan->desc->vd;
> +		struct virt_dma_chan *vc = &vchan->vc;
>  
> -			list_add_tail(&vd->node, &vc->desc_completed);
> -		}
> +		list_add_tail(&vd->node, &vc->desc_completed);

[Severity: High]
Does unconditionally adding the descriptor to desc_completed risk list
corruption for non-cyclic transfers?

When a non-cyclic transfer completes, sun6i_dma_interrupt() calls
vchan_cookie_complete(), which places the descriptor on vc->desc_completed,
sets pchan->done = pchan->desc, and schedules the vchan_complete tasklet.
However, pchan->desc isn't cleared until sun6i_dma_tasklet runs.

If sun6i_dma_terminate_all() executes during this window, it observes a
non-NULL pchan->desc and executes list_add_tail() again. Since the descriptor
is already on the list (or has been spliced/freed by the completion tasklet),
this double-add could corrupt the linked list pointers. Should this check
if (pchan->desc != pchan->done) first?

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race condition between
sun6i_dma_interrupt() and sun6i_dma_terminate_all()?

In sun6i_dma_interrupt(), pchan->vchan is read locklessly. If
sun6i_dma_terminate_all() runs concurrently on another CPU, it can acquire the
channel lock, set pchan->desc = NULL, and release the lock:

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

CPU2 (sun6i_dma_interrupt):
    spin_lock(&vchan->vc.lock);
    vchan_cookie_complete(&pchan->desc->vd);

Could this cause a fatal NULL pointer dereference when &pchan->desc->vd
evaluates to NULL inside vchan_cookie_complete()?


[Severity: High]
This is a pre-existing issue, but does sun6i_dma_tx_status() have a
use-after-free risk when calculating residue?

When a transfer finishes, the vchan_complete tasklet completes the callback
and frees the descriptor memory. However, pchan->desc is not cleared until
the separate sun6i_dma_tasklet runs.

If dmaengine_tx_status() is called in this window, vchan_find_desc() returns
NULL, and the code falls back to checking pchan && pchan->desc. It then calls
sun6i_get_chan_size(pchan), which accesses the descriptor:

drivers/dma/sun6i-dma.c:sun6i_get_chan_size() {
    ...
    for (lli = txd->v_lli; lli; lli = lli->v_lli_next) {
    ...
}

Will this dereference the already-freed descriptor memory?


[Severity: High]
This is a pre-existing issue, but does the interrupt handler risk prematurely
completing a newly started transfer?

sun6i_dma_terminate_all() stops the physical channel but leaves pending
interrupt bits set in the hardware DMA_IRQ_STAT register. The
sun6i_dma_tasklet can immediately reallocate this physical channel to a
new virtual channel and start it.

If the delayed hardware interrupt from the old transfer fires,
sun6i_dma_interrupt() reads pchan->vchan locklessly:

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
    ...
    vchan = pchan->vchan;
    if (vchan && (status & vchan->irq_type)) {
    ...
}

If it evaluates as the newly assigned vchan, and non-cyclic transfers share
the same irq_type bitmask, could the handler incorrectly acquire the new
vchan->vc.lock and prematurely complete the new transfer's descriptor?

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