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

[Jijie Shao]

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 <almasrymina@xxxxxxxxxx>
Assisted-by: OhMyOpenCode:GLM-5.2
Signed-off-by: Jijie Shao <shaojijie@xxxxxxxxxx>
---
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/20260729110249.2824835-1-shaojijie@xxxxxxxxxx

Changes in v3:
- Fix unlikely() to likely() for PP_DMA_INDEX_BITS to match
  file convention.
- Link to v2: https://lore.kernel.org/r/20260727132612.3277927-1-shaojijie@xxxxxxxxxx

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/20260724092135.414699-1-shaojijie@xxxxxxxxxx
---
 net/core/page_pool.c | 46 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 21dc4a9c8714..497bb1906fc3 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 (!pool->dma_map)
+		return;
+
+	/* 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);
 
 	if (unlikely(!PP_DMA_INDEX_BITS))
-		return 0;
+		goto unmap;
 
 	id = netmem_get_dma_index(netmem);
 	if (!id)
-		return -1;
+		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;
-
-	netmem_set_dma_index(netmem, 0);
+		return;
 
-	return 0;
+unmap:
+	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,13 @@ 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;
-
 	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
@@ -1172,7 +1172,7 @@ static void page_pool_scrub(struct page_pool *pool)
 		}
 
 		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: 2195424c3da2ef1829a63b807e3a900a90e57d85
-- 
2.33.0