[PATCH 0/2] KVM: selftests: Fix sync_regs race tests on AMD

[PATCH 0/2] KVM: selftests: Fix sync_regs race tests on AMD

[Sean Christopherson]

The recently introduced sync_regs subtests to detect TOCTOU bugs hang on
AMD because KVM synthesizes INIT on triple fault shutdown (because AMD says
the VMCB is undefined after shutdown).  Fix it by loading known good state
if a shutdown occurs.

Patch two explicitly injects #UD as the "good" vector so that the test
doesn't rely on KVM inadvertantly setting the vector to #DE (old, buggy
behavior).

Sean Christopherson (2):
  KVM: selftests: Reload "good" vCPU state if vCPU hits shutdown
  KVM: selftests: Explicit set #UD when *potentially* injecting
    exception

 .../selftests/kvm/x86_64/sync_regs_test.c       | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)


base-commit: b6d40d24bb2eae6d4c2d4799ddbffcf7bae663db
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 1/2] KVM: selftests: Reload "good" vCPU state if vCPU hits shutdown

[Sean Christopherson]

Reload known good vCPU state if the vCPU triple faults in any of the
race_sync_regs() subtests, e.g. if KVM successfully injects an exception
(the vCPU isn't configured to handle exceptions).  On Intel, the VMCS
is preserved even after shutdown, but AMD's APM states that the VMCB is
undefined after a shutdown and so KVM synthesizes an INIT to sanitize
vCPU/VMCB state, e.g. to guard against running with a garbage VMCB.

The synthetic INIT results in the vCPU never exiting to userspace, as it
gets put into Real Mode at the reset vector, which is full of zeros (as is
GPA 0 and beyond), and so executes ADD for a very, very long time.

Fixes: 60c4063b4752 ("KVM: selftests: Extend x86's sync_regs_test to check for event vector races")
Cc: Michal Luczaj <mhal@rbox.co>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../testing/selftests/kvm/x86_64/sync_regs_test.c  | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
index 93fac74ca0a7..21e99dae2ff2 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -152,6 +152,7 @@ static noinline void *race_sregs_cr4(void *arg)
 static void race_sync_regs(void *racer)
 {
 	const time_t TIMEOUT = 2; /* seconds, roughly */
+	struct kvm_x86_state *state;
 	struct kvm_translation tr;
 	struct kvm_vcpu *vcpu;
 	struct kvm_run *run;
@@ -166,6 +167,9 @@ static void race_sync_regs(void *racer)
 	vcpu_run(vcpu);
 	run->kvm_valid_regs = 0;
 
+	/* Save state *before* spawning the thread that mucks with vCPU state. */
+	state = vcpu_save_state(vcpu);
+
 	/*
 	 * Selftests run 64-bit guests by default, both EFER.LME and CR4.PAE
 	 * should already be set in guest state.
@@ -179,7 +183,14 @@ static void race_sync_regs(void *racer)
 	TEST_ASSERT_EQ(pthread_create(&thread, NULL, racer, (void *)run), 0);
 
 	for (t = time(NULL) + TIMEOUT; time(NULL) < t;) {
-		__vcpu_run(vcpu);
+		/*
+		 * Reload known good state if the vCPU triple faults, e.g. due
+		 * to the unhandled #GPs being injected.  VMX preserves state
+		 * on shutdown, but SVM synthesizes an INIT as the VMCB state
+		 * is architecturally undefined on triple fault.
+		 */
+		if (!__vcpu_run(vcpu) && run->exit_reason == KVM_EXIT_SHUTDOWN)
+			vcpu_load_state(vcpu, state);
 
 		if (racer == race_sregs_cr4) {
 			tr = (struct kvm_translation) { .linear_address = 0 };
@@ -190,6 +201,7 @@ static void race_sync_regs(void *racer)
 	TEST_ASSERT_EQ(pthread_cancel(thread), 0);
 	TEST_ASSERT_EQ(pthread_join(thread, NULL), 0);
 
+	kvm_x86_state_cleanup(state);
 	kvm_vm_free(vm);
 }
 
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 2/2] KVM: selftests: Explicit set #UD when *potentially* injecting exception

[Sean Christopherson]

Explicitly set the exception vector to #UD when potentially injecting an
exception in sync_regs_test's subtests that try to detect TOCTOU bugs
in KVM's handling of exceptions injected by userspace.  A side effect of
the original KVM bug was that KVM would clear the vector, but relying on
KVM to clear the vector (i.e. make it #DE) makes it less likely that the
test would ever find *new* KVM bugs, e.g. because only the first iteration
would run with a legal vector to start.

Explicitly inject #UD for race_events_inj_pen() as well, e.g. so that it
doesn't inherit the illegal 255 vector from race_events_exc(), which
currently runs first.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/x86_64/sync_regs_test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
index 21e99dae2ff2..00965ba33f73 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -91,6 +91,8 @@ static void *race_events_inj_pen(void *arg)
 	struct kvm_run *run = (struct kvm_run *)arg;
 	struct kvm_vcpu_events *events = &run->s.regs.events;
 
+	WRITE_ONCE(events->exception.nr, UD_VECTOR);
+
 	for (;;) {
 		WRITE_ONCE(run->kvm_dirty_regs, KVM_SYNC_X86_EVENTS);
 		WRITE_ONCE(events->flags, 0);
@@ -115,6 +117,7 @@ static void *race_events_exc(void *arg)
 	for (;;) {
 		WRITE_ONCE(run->kvm_dirty_regs, KVM_SYNC_X86_EVENTS);
 		WRITE_ONCE(events->flags, 0);
+		WRITE_ONCE(events->exception.nr, UD_VECTOR);
 		WRITE_ONCE(events->exception.pending, 1);
 		WRITE_ONCE(events->exception.nr, 255);
 
-- 
2.42.0.rc1.204.g551eb34607-goog

Re: [PATCH 0/2] KVM: selftests: Fix sync_regs race tests on AMD

[Sean Christopherson]

On Thu, 17 Aug 2023 16:34:28 -0700, Sean Christopherson wrote:
> The recently introduced sync_regs subtests to detect TOCTOU bugs hang on
> AMD because KVM synthesizes INIT on triple fault shutdown (because AMD says
> the VMCB is undefined after shutdown).  Fix it by loading known good state
> if a shutdown occurs.
> 
> Patch two explicitly injects #UD as the "good" vector so that the test
> doesn't rely on KVM inadvertantly setting the vector to #DE (old, buggy
> behavior).
> 
> [...]

Applied to kvm-x86 selftests.

[1/2] KVM: selftests: Reload "good" vCPU state if vCPU hits shutdown
      https://github.com/kvm-x86/linux/commit/5002b112a5ad
[2/2] KVM: selftests: Explicit set #UD when *potentially* injecting exception
      https://github.com/kvm-x86/linux/commit/02dc2543e379

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes