[PATCH 5.15 0/2] KVM: x86: Fix for dirty logging emulated atomics

[PATCH 5.15 0/2] KVM: x86: Fix for dirty logging emulated atomics

[Sean Christopherson]

Two KVM x86 backports for 5.15.  Patch 2 is the primary motivation (fix
for potential guest data corruption after live migration).

Patch 1 is a (very) soft dependency to resolve a conflict.  It's not strictly
necessary (manually resolving the conflict wouldn't be difficult), but it
is a fix that has been in upstream for a long time.  The only reason I didn't
tag it for stable from the get-go is that the bug it fixes is very
theoretical.  At this point, the odds of the patch causing problems are
lower than the odds of me botching a manual backport.

Sean Christopherson (2):
  KVM: x86: Bail to userspace if emulation of atomic user access faults
  KVM: x86: Mark target gfn of emulated atomic instruction as dirty

 arch/x86/kvm/x86.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)


base-commit: 9465fef4ae351749f7068da8c78af4ca27e61928
-- 
2.44.0.478.gd926399ef9-goog

[PATCH 5.15 1/2] KVM: x86: Bail to userspace if emulation of atomic user access faults

[Sean Christopherson]

Upstream commit 5d6c7de6446e9ab3fb41d6f7d82770e50998f3de.

Exit to userspace when emulating an atomic guest access if the CMPXCHG on
the userspace address faults.  Emulating the access as a write and thus
likely treating it as emulated MMIO is wrong, as KVM has already
confirmed there is a valid, writable memslot.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Message-Id: <20220202004945.2540433-6-seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index aa6f700f8c5f..a9c26397dcfd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7105,7 +7105,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
 	}
 
 	if (r < 0)
-		goto emul_write;
+		return X86EMUL_UNHANDLEABLE;
 	if (r)
 		return X86EMUL_CMPXCHG_FAILED;
 
-- 
2.44.0.478.gd926399ef9-goog

[PATCH 5.15 2/2] KVM: x86: Mark target gfn of emulated atomic instruction as dirty

[Sean Christopherson]

Upstream commit 910c57dfa4d113aae6571c2a8b9ae8c430975902.

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")
Cc: stable@vger.kernel.org
Cc: David Matlack <dmatlack@google.com>
Cc: Pasha Tatashin <tatashin@google.com>
Cc: Michael Krebs <mkrebs@google.com>
base-commit: 6769ea8da8a93ed4630f1ce64df6aafcaabfce64
Reviewed-by: Jim Mattson <jmattson@google.com>
Link: https://lore.kernel.org/r/20240215010004.1456078-2-seanjc@google.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a9c26397dcfd..dc0a7b9469e3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7106,6 +7106,16 @@ 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;
 
-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH 5.15 0/2] KVM: x86: Fix for dirty logging emulated atomics

[Greg Kroah-Hartman]

On Thu, Apr 04, 2024 at 04:40:02PM -0700, Sean Christopherson wrote:
> Two KVM x86 backports for 5.15.  Patch 2 is the primary motivation (fix
> for potential guest data corruption after live migration).
> 
> Patch 1 is a (very) soft dependency to resolve a conflict.  It's not strictly
> necessary (manually resolving the conflict wouldn't be difficult), but it
> is a fix that has been in upstream for a long time.  The only reason I didn't
> tag it for stable from the get-go is that the bug it fixes is very
> theoretical.  At this point, the odds of the patch causing problems are
> lower than the odds of me botching a manual backport.
> 
> Sean Christopherson (2):
>   KVM: x86: Bail to userspace if emulation of atomic user access faults
>   KVM: x86: Mark target gfn of emulated atomic instruction as dirty
> 
>  arch/x86/kvm/x86.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 9465fef4ae351749f7068da8c78af4ca27e61928
> -- 
> 2.44.0.478.gd926399ef9-goog

All now queued up, thanks.

greg k-h