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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition in IRQ handler leading to NULL pointer dereference
- [High] Missing tasklet_schedule in sun6i_dma_resume causes stalled transfers
--

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

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

The commit refactors the error handling paths in the sun6i-dma driver by
introducing the helper functions sun6i_dma_free_desc() and
sun6i_dma_free_desc_virt(). This resolves a use-after-free issue when
iterating and freeing a virtual LLI list in sun6i_dma_prep_slave_sg() and
sun6i_dma_prep_dma_cyclic().

> 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);
> 				}
> 			}
> ...
> }

[Severity: High]
This is a pre-existing issue, but could this cause a NULL pointer dereference?

Looking at sun6i_dma_terminate_all(), it clears pchan->desc:

> drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
> ...
> 	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_terminate_all() runs concurrently with sun6i_dma_interrupt() on
SMP systems, sun6i_dma_terminate_all() holds vchan->vc.lock, stops the
hardware, and sets pchan->desc = NULL. Meanwhile, the IRQ handler reads
pchan->vchan locklessly.

If the IRQ handler evaluates this before it is cleared, it will proceed. For
non-cyclic transfers, the IRQ handler blocks on vchan->vc.lock. When
sun6i_dma_terminate_all() releases the lock, the IRQ handler continues and
evaluates &pchan->desc->vd. Since pchan->desc is NULL, could this pass a NULL
pointer to vchan_cookie_complete() and cause a kernel panic? For cyclic
transfers, the race appears even wider since the handler evaluates
&pchan->desc->vd completely locklessly.

> 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);
> 	}
> ...
> }

[Severity: High]
This is a pre-existing issue, but does this code leave transfers stalled
indefinitely?

If a DMA channel is paused (sun6i_dma_pause) while it has pending descriptors
but is not yet assigned to a physical hardware channel (pchan == NULL), it is
removed from the sdev->pending list.

When the channel is later resumed here in sun6i_dma_resume(), it is re-added
to sdev->pending. However, sun6i_dma_resume() fails to call
tasklet_schedule(&sdev->task). Since the tasklet is responsible for pulling
virtual channels off the pending list and assigning them to physical channels,
will this channel sit in the pending queue indefinitely until an independent
event schedules the tasklet?

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