Re: [PATCH] dmaengine: sun6i: Fix physical channel index in interrupt handler

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing interrupt handling for higher DMA channels due to truncating integer division.
- [High] TOCTOU race condition in `sun6i_dma_interrupt` leads to NULL pointer dereference.
- [High] Softirq deadlock in `sun6i_dma_pause` and `sun6i_dma_terminate_all` due to unprotected `sdev->lock` acquisition.
- [High] TOCTOU race reading `vchan->phy` locklessly causing DMA memory corruption (use-after-free on target buffers).
--

commit b276e8e28f169d8ad907c8404d5562072f082f83
Author: Hongling Zeng <zenghongling@kylinos.cn>
dmaengine: sun6i: Fix physical channel index in interrupt handler

This commit fixes a bug where SoCs with more than 8 DMA channels incorrectly
routed interrupts from higher physical channels to physical channels 0-7. It
updates the physical channel lookup to use the absolute channel index instead
of just the inner loop index.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..3e03d39e25aa6 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -543,21 +543,21 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  	struct sun6i_vchan *vchan;
>  	struct sun6i_pchan *pchan;
>  	int i, j, ret = IRQ_NONE;
>  	u32 status;
>  
>  	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {

[Severity: High]
This is a pre-existing issue, but does this loop condition skip higher
channels on some SoCs?

Since DMA_IRQ_CHAN_NR is 8, if sdev->num_pchans is not a multiple of 8 (for
example 12 on Allwinner H3), the truncating division (12 / 8 = 1) causes the
loop to only check i = 0 (channels 0-7). The interrupts for channels 8-11
would remain unacknowledged, which might lead to an interrupt storm.

Could this be fixed by using DIV_ROUND_UP(sdev->num_pchans, DMA_IRQ_CHAN_NR)?

>  		status = readl(sdev->base + DMA_IRQ_STAT(i));
>  		if (!status)
>  			continue;
>  
>  		dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
>  			str_high_low(i), status);
>  
>  		writel(status, sdev->base + DMA_IRQ_STAT(i));
>  
>  		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
> -			pchan = sdev->pchans + j;
> +			pchan = sdev->pchans + i * DMA_IRQ_CHAN_NR + 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);
>  					pchan->done = pchan->desc;
>  					spin_unlock(&vchan->vc.lock);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to locklessly cache
pchan->vchan and then dereference pchan->desc inside the lock?

If sun6i_dma_terminate_all() is called concurrently on another CPU:

CPU1 (terminate_all):
    spin_lock(&vchan->vc.lock);
    vchan->phy = NULL;
    pchan->vchan = NULL;
    pchan->desc = NULL;
    spin_unlock(&vchan->vc.lock);

CPU2 (sun6i_dma_interrupt):
    vchan = pchan->vchan; // Read locklessly before CPU1 sets it to NULL
    ...
    spin_lock(&vchan->vc.lock); // Waits for CPU1
    vchan_cookie_complete(&pchan->desc->vd); // pchan->desc is now NULL

Could this lead to a NULL pointer dereference in vchan_cookie_complete()?

>  				}
>  			}
>  
>  			status = status >> DMA_IRQ_CHAN_WIDTH;
>  		}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the lock acquisition in
sun6i_dma_pause() and sun6i_dma_terminate_all() risk a softirq deadlock?

In sun6i_dma_pause():

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

And similarly in sun6i_dma_terminate_all(). Since these functions acquire
sdev->lock without disabling softirqs, could they deadlock if preempted by
sun6i_dma_tasklet() running in softirq context on the same CPU, which calls
spin_lock_irq(&sdev->lock)?

[Severity: High]
This isn't a bug introduced by this patch, but does sun6i_dma_terminate_all()
have a race condition when reading vchan->phy locklessly?

drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
    struct sun6i_pchan *pchan = vchan->phy;
    ...
    spin_lock_irqsave(&vchan->vc.lock, flags);
    ...
    if (pchan) {
        writel(DMA_CHAN_ENABLE_STOP, pchan->base + DMA_CHAN_ENABLE);
    ...
}

If sun6i_dma_tasklet() concurrently allocates a physical channel and updates
vchan->phy right after this lockless read, pchan will remain NULL locally.
The hardware stop command would be skipped, allowing the DMA hardware to
continue running while the caller assumes it has been stopped. Can this lead
to memory corruption if the caller frees or reuses the target buffers?

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