[PATCH 0/2] dmaengine: sun6i: Fix two bugs in the DMA status path

[PATCH 0/2] dmaengine: sun6i: Fix two bugs in the DMA status path

[Christian Lugnberg]

This series fixes two bugs in the sun6i DMA driver's tx_status path.

The first patch fixes a non-atomic read of DMA_CHAN_LLI_ADDR and
DMA_CHAN_CUR_CNT in sun6i_get_chan_size(), introduced when cyclic DMA
support was added. The race causes spurious ALSA xruns on Allwinner H3
(Barix IPAM400), confirmed by observation on kernel 6.12.

The second patch fixes a null pointer dereference in sun6i_dma_tx_status()
where to_sun6i_desc() was called unconditionally on the return value of
vchan_find_desc() before the NULL check, present since the driver was
first introduced.

Both patches are candidates for stable backport.

Christian Lugnberg (2):
  dmaengine: sun6i: fix non-atomic read of DMA position registers
  dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status

 drivers/dma/sun6i-dma.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.54.0 (Apple Git-156)

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

[Christian Lugnberg]

sun6i_get_chan_size() reads DMA_CHAN_LLI_ADDR and DMA_CHAN_CUR_CNT in two
separate readl() calls with no synchronisation between them:

    pos   = readl(pchan->base + DMA_CHAN_LLI_ADDR);
    bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);

DMA_CHAN_LLI_ADDR holds the physical address of the *next* descriptor the
engine will load once the current one completes. DMA_CHAN_CUR_CNT holds the
remaining byte count for the *current* descriptor. If the DMA engine
advances to the next LLI entry between the two reads, pos becomes stale: it
still points to what was the next descriptor at the time of the first read,
but that descriptor is now the current one and CUR_CNT reflects its initial
(full) byte count. The subsequent virtual-chain walk starts one entry too
early and accumulates an extra full period's worth of bytes into the
residue estimate.

For ALSA cyclic buffers the over-counted residue can reach the full buffer
size, causing the computed playback position to appear to jump backward to
near zero. The ALSA PCM core treats such a backward discontinuity in hw_ptr
as evidence that the buffer has underrun and declares an xrun.

On the Barix IPAM400 (Allwinner H3, kernel 6.12) this manifests as audible
glitches accompanied by spurious xrun log entries, confirmed by two
independent observations:

First, the ALSA buffer in the affected configuration is 2 seconds deep with
a 500 ms refill period (the interval at which the player software wakes up
to top up the buffer). For a real underrun to occur the player would have
to stall for the full 2 seconds without writing any audio — effectively
impossible under normal scheduling conditions. Yet xruns are observed
regularly.

Second, the underrun duration reported by the kernel at xrun time is
~30 µs, roughly one audio sample at 44100 Hz. A genuine drain of a 2
second buffer cannot resolve in 30 µs; only a phantom position jump
caused by a register read race can produce such a number.

Observed on a 44100 Hz stereo S16_LE stream:

  $ cat /proc/asound/Codec/pcm0p/sub0/status
  state: XRUN
  delay: 0
  avail: 88200
  avail_max: 22514

The avail_max of 22514 frames (511 ms) matches exactly one ALSA period —
the amount added by starting the LLI chain walk one entry too early.

The race window itself is narrow. Each DMA descriptor covers approximately
88 samples (~2 ms at 44100 Hz), so the engine advances to a new descriptor
roughly every 2 ms. The two readl() calls must straddle that exact boundary
for the corruption to occur, which explains why the bug is intermittent.

The bug is further confirmed by the xrun_debug bit 2 toggle (jiffies
position validation). With it enabled xruns cease immediately and do not
return; clearing it causes xruns to reappear within minutes. This on/off
reproducibility isolates the fault to the hw_ptr position reporting path;
the DMA engine itself is functioning correctly, as evidenced by hw_ptr
advancing at a steady 44100 frames/sec between events:

  $ echo 4 > /proc/asound/Codec/pcm0p/xrun_debug  # xruns stop
  $ echo 0 > /proc/asound/Codec/pcm0p/xrun_debug  # xruns return

Fix this by re-reading DMA_CHAN_LLI_ADDR after DMA_CHAN_CUR_CNT and
retrying if the value changed. This double-read pattern guarantees that
both registers were sampled during the same descriptor interval. The cost
is at most one extra readl() pair per call in the racy case, which occurs
only at descriptor boundaries (~every 2 ms) and is negligible.

Fixes: a90e173f3faf ("dmaengine: sun6i: Add cyclic capability")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
---
 drivers/dma/sun6i-dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index f47a326dd7ff..04fe1f5042e9 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;
-- 
2.54.0 (Apple Git-156)

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

[sashiko-bot]

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 <christian.lugnberg@soundtrack.io>

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/20260814132906.70322-1-christian.lugnberg@soundtrack.io?part=1

[PATCH 2/2] dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status

[Christian Lugnberg]

sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
descriptor for a given cookie, then unconditionally dereferences the
result via to_sun6i_desc() before checking whether the pointer is NULL:

    vd = vchan_find_desc(&vchan->vc, cookie);
    txd = to_sun6i_desc(&vd->tx);   /* vd may be NULL here */

    if (vd) {
        for (lli = txd->v_lli; ...)

vchan_find_desc() returns NULL when the descriptor has already been
completed or is in-flight on a physical channel and no longer present
in the virtual channel's descriptor list. Dereferencing NULL via
to_sun6i_desc() in that case is undefined behaviour and will oops on
any architecture that faults on NULL pointer access.

Move the to_sun6i_desc() call inside the if (vd) block so it is only
reached when vd is known to be non-NULL:

    vd = vchan_find_desc(&vchan->vc, cookie);
    if (vd) {
        struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
        for (lli = txd->v_lli; ...)

Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
---
 drivers/dma/sun6i-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 04fe1f5042e9..7704b016aed8 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -981,7 +981,6 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
 	struct sun6i_pchan *pchan = vchan->phy;
 	struct sun6i_dma_lli *lli;
 	struct virt_dma_desc *vd;
-	struct sun6i_desc *txd;
 	enum dma_status ret;
 	unsigned long flags;
 	size_t bytes = 0;
@@ -993,9 +992,9 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
 	spin_lock_irqsave(&vchan->vc.lock, flags);
 
 	vd = vchan_find_desc(&vchan->vc, cookie);
-	txd = to_sun6i_desc(&vd->tx);
 
 	if (vd) {
+		struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
 		for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
 			bytes += lli->len;
 	} else if (!pchan || !pchan->desc) {
-- 
2.54.0 (Apple Git-156)

Re: [PATCH 2/2] dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status

[sashiko-bot]

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

New issues:
- [Medium] The commit message falsely claims to fix a NULL pointer dereference that causes a kernel oops, but the original code performs no memory access at that location.

Pre-existing issues:
- [High] TOCTOU race condition from lockless read of `vchan->phy` causes hardware state corruption, use-after-free, and NULL pointer dereferences.
- [Medium] sun6i_dma_tx_status fails to verify the active descriptor's cookie, causing it to report incorrect residue for unrelated cookies.
--

--- Patch [2]: [PATCH 2/2] dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status ---
commit a4a11017b14d1cd4a177bac4da56b1c3eb480161
Author: Christian Lugnberg <christian.lugnberg@soundtrack.io>

dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status

This commit attempts to fix an alleged null pointer dereference in
sun6i_dma_tx_status() by moving the to_sun6i_desc() call inside the
NULL check for vd.

> vchan_find_desc() returns NULL when the descriptor has already been
> completed or is in-flight on a physical channel and no longer present
> in the virtual channel's descriptor list. Dereferencing NULL via
> to_sun6i_desc() in that case is undefined behaviour and will oops on
> any architecture that faults on NULL pointer access.

[Severity: Medium]
Is this description accurate?

Looking at the original code, to_sun6i_desc() expands to a container_of()
macro on &vd->tx. Since tx is at offset 0 of virt_dma_desc and vd is at
offset 0 of sun6i_desc, calculating the offset on a NULL pointer evaluates
to NULL through pure pointer arithmetic.

Does this actually perform a memory access that would trigger a hardware
fault or oops?

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index eb060ba2d4738..070101c109a3f 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -984,7 +984,6 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  	struct sun6i_pchan *pchan = vchan->phy;

[Severity: High]
This is a pre-existing issue, but can this lockless read lead to state
corruption or a use-after-free?

If vchan->phy is read into the local variable pchan outside the lock,
could a concurrent tasklet take the lock, complete the transfer, clear
vchan->phy, and reassign the physical channel to a completely different
virtual channel before we acquire the lock here?

If so, would we be operating on a reassigned pchan without verifying
ownership inside the lock? It looks like this same pattern exists in
sun6i_dma_pause(), sun6i_dma_resume(), and sun6i_dma_terminate_all().

>  	struct sun6i_dma_lli *lli;
>  	struct virt_dma_desc *vd;
> -	struct sun6i_desc *txd;
>  	enum dma_status ret;
>  	unsigned long flags;
>  	size_t bytes = 0;

[ ... ]

>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>  
>  	vd = vchan_find_desc(&vchan->vc, cookie);
> -	txd = to_sun6i_desc(&vd->tx);
>  
>  	if (vd) {
> +		struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
>  		for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
>  			bytes += lli->len;
>  	} else if (!pchan || !pchan->desc) {
>  		bytes = 0;
>  	} else {
>  		bytes = sun6i_get_chan_size(pchan);

[Severity: Medium]
This is a pre-existing issue, but does this code accurately report the
residue if the queried cookie doesn't match the active descriptor?

If the queried cookie is in the desc_submitted list and therefore not
found by vchan_find_desc(), this fallback branch assumes pchan->desc
corresponds to the requested transfer.

Should we verify if pchan->desc->vd.tx.cookie == cookie before blindly
calling sun6i_get_chan_size(pchan) and returning the residue of a
potentially unrelated transfer?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814132906.70322-1-christian.lugnberg@soundtrack.io?part=2