Skip to contentSkip to conversation

dmaengine: mmp_pdma: Fix race condition in mmp_pdma_residue()

PATCH Superseded Wenshan Lan 1 participant 2 messages Feb 28 Stable branch

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 dmaengine: mmp_pdma: Fix race condition in mmp_pdma_residue()

From: Guodong Xu <[email protected]>

[ 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.

---
 drivers/dma/mmp_pdma.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/dma/mmp_pdma.c +6 -0
index e8d71b35593e..bac4905c47db 100644
@@ -764,6 +764,7 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
 {
 	struct mmp_pdma_desc_sw *sw;
 	u32 curr, residue = 0;
+	unsigned long flags;
 	bool passed = false;
 	bool cyclic = chan->cyclic_first != NULL;
 
@@ -779,6 +780,8 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
 	else
 		curr = readl(chan->phy->base + DSADR(chan->phy->idx));
 
+	spin_lock_irqsave(&chan->desc_lock, flags);
+
 	list_for_each_entry(sw, &chan->chain_running, node) {
 		u32 start, end, len;
 
@@ -822,6 +825,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;
@@ -829,6 +833,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;
 }
Signature · 1 line
2.43.0
#1 lore raw

Please ignore this email for I forgot cc the related person.

Quoted 78 lines · Wenshan Lan

From: Guodong Xu <[email protected]>

[ 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.

---
  drivers/dma/mmp_pdma.c | 6 ++++++
  1 file changed, 6 insertions(+)
drivers/dma/mmp_pdma.c +6 -0
index e8d71b35593e..bac4905c47db 100644
@@ -764,6 +764,7 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
  {
  	struct mmp_pdma_desc_sw *sw;
  	u32 curr, residue = 0;
+	unsigned long flags;
  	bool passed = false;
  	bool cyclic = chan->cyclic_first != NULL;
 
@@ -779,6 +780,8 @@ static unsigned int mmp_pdma_residue(struct mmp_pdma_chan *chan,
  	else
  		curr = readl(chan->phy->base + DSADR(chan->phy->idx));
 
+	spin_lock_irqsave(&chan->desc_lock, flags);
+
  	list_for_each_entry(sw, &chan->chain_running, node) {
  		u32 start, end, len;
 
@@ -822,6 +825,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;
@@ -829,6 +833,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;
  }