Re: [PATCH 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers

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

Pre-existing issues:
- [Critical] A race condition between two asynchronously scheduled tasklets can lead to a Use-After-Free when calculating the residue of a completed DMA transfer.
- [Critical] The hardware DMA descriptor (`struct sun6i_dma_lli`) uses native `u32` fields instead of `__le32` and lacks `cpu_to_le32()` conversions, which will cause data corruption on big-endian systems.
- [High] Lockless reading of `vchan->phy` before acquiring `vchan->vc.lock` allows a race condition where `tx_status` can report the residue of a completely unrelated transfer.
--

--- Patch [1]: [PATCH 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers ---
commit 6f389dcb6ffcfcb91977e76e00d8063e17e3119f
Author: Christian Lugnberg <[email protected]>

dmaengine: sun6i: fix non-atomic read of DMA position registers

This commit fixes a race condition when reading DMA_CHAN_LLI_ADDR and
DMA_CHAN_CUR_CNT in sun6i_get_chan_size(). It implements a double-read
pattern to ensure both registers are sampled during the same descriptor
interval. This prevents inaccurate residue calculations that caused
phantom position jumps and false xrun reports.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..eb060ba2d4738 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -354,8 +354,10 @@ static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
>  	size_t bytes;
>  	dma_addr_t pos;
>  
> -	pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> -	bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	do {
> +		pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> +		bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	} while (pos != readl(pchan->base + DMA_CHAN_LLI_ADDR));
>  
>  	if (pos == LLI_LAST_ITEM)
>  		return bytes;

[Severity: High]
This is a pre-existing issue, but can sun6i_get_chan_size() be called with
a stale pchan?

Looking at sun6i_dma_tx_status(), the pchan pointer is read locklessly:

    struct sun6i_pchan *pchan = vchan->phy;
    ...
    spin_lock_irqsave(&vchan->vc.lock, flags);

If the sun6i_dma_tasklet() reallocates this physical channel to a different
virtual channel before the lock is acquired, pchan becomes stale. Once the
lock is held, sun6i_dma_tx_status() fails to find the original descriptor,
falls back to using the stale pchan, and calls sun6i_get_chan_size().

Will this result in calculating the residue based on a completely unrelated
transfer?

[Severity: Critical]
This is another pre-existing issue, but is it safe to trust pchan->desc when
calculating the size?

In sun6i_dma_tx_status(), if the descriptor is no longer in the active lists:

    } else if (!pchan || !pchan->desc) {
        bytes = 0;
    } else {
        bytes = sun6i_get_chan_size(pchan);
    }

If the vchan tasklet has just run, it might have already freed the descriptor.
When sun6i_get_chan_size() is called, it dereferences this dangling pointer:

    struct sun6i_desc *txd = pchan->desc;
    ...
    for (lli = txd->v_lli; lli; lli = lli->v_lli_next) {

Could this race condition between the asynchronously scheduled tasklets lead
to a use-after-free?

[Severity: Critical]
This is also a pre-existing issue, but does the hardware descriptor layout
properly account for CPU endianness?

In struct sun6i_dma_lli, the fields are defined as native u32 rather than
__le32:

    struct sun6i_dma_lli {
        u32         cfg;
        u32         src;
        ...
    };

And in sun6i_dma_set_addr(), values are assigned directly without
cpu_to_le32() conversions:

    v_lli->src = lower_32_bits(src);
    v_lli->dst = lower_32_bits(dst);

Since the DMA controller reads these values from memory, won't this cause
data corruption and DMA faults on big-endian systems?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1