[PATCH AUTOSEL 6.18] dmaengine: mmp_pdma: Fix race condition in mmp_pdma_residue()

[Sasha Levin]

From: Guodong Xu <guodong at riscstar.com>

[ Upstream commit a143545855bc2c6e1330f6f57ae375ac44af00a7 ]

Add proper locking in mmp_pdma_residue() to prevent use-after-free when
accessing descriptor list and descriptor contents.

The race occurs when multiple threads call tx_status() while the tasklet
on another CPU is freeing completed descriptors:

CPU 0                              CPU 1
-----                              -----
mmp_pdma_tx_status()
mmp_pdma_residue()
  -> NO LOCK held
     list_for_each_entry(sw, ..)
                                   DMA interrupt
                                   dma_do_tasklet()
                                     -> spin_lock(&desc_lock)
                                        list_move(sw->node, ...)
                                        spin_unlock(&desc_lock)
  |                                     dma_pool_free(sw) <- FREED!
  -> access sw->desc <- UAF!

This issue can be reproduced when running dmatest on the same channel with
multiple threads (threads_per_chan > 1).

Fix by protecting the chain_running list iteration and descriptor access
with the chan->desc_lock spinlock.

Signed-off-by: Juan Li <lijuan at linux.spacemit.com>
Signed-off-by: Guodong Xu <guodong at riscstar.com>
Link: https://patch.msgid.link/20251216-mmp-pdma-race-v1-1-976a224bb622@riscstar.com
Signed-off-by: Vinod Koul <vkoul at kernel.org>
Signed-off-by: Sasha Levin <sashal at kernel.org>
---

LLM Generated explanations, may be completely bogus:

This confirms the `mmp_pdma_residue()` function was added in 2014. The
race condition has existed since then.

## Analysis Summary

### 1. COMMIT MESSAGE ANALYSIS

The commit message clearly describes:
- A **use-after-free (UAF) race condition** in `mmp_pdma_residue()`
- The race occurs between `tx_status()` (calling `mmp_pdma_residue()`)
  and the DMA tasklet (`dma_do_tasklet()`)
- **Clear reproduction steps**: running dmatest with `threads_per_chan >
  1`
- The CPU timeline diagram explicitly shows the race window between
  unlocked list iteration and descriptor free

Keywords: "Fix race condition", "use-after-free", "UAF"

### 2. CODE CHANGE ANALYSIS

**The bug:** `mmp_pdma_residue()` iterates over `chan->chain_running`
list and accesses descriptor contents (`sw->desc`) without holding the
`desc_lock` spinlock. Meanwhile, `dma_do_tasklet()` can:
1. Hold `desc_lock`
2. Move descriptors from `chain_running` to `chain_cleanup` via
   `list_move()`
3. Release `desc_lock`
4. Free the descriptor via `dma_pool_free()`

If `mmp_pdma_residue()` is iterating over the list when this happens, it
can access freed memory (UAF).

**The fix:** Adds `spin_lock_irqsave(&chan->desc_lock, flags)` before
the `list_for_each_entry()` loop and `spin_unlock_irqrestore()` at all
exit points from the function. This is:
- Small (+6 lines, -0 lines of actual logic)
- Surgical - uses the existing `desc_lock` that already protects this
  list elsewhere
- Follows existing driver patterns (other list operations use this lock)

### 3. CLASSIFICATION

This is a **bug fix** for a **use-after-free vulnerability**. This is a
serious memory safety issue:
- Can cause kernel crashes/panics
- Can lead to data corruption
- Potential security implications (UAF bugs are commonly exploitable)

### 4. SCOPE AND RISK ASSESSMENT

- **Lines changed**: ~10 lines (just adding lock/unlock around existing
  code)
- **Files touched**: 1 file
- **Complexity**: Very low - straightforward spinlock acquisition
- **Risk of regression**: Minimal - uses existing lock that is designed
  for this purpose
- **Potential issues**: Slightly increased lock contention when calling
  `tx_status()`, but this is necessary for correctness

### 5. USER IMPACT

- **Who is affected**: Users of MMP PDMA DMA controller (Marvell PXA/MMP
  platforms, SpacemiT K1)
- **Trigger conditions**: Multi-threaded DMA operations on same channel
  (common in dmatest, may occur in real workloads)
- **Severity**: High - UAF can cause kernel crashes or undefined
  behavior
- **Reproducibility**: Can be reproduced with dmatest (`threads_per_chan
  > 1`)

### 6. STABILITY INDICATORS

- Has proper sign-offs: `Signed-off-by` from author, forwarder, and DMA
  maintainer (Vinod Koul)
- Link to mailing list discussion included
- The existing lock (`desc_lock`) has been in the driver since the
  beginning
- The locking pattern matches other functions in the driver

### 7. DEPENDENCY CHECK

- No dependencies on other commits
- The affected code (`mmp_pdma_residue()`) has existed since 2014
- The `desc_lock` spinlock exists in all stable trees
- Should apply cleanly to any kernel from 3.15+ (when residue reporting
  was added)

**Note**: Recent mainline commits added SpacemiT K1 support with 64-bit
operations (`pdev->ops->get_desc_dst_addr`, etc.), but the core logic
and the race condition existed with the original 32-bit implementation
too. For older stable trees (pre-6.x), minor context adjustments might
be needed due to the `pdev->ops->` refactoring, but the fix concept
remains the same.

### Conclusion

This commit fixes a clear **use-after-free race condition** in the MMP
PDMA driver. The bug:
- Can cause kernel crashes/panics
- Has existed since 2014 (when residue reporting was added)
- Is reproducible with standard kernel testing tools (dmatest)

The fix:
- Is small and surgical (~10 lines of spinlock additions)
- Uses existing infrastructure (the `desc_lock` already exists for this
  purpose)
- Follows existing driver patterns
- Has minimal regression risk

This is exactly the type of fix stable trees need: a clearly correct fix
for a real bug that users can hit, with minimal risk of introducing new
problems.

**YES**

 drivers/dma/mmp_pdma.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index d07229a748868..481b58c414e47 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -928,6 +928,7 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
 {
 	struct mmp_pdma_desc_sw *sw;
 	struct mmp_pdma_device *pdev = to_mmp_pdma_dev(chan->chan.device);
+	unsigned long flags;
 	u64 curr;
 	u32 residue = 0;
 	bool passed = false;
@@ -945,6 +946,8 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
 	else
 		curr = pdev->ops->read_src_addr(chan->phy);
 
+	spin_lock_irqsave(&chan->desc_lock, flags);
+
 	list_for_each_entry(sw, &chan->chain_running, node) {
 		u64 start, end;
 		u32 len;
@@ -989,6 +992,7 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
 			continue;
 
 		if (sw->async_tx.cookie == cookie) {
+			spin_unlock_irqrestore(&chan->desc_lock, flags);
 			return residue;
 		} else {
 			residue = 0;
@@ -996,6 +1000,8 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
 		}
 	}
 
+	spin_unlock_irqrestore(&chan->desc_lock, flags);
+
 	/* We should only get here in case of cyclic transactions */
 	return residue;
 }
-- 
2.51.0