Re: [PATCH 3/5] x86/mm/pat: acquire init_mm read lock on attribute change to avoid UAF

[Lorenzo Stoakes (ARM)]

On Tue, Jul 28, 2026 at 04:07:46PM +0300, Mike Rapoport wrote:
> From: "Lorenzo Stoakes (ARM)" <ljs@xxxxxxxxxx>
>
> A previous commit protected us against races between ptdump and CPA
> collapse, however one still exists between attribute changes and collapse
> as reported by Denis V. Lunev (linked).
>
> When an attribute change arises, a lockless page table walker obtains a PTE
> entry, which is later written to via set_pte_atomic():
>
> ...
> -> change_page_attr_set_clr()
> -> __change_page_attr_set_clr()
> -> __change_page_attr()
> 	-> _lookup_address_cpa()
> 	-> lookup_address_in_pgd_attr()
> 	-> [ lockless page table walker ]
> -> set_pte_atomic()
>
> There is nothing preventing a concurrent CPA collapse which can free the
> PTE that was retrieved here, resulting in a use-after-free.
>
> With the mmap write lock taken on init_mm over CPA collapse, we can now
> resolve this race by acquiring an mmap read lock on init_mm over
> __change_page_attr_set_clr().
>
> This locks across the whole operation over which the walk and the PTE entry
> write occurs, solving the race.
>
> It is safe to do this here, as no spinlocks are held upon entry to
> __change_page_attr_set_clr().
>
> However, the lock must not be held over an allocation, as allocation can
> trigger reclaim and shrinkers may call into CPA recursively, making
> deadlocks possible (init_mm -> ... -> fs_reclaim -> init_mm).
>
> A page table is allocated when a huge page needs to be split:
>
> -> change_page_attr_set_clr()
> -> __change_page_attr_set_clr()
> -> __change_page_attr()
> -> split_large_page()
> [ pagetable_alloc() ]
> -> __split_large_page()
>
> Avoid deadlocks by dropping the mmap lock across pagetable_alloc() in
> split_large_page() and track whether this is needed by adding a new
> 'init_mm_read_locked' flag to struct cpa_data.
>
> This is safe as __split_large_page() (called with locks re-established)
> revalidates that the page table entry is the same as it was prior to the
> locks being dropped and __change_page_attr() repeats the entire page table
> walk whenever a split occurs, so concurrent split and collapse are
> accounted for.
>
> Concurrent ptdump is also safe as the lock is only dropped over page table
> allocation during which time the page table has not yet been modified.
>
> The CPA_COLLAPSE flag is only set by set_memory_rox(), which exclusively
> operates upon vmalloc ranges, and on x86 only within the module mapping
> space.
>
> This is important, because some callers directly invoke
> __change_page_attr_set_clr(), bypassing this lock. However, none of these
> operate within the module mapping space.
>
> * cpa_process_alias() - a recursive helper called by
>   __change_page_attr_set_clr().
> * __set_memory_enc_pgtable() - operates on the direct mapping and (via
>   __vmbus_establish_gpadl()) the vmalloc mapping space.
> * __set_pages_[n]p() - called by set_direct_map_[invalid, default,
>   valid]_noflush(), __kernel_map_pages() - operates on the direct map.
> * kernel_[un]map_pages_in_pgd() - operates on EFI ranges.
>
> This work is based upon Denis V. Lunev's excellent analysis of the bug with
> gratitude.
>
> Link: https://lore.kernel.org/all/20260626163213.2284080-1-den@xxxxxxxxxx/
> Fixes: 41d88484c71c ("x86/mm/pat: restore large ROX pages after fragmentation")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> ---
>  arch/x86/mm/pat/set_memory.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 4ba16d72a535..26131ecc0e1c 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -50,7 +50,8 @@ struct cpa_data {
>  	unsigned int	flags;
>  	unsigned int	force_split		: 1,
>  			force_static_prot	: 1,
> -			force_flush_all		: 1;
> +			force_flush_all		: 1,
> +			init_mm_read_locked	: 1;
>  	struct page	**pages;
>  };
>
> @@ -435,8 +436,6 @@ static void __cpa_collapse_large_pages(struct cpa_data *cpa)
>  	int collapsed = 0;
>  	int i;
>
> -	cpa_lock();
> -
>  	if (cpa->flags & (CPA_PAGES_ARRAY | CPA_ARRAY)) {
>  		for (i = 0; i < cpa->numpages; i++)
>  			collapsed += collapse_large_pages(__cpa_addr(cpa, i),
> @@ -450,10 +449,8 @@ static void __cpa_collapse_large_pages(struct cpa_data *cpa)
>  			collapsed += collapse_large_pages(addr, &pgtables);
>  	}
>
> -	if (!collapsed) {
> -		cpa_unlock();
> +	if (!collapsed)
>  		return;
> -	}
>
>  	flush_tlb_all();
>
> @@ -461,8 +458,6 @@ static void __cpa_collapse_large_pages(struct cpa_data *cpa)
>  		list_del(&ptdesc->pt_list);
>  		pagetable_free(ptdesc);
>  	}
> -
> -	cpa_unlock();
>  }
>
>  static void cpa_collapse_large_pages(struct cpa_data *cpa)
> @@ -1270,8 +1265,13 @@ static int split_large_page(struct cpa_data *cpa, pte_t *kpte,
>  	struct ptdesc *ptdesc;
>
>  	cpa_unlock();
> +	if (cpa->init_mm_read_locked)
> +		mmap_read_unlock(&init_mm);
>  	ptdesc = pagetable_alloc(GFP_KERNEL, 0);
> +	if (cpa->init_mm_read_locked)
> +		mmap_read_lock(&init_mm);
>  	cpa_lock();

Yeah this retains the correct ordering (sem -> spinlock) so LGTM!

> +
>  	if (!ptdesc)
>  		return -ENOMEM;
>
> @@ -2139,7 +2139,11 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages,
>  	cpa.curpage = 0;
>  	cpa.force_split = force_split;
>
> -	ret = __change_page_attr_set_clr(&cpa, 1);
> +	/* Avoid race with concurrent CPA collapse. */
> +	cpa.init_mm_read_locked = true;
> +	scoped_guard(mmap_read_lock, &init_mm)
> +		ret = __change_page_attr_set_clr(&cpa, 1);
> +	cpa.init_mm_read_locked = false;
>
>  	/*
>  	 * Check whether we really changed something:
>
> --
> 2.53.0
>

Cheers, Lorenzo