Received-SPF: pass (google.com: domain of linux-kernel+bounces-67533-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 15 Feb 2024 10:45:27 -0800
In-Reply-To: <CALzav=c0MFB7UG7yaXB3bAFampYO_xN=5Pjao6La55wy4cwjSw@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240215010004.1456078-1-seanjc@google.com> <20240215010004.1456078-2-seanjc@google.com>
 <CALzav=c0MFB7UG7yaXB3bAFampYO_xN=5Pjao6La55wy4cwjSw@mail.gmail.com>
Message-ID: <Zc5bx4p6z8e3CmKK@google.com>
Subject: Re: [PATCH 1/2] KVM: x86: Mark target gfn of emulated atomic
 instruction as dirty
From: Sean Christopherson <seanjc@google.com>
To: David Matlack <dmatlack@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Pasha Tatashin <tatashin@google.com>, Michael Krebs <mkrebs@google.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 15, 2024, David Matlack wrote:
> On Wed, Feb 14, 2024 at 5:00=E2=80=AFPM Sean Christopherson <seanjc@googl=
e.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 t=
he
> > 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 dur=
ing
> > the unmap phase.
> >
> > Mark the page dirty even if the CMPXCHG fails, as the old data is writt=
en
> > 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 AB=
I
> > is that all writes are dirty logged regardless of the value written.  A=
nd
> > 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 t=
he
> > actual work of triaging and debugging.
> >
> > Fixes: 1c2361f667f3 ("KVM: x86: Use __try_cmpxchg_user() to emulate ato=
mic accesses")
>=20
> 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 tri=
gger
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 h=
aven'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 *a=
nd*
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 i=
n
practice.

What I am pushing for now is an off-by-default module param to let userspac=
e
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 skyro=
ckets.
(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.co=
m

> 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 g=
uest
A/D bits in FNAME(update_accessed_dirty_bits)().  And that one we *definite=
ly*
don't want to revert; see commit 2a8859f373b0 ("KVM: x86/mmu: do compare-an=
d-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 un=
safe,
but it also doesn't really buy us anything, and long term we have line of s=
ight
to closing the holes for good.  And unlike the nVMX code, where it's reason=
able
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 hu=
stling
to get this minimal fix posted.

E.g.

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

Signed-off-by: Sean Christopherson <seanjc@google.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 k=
vm_vcpu *vcpu,
 		if (unlikely(!walker->pte_writable[level - 1]))
 			continue;
=20
-		ret =3D __try_cmpxchg_user(ptep_user, &orig_pte, pte, fault);
+		ret =3D kvm_try_cmpxchg_user(ptep_user, &orig_pte, pte, fault,
+					   vcpu, table_gfn);
 		if (ret)
 			return ret;
=20
-		kvm_vcpu_mark_page_dirty(vcpu, table_gfn);
 		walker->ptes[level - 1] =3D 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);
 }
=20
-#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))
=20
 static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 				     unsigned long addr,
@@ -7960,6 +7961,7 @@ static int emulator_cmpxchg_emulated(struct x86_emula=
te_ctxt *ctxt,
 	u64 page_line_mask;
 	unsigned long hva;
 	gpa_t gpa;
+	gfn_t gfn;
 	int r;
=20
 	/* guests cmpxchg8b have to be emulated atomically */
@@ -7990,18 +7992,19 @@ static int emulator_cmpxchg_emulated(struct x86_emu=
late_ctxt *ctxt,
=20
 	hva +=3D offset_in_page(gpa);
=20
+	gfn =3D gpa_to_gfn(gpa);
 	switch (bytes) {
 	case 1:
-		r =3D emulator_try_cmpxchg_user(u8, hva, old, new);
+		r =3D emulator_try_cmpxchg_user(u8, hva, old, new, vcpu, gfn);
 		break;
 	case 2:
-		r =3D emulator_try_cmpxchg_user(u16, hva, old, new);
+		r =3D emulator_try_cmpxchg_user(u16, hva, old, new, vcpu, gfn);
 		break;
 	case 4:
-		r =3D emulator_try_cmpxchg_user(u32, hva, old, new);
+		r =3D emulator_try_cmpxchg_user(u32, hva, old, new, vcpu, gfn);
 		break;
 	case 8:
-		r =3D emulator_try_cmpxchg_user(u64, hva, old, new);
+		r =3D emulator_try_cmpxchg_user(u64, hva, old, new, vcpu, gfn);
 		break;
 	default:
 		BUG();
@@ -8009,16 +8012,6 @@ static int emulator_cmpxchg_emulated(struct x86_emul=
ate_ctxt *ctxt,
=20
 	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;
=20
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);
 }
=20
+
+
+/*
+ * Mark the page dirty even if the CMPXCHG fails (but didn't fault), as th=
e old
+ * old value is written back on failure.  Note, for live migration, this i=
s
+ * unnecessarily conservative as CMPXCHG writes back the original value an=
d 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 =3D __try_cmpxchg_user(ptr, oldp, nval, label);	\
+	if (ret >=3D 0)						\
+		kvm_vcpu_mark_page_dirty(vcpu, gfn);		\
+	ret;							\
+})
+
 void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc=
_eip);
=20
 u64 get_kvmclock_ns(struct kvm *kvm);

base-commit: 6769ea8da8a93ed4630f1ce64df6aafcaabfce64
--=20