[PATCH 5.10.y] KVM: arm64: Retry fault if vma_lookup() results become invalid

[PATCH 5.10.y] KVM: arm64: Retry fault if vma_lookup() results become invalid

[Bjoern Doebel]

From: David Matlack <dmatlack@google.com>

commit 13ec9308a85702af7c31f3638a2720863848a7f2 upstream.

Read mmu_invalidate_seq before dropping the mmap_lock so that KVM can
detect if the results of vma_lookup() (e.g. vma_shift) become stale
before it acquires kvm->mmu_lock. This fixes a theoretical bug where a
VMA could be changed by userspace after vma_lookup() and before KVM
reads the mmu_invalidate_seq, causing KVM to install page table entries
based on a (possibly) no-longer-valid vma_shift.

Re-order the MMU cache top-up to earlier in user_mem_abort() so that it
is not done after KVM has read mmu_invalidate_seq (i.e. so as to avoid
inducing spurious fault retries).

This bug has existed since KVM/ARM's inception. It's unlikely that any
sane userspace currently modifies VMAs in such a way as to trigger this
race. And even with directed testing I was unable to reproduce it. But a
sufficiently motivated host userspace might be able to exploit this
race.

Fixes: 94f8e6418d39 ("KVM: ARM: Handle guest faults in KVM")
Cc: stable@vger.kernel.org
Reported-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
Reviewed-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20230313235454.2964067-1-dmatlack@google.com
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
[doebel: adjust to contextual and naming differences in 5.10]
Signed-off-by: Bjoern Doebel <doebel@amazon.de>
---
The fix is already present in every newer supported stable tree, so
5.10.y is the only one left without it:

  5.15.y  00f74003edf5 (backport)
  6.1.y   e1562cc202c9 (backport)
  6.6.y   and later contain the mainline commit 13ec9308a857 itself

Tested using the Amazon Linux kernel test suite.

 arch/arm64/kvm/mmu.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 26068456ec0f3..2cda74acada2e 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -769,6 +769,19 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		return -EFAULT;
 	}
 
+	/*
+	 * Permission faults just need to update the existing leaf entry,
+	 * and so normally don't require allocations from the memcache. The
+	 * only exception to this is when dirty logging is enabled at runtime
+	 * and a write fault needs to collapse a block entry into a table.
+	 */
+	if (fault_status != FSC_PERM || (logging_active && write_fault)) {
+		ret = kvm_mmu_topup_memory_cache(memcache,
+						 kvm_mmu_cache_min_pages(kvm));
+		if (ret)
+			return ret;
+	}
+
 	/* Let's check if we will get back a huge page backed by hugetlbfs */
 	mmap_read_lock(current->mm);
 	vma = find_vma_intersection(current->mm, hva, hva + 1);
@@ -818,32 +831,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		fault_ipa &= ~(vma_pagesize - 1);
 
 	gfn = fault_ipa >> PAGE_SHIFT;
-	mmap_read_unlock(current->mm);
 
 	/*
-	 * Permission faults just need to update the existing leaf entry,
-	 * and so normally don't require allocations from the memcache. The
-	 * only exception to this is when dirty logging is enabled at runtime
-	 * and a write fault needs to collapse a block entry into a table.
+	 * Read mmu_notifier_seq so that KVM can detect if the results of
+	 * find_vma_intersection() or gfn_to_pfn_prot() become stale prior to
+	 * acquiring kvm->mmu_lock.
+	 *
+	 * Rely on mmap_read_unlock() for an implicit smp_rmb(), which pairs
+	 * with the smp_wmb() in kvm_mmu_notifier_invalidate_range_end().
 	 */
-	if (fault_status != FSC_PERM || (logging_active && write_fault)) {
-		ret = kvm_mmu_topup_memory_cache(memcache,
-						 kvm_mmu_cache_min_pages(kvm));
-		if (ret)
-			return ret;
-	}
-
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
-	/*
-	 * Ensure the read of mmu_notifier_seq happens before we call
-	 * gfn_to_pfn_prot (which calls get_user_pages), so that we don't risk
-	 * the page we just got a reference to gets unmapped before we have a
-	 * chance to grab the mmu_lock, which ensure that if the page gets
-	 * unmapped afterwards, the call to kvm_unmap_hva will take it away
-	 * from us again properly. This smp_rmb() interacts with the smp_wmb()
-	 * in kvm_mmu_notifier_invalidate_<page|range_end>.
-	 */
-	smp_rmb();
+	mmap_read_unlock(current->mm);
 
 	pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
 	if (pfn == KVM_PFN_ERR_HWPOISON) {
-- 
2.51.0

Re: [PATCH 5.10.y] KVM: arm64: Retry fault if vma_lookup() results become invalid

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing read memory barrier (`smp_rmb()`) before resolving the guest physical address, which breaks the MMU notifier retry protocol.
--

--- Patch [1]: [PATCH 5.10.y] KVM: arm64: Retry fault if vma_lookup() results become invalid ---
commit f032c4129aeef677df2aa27f38db191cca6b6b3d
Author: David Matlack <dmatlack@google.com>

KVM: arm64: Retry fault if vma_lookup() results become invalid

This commit reads mmu_invalidate_seq before dropping the mmap_lock to detect
if vma_lookup() results become stale before acquiring kvm->mmu_lock. It also
moves the MMU cache top-up earlier in user_mem_abort() to avoid spurious
fault retries.

Link: https://lore.kernel.org/r/20230313235454.2964067-1-dmatlack@google.com

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 26068456ec0f3..2cda74acada2e 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> @@ -818,32 +831,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		fault_ipa &= ~(vma_pagesize - 1);
>  
>  	gfn = fault_ipa >> PAGE_SHIFT;
> -	mmap_read_unlock(current->mm);
>  
> -	/*
> -	 * Permission faults just need to update the existing leaf entry,
[ ... ]
> -	if (fault_status != FSC_PERM || (logging_active && write_fault)) {
[ ... ]
> -	}
> -
>  	mmu_seq = vcpu->kvm->mmu_notifier_seq;
> -	/*
> -	 * Ensure the read of mmu_notifier_seq happens before we call
> -	 * gfn_to_pfn_prot (which calls get_user_pages), so that we don't risk
> -	 * the page we just got a reference to gets unmapped before we have a
> -	 * chance to grab the mmu_lock, which ensure that if the page gets
> -	 * unmapped afterwards, the call to kvm_unmap_hva will take it away
> -	 * from us again properly. This smp_rmb() interacts with the smp_wmb()
> -	 * in kvm_mmu_notifier_invalidate_<page|range_end>.
> -	 */
> -	smp_rmb();
> +	/*
> +	 * Read mmu_notifier_seq so that KVM can detect if the results of
> +	 * find_vma_intersection() or gfn_to_pfn_prot() become stale prior to
> +	 * acquiring kvm->mmu_lock.
> +	 *
> +	 * Rely on mmap_read_unlock() for an implicit smp_rmb(), which pairs
> +	 * with the smp_wmb() in kvm_mmu_notifier_invalidate_range_end().
> +	 */
> +	mmap_read_unlock(current->mm);
>  
>  	pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);

[Severity: Critical]
Does removing the explicit smp_rmb() here in user_mem_abort() break the MMU
notifier retry protocol?

The comment states that we can rely on mmap_read_unlock() for an implicit
smp_rmb(). However, mmap_read_unlock() via up_read() typically provides only
RELEASE semantics.

RELEASE semantics prevent prior memory operations from being reordered past
the unlock, but they do not prevent subsequent memory reads from being
hoisted up before the unlock.

This means the CPU is permitted to reorder the lockless page table reads
inside gfn_to_pfn_prot() to execute before the read of kvm->mmu_notifier_seq
occurs.

If a concurrent MMU notifier (e.g. from page reclaim) unmaps the page and
increments the sequence counter between the hoisted PFN read and the
mmu_seq read, KVM would capture the new sequence number.

Would this cause the mmu_notifier_retry() check under mmu_lock to falsely
succeed, allowing KVM to map a freed host page into the guest's stage-2
page tables?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821090538.3493280-1-doebel@amazon.de?part=1

Re: [PATCH 5.10.y] KVM: arm64: Retry fault if vma_lookup() results become invalid

[Bjoern Doebel]

On Fri, Aug 21, 2026 at 09:14:39AM +0000, sashiko-bot@kernel.org wrote:
> > -	}
> > -
> >  	mmu_seq = vcpu->kvm->mmu_notifier_seq;
> > -	/*
> > -	 * Ensure the read of mmu_notifier_seq happens before we call
> > -	 * gfn_to_pfn_prot (which calls get_user_pages), so that we don't risk
> > -	 * the page we just got a reference to gets unmapped before we have a
> > -	 * chance to grab the mmu_lock, which ensure that if the page gets
> > -	 * unmapped afterwards, the call to kvm_unmap_hva will take it away
> > -	 * from us again properly. This smp_rmb() interacts with the smp_wmb()
> > -	 * in kvm_mmu_notifier_invalidate_<page|range_end>.
> > -	 */
> > -	smp_rmb();
> > +	/*
> > +	 * Read mmu_notifier_seq so that KVM can detect if the results of
> > +	 * find_vma_intersection() or gfn_to_pfn_prot() become stale prior to
> > +	 * acquiring kvm->mmu_lock.
> > +	 *
> > +	 * Rely on mmap_read_unlock() for an implicit smp_rmb(), which pairs
> > +	 * with the smp_wmb() in kvm_mmu_notifier_invalidate_range_end().
> > +	 */
> > +	mmap_read_unlock(current->mm);
> >  
> >  	pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
> 
> [Severity: Critical]
> Does removing the explicit smp_rmb() here in user_mem_abort() break the MMU
> notifier retry protocol?
> 
> The comment states that we can rely on mmap_read_unlock() for an implicit
> smp_rmb(). However, mmap_read_unlock() via up_read() typically provides only
> RELEASE semantics.

Note that this removal of smp_rmb() is not new in this backport but
verbatim taken from the mainline patch. It is also contained in all
previous backports of the patch.

I'm not an expert on the memory semantics here and open to suggestions.
But _if_ we consider this a problem, we would have to go back and review
mainline and the other backports as well.

Bjoern