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

[Mina Almasry]

On Wed, Jul 29, 2026 at 4:03 AM Jijie Shao <shaojijie@xxxxxxxxxx> wrote:
>
> 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:
>
> 1. Cache dma_addr to a local variable before xa_cmpxchg() in
>    __page_pool_release_netmem_dma(), so the scrub winner uses the
>    cached address for dma_unmap and never touches netmem fields after
>    the cmpxchg.
>
> 2. Rename page_pool_release_dma_index() to page_pool_remove_dma_mapping()
>    and strip it down to a pure cmpxchg wrapper; it no longer clears DMA
>    index bits.
>
> 3. Move dma_addr and DMA index cleanup to page_pool_return_netmem(),
>    which holds a page ref and can safely write netmem fields regardless
>    of the cmpxchg outcome.
>
> The scrub path (no ref) only does cmpxchg + dma_unmap on the cached
> address; the unref path (holds ref) clears dma_addr and DMA index bits
> unconditionally before put_page().
>
> Suggested-by: Mina Almasry <almasrymina@xxxxxxxxxx>

Thanks for trying to give credit but I didn't do much but feed your
earlier iteration to gemini. I don't mind either way but you can
remove.

> Fixes: ee62ce7a1d90 ("page_pool: Track DMA-mapped pages and unmap them when destroying the pool")
> Assisted-by: OhMyOpenCode:GLM-5.2

I fed this to gemini and I got good feedback, but it needs improved
human readability.

> Signed-off-by: Jijie Shao <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 | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 21dc4a9c8714..b7f5cdb6c1c4 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -500,8 +500,8 @@ 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 int page_pool_remove_dma_mapping(struct page_pool *pool,
> +                                       netmem_ref netmem)

Not a readable name. IDK what 'removing dma mapping' means. It's not
doing an unmap in the current iteration. Pick a new human-readable
name based on the changes below:

>  {
>         struct page *old, *page = netmem_to_page(netmem);
>         unsigned long id;
> @@ -517,12 +517,7 @@ static int page_pool_release_dma_index(struct page_pool *pool,
>                 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 0;
> +       return (old == page) ? 0 : -1;

This function, AFAICT, has only 1 call site where this return value
decides whether to dma_unmap_page_attr. That's not easy for a human to
follow. Just put the dma_unmap_page_attrs in this function.

I asked gemini to apply my feedback and it gave me this (untested)
patch. Consider reviewing this one and putting it through your
testing/etc. Please review the comments around the code. Some look
outdated? It's untested:

```
net/core/page_pool.c
index 21dc4a9c8714d..628854b56315b 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -500,29 +500,34 @@ static int page_pool_register_dma_index(struct
page_pool *pool,
        return err;
 }

-static int page_pool_release_dma_index(struct page_pool *pool,
+static void page_pool_release_dma_index(struct page_pool *pool,
                                       netmem_ref netmem)
 {
        struct page *old, *page = netmem_to_page(netmem);
        unsigned long id;
+       dma_addr_t dma;
+
+       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);
+       if (old != page)
+               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 +733,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_set_dma_addr_netmem(netmem, 0);
+       page_pool_release_dma_index(pool, netmem);
 }

 /* Disconnects a page (from a page_pool).  API users can have a need
@@ -759,10 +753,17 @@ static void page_pool_return_netmem(struct
page_pool *pool, netmem_ref netmem)
        bool put;

        put = true;
-       if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
+       if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops) {
                put = pool->mp_ops->release_netmem(pool, netmem);
-       else
+       } else {
                __page_pool_release_netmem_dma(pool, netmem);
+               /* clear dma_addr/DMA index; safe because we hold a ref */
+               if (pool->dma_map) {
+                       page_pool_set_dma_addr_netmem(netmem, 0);
+                       if (likely(PP_DMA_INDEX_BITS))
+                               netmem_set_dma_index(netmem, 0);
+               }
+       }

        /* This may be the last page returned, releasing the pool, so
         * it is not safe to reference pool afterwards.
```



-- 
Thanks,
Mina