[PATCH v5 net] net: page_pool: fix UAF in __page_pool_release_netmem_dma on xa_cmpxchg race

This bug was discovered while testing the hns3 driver under channel
reconfiguration (`ethtool -L` / `ethtool -G`) with iperf3 traffic on
arm64. The race is intermittently triggered when page_pool_destroy()
runs page_pool_scrub() concurrently with page return via
page_pool_put_netmem() on a different CPU. A WARN in
page_pool_clear_pp_info() surfaced the dangling DMA index bits left
by the cmpxchg loser, which led to the investigation.

page_pool_scrub() iterates pool->dma_mapped via xa_for_each() with no
page ref held. __page_pool_release_netmem_dma() currently reads and
writes netmem fields (dma_addr, DMA index bits in pp_magic) after
xa_cmpxchg() returns. The unref path calls put_page() unconditionally
regardless of the cmpxchg outcome; when it loses the cmpxchg, it still
frees the page before the scrub winner finishes these netmem accesses,
so scrub touches a freed page -- a Use-After-Free.

Fix this by splitting the DMA release into two functions:

1. __page_pool_unmap_netmem_dma() caches dma_addr before xa_cmpxchg(),
   does the cmpxchg to remove the DMA mapping, and calls dma_unmap on
   the cached address. It never touches netmem fields after the cmpxchg,
   making it safe for the scrub path which holds no page ref.

2. __page_pool_release_netmem_dma() wraps the above and additionally
   clears dma_addr and DMA index bits in netmem fields. This is safe
   only when the caller holds a page ref, so it is used by the return
   path (page_pool_return_netmem).

The scrub path calls __page_pool_unmap_netmem_dma() directly; the return
path calls __page_pool_release_netmem_dma().

Fixes: ee62ce7a1d90 ("page_pool: Track DMA-mapped pages and unmap them when destroying the pool")
Suggested-by: Mina Almasry <[email protected]>
Reviewed-by: Mina Almasry <[email protected]>
Assisted-by: OhMyOpenCode:GLM-5.2
Signed-off-by: Jijie Shao <[email protected]>
---
Changes in v5:
- Replace goto label with if block per Jakub's review.
- Add bug discovery context to commit message per Jakub's request.
- Add Reviewed-by tag from Mina.
- Link to v4: https://lore.kernel.org/r/[email protected]

Changes in v4:
- Restructure per Mina's review: merge page_pool_remove_dma_mapping()
  into __page_pool_unmap_netmem_dma() with dma_unmap inlined via goto
  label; simplify __page_pool_release_netmem_dma() to a thin wrapper.
- Link to v3: https://lore.kernel.org/r/[email protected]

Changes in v3:
- Fix unlikely() to likely() for PP_DMA_INDEX_BITS to match
  file convention.
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- Redesign the fix per Mina's review: v1's unconditional
  netmem_set_dma_index() introduced a UAF when the scrub path
  (no page ref) writes to a page freed by the unref path.
- Cache dma_addr before xa_cmpxchg; move dma_addr/DMA index
  cleanup to page_pool_return_netmem() which holds a page ref.
- Rename page_pool_release_dma_index() to
  page_pool_remove_dma_mapping() to reflect its new role as a
  pure cmpxchg wrapper.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
 net/core/page_pool.c | 66 +++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 21dc4a9c8714..50ee550fef73 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -500,29 +500,40 @@ static int page_pool_register_dma_index(struct page_pool *pool,
 	return err;
 }
 
-static int page_pool_release_dma_index(struct page_pool *pool,
-				       netmem_ref netmem)
+static void __page_pool_unmap_netmem_dma(struct page_pool *pool,
+					 netmem_ref netmem)
 {
 	struct page *old, *page = netmem_to_page(netmem);
 	unsigned long id;
+	dma_addr_t dma;
 
-	if (unlikely(!PP_DMA_INDEX_BITS))
-		return 0;
-
-	id = netmem_get_dma_index(netmem);
-	if (!id)
-		return -1;
+	if (!pool->dma_map)
+		return;
 
-	if (in_softirq())
-		old = xa_cmpxchg(&pool->dma_mapped, id, page, NULL, 0);
-	else
-		old = xa_cmpxchg_bh(&pool->dma_mapped, id, page, NULL, 0);
-	if (old != page)
-		return -1;
+	/* Cache dma_addr before xa_cmpxchg. The scrub path holds no page ref;
+	 * the unref path calls put_page() regardless of cmpxchg outcome, so
+	 * after the cmpxchg we cannot safely touch netmem fields.
+	 */
+	dma = page_pool_get_dma_addr_netmem(netmem);
 
-	netmem_set_dma_index(netmem, 0);
+	if (likely(PP_DMA_INDEX_BITS)) {
+		id = netmem_get_dma_index(netmem);
+		if (!id)
+			return;
+
+		if (in_softirq())
+			old = xa_cmpxchg(&pool->dma_mapped,
+					 id, page, NULL, 0);
+		else
+			old = xa_cmpxchg_bh(&pool->dma_mapped,
+					    id, page, NULL, 0);
+		if (old != page)
+			return;
+	}
 
-	return 0;
+	dma_unmap_page_attrs(pool->p.dev, dma,
+			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
+			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
 }
 
 static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t gfp)
@@ -728,24 +739,16 @@ void page_pool_clear_pp_info(netmem_ref netmem)
 static __always_inline void __page_pool_release_netmem_dma(struct page_pool *pool,
 							   netmem_ref netmem)
 {
-	dma_addr_t dma;
-
+	/* Caller must hold a page ref: __page_pool_unmap_netmem_dma() is
+	 * safe without a ref, but the field clears below require it.
+	 */
 	if (!pool->dma_map)
-		/* Always account for inflight pages, even if we didn't
-		 * map them
-		 */
 		return;
 
-	if (page_pool_release_dma_index(pool, netmem))
-		return;
-
-	dma = page_pool_get_dma_addr_netmem(netmem);
-
-	/* When page is unmapped, it cannot be returned to our pool */
-	dma_unmap_page_attrs(pool->p.dev, dma,
-			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
-			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
+	__page_pool_unmap_netmem_dma(pool, netmem);
 	page_pool_set_dma_addr_netmem(netmem, 0);
+	if (likely(PP_DMA_INDEX_BITS))
+		netmem_set_dma_index(netmem, 0);
 }
 
 /* Disconnects a page (from a page_pool).  API users can have a need
@@ -1171,8 +1174,9 @@ static void page_pool_scrub(struct page_pool *pool)
 				synchronize_net();
 		}
 
+		/* No page ref, dma-unmap only. */
 		xa_for_each(&pool->dma_mapped, id, ptr)
-			__page_pool_release_netmem_dma(pool, page_to_netmem((struct page *)ptr));
+			__page_pool_unmap_netmem_dma(pool, page_to_netmem((struct page *)ptr));
 	}
 
 	/* No more consumers should exist, but producers could still

base-commit: 594d905195024b228c962627ae5ae7c17bd582a4
-- 
2.33.0