Message-ID: <Zc5bx4p6z8e3CmKK@google.com>
Date: Thu, 15 Feb 2024 10:45:27 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: David Matlack <dmatlack@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Pasha Tatashin <tatashin@...gle.com>, Michael Krebs <mkrebs@...gle.com>
Subject: Re: [PATCH 1/2] KVM: x86: Mark target gfn of emulated atomic
 instruction as dirty

On Thu, Feb 15, 2024, David Matlack wrote:
> On Wed, Feb 14, 2024 at 5:00 PM Sean Christopherson <seanjc@...gle.com> wrote:
> >
> > When emulating an atomic access on behalf of the guest, mark the target
> > gfn dirty if the CMPXCHG by KVM is attempted and doesn't fault.  This
> > fixes a bug where KVM effectively corrupts guest memory during live
> > migration by writing to guest memory without informing userspace that the
> > page is dirty.
> >
> > Marking the page dirty got unintentionally dropped when KVM's emulated
> > CMPXCHG was converted to do a user access.  Before that, KVM explicitly
> > mapped the guest page into kernel memory, and marked the page dirty during
> > the unmap phase.
> >
> > Mark the page dirty even if the CMPXCHG fails, as the old data is written
> > back on failure, i.e. the page is still written.  The value written is
> > guaranteed to be the same because the operation is atomic, but KVM's ABI
> > is that all writes are dirty logged regardless of the value written.  And
> > more importantly, that's what KVM did before the buggy commit.
> >
> > Huge kudos to the folks on the Cc list (and many others), who did all the
> > actual work of triaging and debugging.
> >
> > Fixes: 1c2361f667f3 ("KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses")
> 
> I'm only half serious but... Should we just revert this commit?

No.

> This commit claims that kvm_vcpu_map() is unsafe because it can race
> with mremap(). But there are many other places where KVM uses
> kvm_vcpu_map() (e.g. nested VMX). It seems like KVM is just not
> compatible with mremap() until we address all the users of
> kvm_vcpu_map(). Patching _just_ emulator_cmpxchg_emulated() seems
> silly but maybe I'm missing some context on what led to commit
> 1c2361f667f3 being written.

The short version is that it's a rather trivial vector for userspace to trigger
UAF.  E.g. map non-refcounted memory into a guest and then unmap the memory.

We tried to fix the nVMX usage, but that proved to be impractical[1].  We haven't
forced the issue because it's not obvious that there's meaningful exposure in
practice, e.g. unless userspace is hiding regular memory from the kernel *and*
oversubscribing VMs, a benign userspace won't be affected.  But at the same time,
we don't have high confidence that the unsafe behavior can't be exploited in
practice.

What I am pushing for now is an off-by-default module param to let userspace
opt-in to unsafe mappings such as these[2].  Because if KVM starts allowing
non-refcounted struct page memory, the ability to exploit these flaws skyrockets.
(Though this reminds me that I need to take another look at whether or not allowing
non-refcounted struct page memory is actually necessary).

[1] https://lore.kernel.org/all/ZBEEQtmtNPaEqU1i@google.com
[2] https://lore.kernel.org/all/20230911021637.1941096-4-stevensd@google.com

> kvm_vcpu_map/unmap() might not be the best interface, but it serves as
> a common choke-point for mapping guest memory to access in KVM. This
> is helpful for avoiding missed dirty logging updates (obviously) and
> will be even more helpful if we add support for freezing guest memory
> and "KVM Userfault" (as discussed in the 1/3 PUCK). I think we all
> agree we should do more of this (common choke points), not less. If
> there's a usecase for mremap()ing guest memory, we should make
> kvm_vcpu_map() play nice with mmu_notifiers.

I agree, but KVM needs to __try_cmpxchg_user() use anyways, when updating guest
A/D bits in FNAME(update_accessed_dirty_bits)().  And that one we *definitely*
don't want to revert; see commit 2a8859f373b0 ("KVM: x86/mmu: do compare-and-exchange
of gPTE via the user address") for details on how broken the previous code was.

In other words, reverting to kvm_vcpu_{un,}map() *probably* isn't wildly unsafe,
but it also doesn't really buy us anything, and long term we have line of sight
to closing the holes for good.  And unlike the nVMX code, where it's reasonable
for KVM to disallow using non-refcounted memory for VMCS pages, disallowing such
memory for emulated atomic accesses is less reasonable.

Rather than revert, to make this more robust in the longer term, we can add a
wrapper in KVM to mark the gfn dirty.  I didn't do it here because I was hustling
to get this minimal fix posted.

E.g.

--
Subject: [PATCH] KVM: x86: Provide a wrapper for __try_cmpxchg_user() to mark
 the gfn dirty

Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 arch/x86/kvm/mmu/paging_tmpl.h |  4 ++--
 arch/x86/kvm/x86.c             | 25 +++++++++----------------
 arch/x86/kvm/x86.h             | 19 +++++++++++++++++++
 3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 4d4e98fe4f35..a8123406fe99 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -246,11 +246,11 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu,
 		if (unlikely(!walker->pte_writable[level - 1]))
 			continue;
 
-		ret = __try_cmpxchg_user(ptep_user, &orig_pte, pte, fault);
+		ret = kvm_try_cmpxchg_user(ptep_user, &orig_pte, pte, fault,
+					   vcpu, table_gfn);
 		if (ret)
 			return ret;
 
-		kvm_vcpu_mark_page_dirty(vcpu, table_gfn);
 		walker->ptes[level - 1] = pte;
 	}
 	return 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3ec9781d6122..bedb51fbbad3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7946,8 +7946,9 @@ static int emulator_write_emulated(struct x86_emulate_ctxt *ctxt,
 				   exception, &write_emultor);
 }
 
-#define emulator_try_cmpxchg_user(t, ptr, old, new) \
-	(__try_cmpxchg_user((t __user *)(ptr), (t *)(old), *(t *)(new), efault ## t))
+#define emulator_try_cmpxchg_user(t, ptr, old, new, vcpu, gfn)		  \
+	(kvm_try_cmpxchg_user((t __user *)(ptr), (t *)(old), *(t *)(new), \
+			      efault ## t, vcpu, gfn))
 
 static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 				     unsigned long addr,
@@ -7960,6 +7961,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 	u64 page_line_mask;
 	unsigned long hva;
 	gpa_t gpa;
+	gfn_t gfn;
 	int r;
 
 	/* guests cmpxchg8b have to be emulated atomically */
@@ -7990,18 +7992,19 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 
 	hva += offset_in_page(gpa);
 
+	gfn = gpa_to_gfn(gpa);
 	switch (bytes) {
 	case 1:
-		r = emulator_try_cmpxchg_user(u8, hva, old, new);
+		r = emulator_try_cmpxchg_user(u8, hva, old, new, vcpu, gfn);
 		break;
 	case 2:
-		r = emulator_try_cmpxchg_user(u16, hva, old, new);
+		r = emulator_try_cmpxchg_user(u16, hva, old, new, vcpu, gfn);
 		break;
 	case 4:
-		r = emulator_try_cmpxchg_user(u32, hva, old, new);
+		r = emulator_try_cmpxchg_user(u32, hva, old, new, vcpu, gfn);
 		break;
 	case 8:
-		r = emulator_try_cmpxchg_user(u64, hva, old, new);
+		r = emulator_try_cmpxchg_user(u64, hva, old, new, vcpu, gfn);
 		break;
 	default:
 		BUG();
@@ -8009,16 +8012,6 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 
 	if (r < 0)
 		return X86EMUL_UNHANDLEABLE;
-
-	/*
-	 * Mark the page dirty _before_ checking whether or not the CMPXCHG was
-	 * successful, as the old value is written back on failure.  Note, for
-	 * live migration, this is unnecessarily conservative as CMPXCHG writes
-	 * back the original value and the access is atomic, but KVM's ABI is
-	 * that all writes are dirty logged, regardless of the value written.
-	 */
-	kvm_vcpu_mark_page_dirty(vcpu, gpa_to_gfn(gpa));
-
 	if (r)
 		return X86EMUL_CMPXCHG_FAILED;
 
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 2f7e19166658..2fabc7cd7e39 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -290,6 +290,25 @@ static inline bool kvm_check_has_quirk(struct kvm *kvm, u64 quirk)
 	return !(kvm->arch.disabled_quirks & quirk);
 }
 
+
+
+/*
+ * Mark the page dirty even if the CMPXCHG fails (but didn't fault), as the old
+ * old value is written back on failure.  Note, for live migration, this is
+ * unnecessarily conservative as CMPXCHG writes back the original value and the
+ * access is atomic, but KVM's ABI is that all writes are dirty logged,
+ * regardless of the value written.
+ */
+#define kvm_try_cmpxchg_user(ptr, oldp, nval, label, vcpu, gfn) \
+({								\
+	int ret;						\
+								\
+	ret = __try_cmpxchg_user(ptr, oldp, nval, label);	\
+	if (ret >= 0)						\
+		kvm_vcpu_mark_page_dirty(vcpu, gfn);		\
+	ret;							\
+})
+
 void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
 
 u64 get_kvmclock_ns(struct kvm *kvm);

base-commit: 6769ea8da8a93ed4630f1ce64df6aafcaabfce64
-- 

Powered by blists - more mailing lists