Re: [PATCH mm-hotfixes v4 2/4] x86/mm/pat: acquire init_mm write lock to avoid UAF

[David CARLIER]

On Thu, 16 Jul 2026 at 22:31, Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
>
> x86 implements page attribute modification using its Change Page
> Attributes (CPA) mechanism.
>
> This tracks properties of ranges such as cache mode through x86 page
> attributes, and as part of that logic manipulates kernel page tables.
>
> Since commit 41d88484c71c ("x86/mm/pat: restore large ROX pages after
> fragmentation") ranges of kernel page table entries can be collapsed into
> huge page table entries as part of this logic.
>
> As part of this collapse, it frees the page tables which the collapsed
> entries previously pointed to, and it does so without any relevant locks
> being held to preclude concurrent kernel page table walkers.
>
> The only way this code can be reached is if CPA_COLLAPSE is specified, and
> this is only set in set_memory_rox() via:
>
> set_memory_rox()
> -> change_page_attr_set_clr()
> -> cpa_flush()
> -> cpa_collapse_large_pages()
>
> Notable users of this are execmem and bpf when manipulating executable
> mappings.
>
> However, this is problematic for ptdump as it walks ranges it does not own
> and thus runs the risk of a use-after-free on page tables freed underneath
> it.
>
> In addition, concurrent CPA collapse operations are possible which can also
> cause races.
>
> Resolve the issue by acquiring the mmap write lock on init_mm across the
> whole operation.
>
> It is safe to acquire a sleeping lock as all the callers invoke
> set_memory_rox() from process context and in any case,
> change_page_attr_set_clr() calls vm_unmap_alias() which ultimately takes a
> mutex, disallowing atomic context here.
>
> Fixes: 41d88484c71c ("x86/mm/pat: restore large ROX pages after fragmentation")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Reviewed-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> ---
>  arch/x86/mm/pat/set_memory.c | 15 ++++++++++++++-
>  include/linux/mmap_lock.h    |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index d023a40a1e03..d1e63f7d267f 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -22,6 +22,7 @@
>  #include <linux/cc_platform.h>
>  #include <linux/set_memory.h>
>  #include <linux/memregion.h>
> +#include <linux/cleanup.h>
>
>  #include <asm/e820/api.h>
>  #include <asm/processor.h>
> @@ -410,7 +411,7 @@ static void __cpa_flush_tlb(void *data)
>
>  static int collapse_large_pages(unsigned long addr, struct list_head *pgtables);
>
> -static void cpa_collapse_large_pages(struct cpa_data *cpa)
> +static void __cpa_collapse_large_pages(struct cpa_data *cpa)
>  {
>         unsigned long start, addr, end;
>         struct ptdesc *ptdesc, *tmp;
> @@ -442,6 +443,18 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa)
>         }
>  }
>
> +static void cpa_collapse_large_pages(struct cpa_data *cpa)
> +{
> +       /*
> +        * Take the mmap write lock on init_mm to:
> +        * - Avoid a use-after-free if raced by ptdump (which takes its own
> +        *   write lock on init_mm).
> +        * - Serialise concurrent CPA walkers.
> +        */
> +       scoped_guard(mmap_write_lock, &init_mm)
> +               __cpa_collapse_large_pages(cpa);
> +}
> +
>  static void cpa_flush(struct cpa_data *cpa, int cache)
>  {
>         unsigned int i;
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index 6b5c2390cc30..047f5f5e2c34 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -621,6 +621,8 @@ static inline void mmap_read_unlock(struct mm_struct *mm)
>
>  DEFINE_GUARD(mmap_read_lock, struct mm_struct *,
>              mmap_read_lock(_T), mmap_read_unlock(_T))
> +DEFINE_GUARD(mmap_write_lock, struct mm_struct *,
> +            mmap_write_lock(_T), mmap_write_unlock(_T))
>  DEFINE_GUARD_COND(mmap_read_lock, _try, mmap_read_trylock(_T))
>
>  static inline void mmap_read_unlock_non_owner(struct mm_struct *mm)
>
> --
> 2.55.0
>

Hi,

This bullet confuses me. __change_page_attr() and friends are serialised by
cpa_lock/pgd_lock and never take init_mm's mmap lock, so this doesn't
exclude them.

Collapse vs collapse looks covered already: collapse_large_pages() holds
pgd_lock across the walk and both collapse_pmd_page()/collapse_pud_page()
calls, and the pmd_leaf()/pud_leaf() early-outs stop a second CPU
re-queueing a collapsed table. I couldn't build the interleaving behind the
cover letter's "could result in memory corruption". Can you spell it out?

If the reason is really Will's v3 point - collapse rounds up to PMD/PUD and
frees tables outside its caller's range, so it has to exclude
walk_kernel_page_table_range() walkers holding the read lock - then say
that. It's a UAF independent of ptdump, and it's why v3's read lock wasn't
enough.

Also: patch 1 justifies the trylock fallback on ptdump being rare, but this
takes the write lock on every set_memory_rox() (BPF JIT, module load,
execmem), even when nothing collapsed. vmap_try_huge_*() then loses the
trylock and quietly falls back to PTEs on every BPF load. No counter, no
trace. Not a blocker and I've no better idea, but fa93b45fd397 was about
getting vmalloc-huge on by default, so it's worth calling out.

Rest looks right. 4/4 restores pud_free_pmd_page() exactly as it was before
fa93b45fd397, and 3/4 builds the same walk init_mm got via
walk_kernel_page_table_range_lockless().

Reviewed-by: David Carlier <devnexen@xxxxxxxxx>

Cheers !