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

Jijie Shao <[email protected]> writes:

> 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]

A bit late to the game (just got back from vacation), but LGTM:

Reviewed-by: Toke Høiland-Jørgensen <[email protected]>