Re: [PATCH 0/2] sync_regs() TOCTOU issues

[Sean Christopherson <seanjc@google.com>]

On Wed, Aug 02, 2023, Sean Christopherson wrote:
> On Fri, 28 Jul 2023 02:12:56 +0200, Michal Luczaj wrote:
> > Both __set_sregs() and kvm_vcpu_ioctl_x86_set_vcpu_events() assume they
> > have exclusive rights to structs they operate on. While this is true when
> > coming from an ioctl handler (caller makes a local copy of user's data),
> > sync_regs() breaks this contract; a pointer to a user-modifiable memory
> > (vcpu->run->s.regs) is provided. This can lead to a situation when incoming
> > data is checked and/or sanitized only to be re-set by a user thread running
> > in parallel.
> > 
> > [...]
> 
> Applied to kvm-x86 selftests (there are in-flight reworks for selftests
> that will conflict, and I didn't want to split the testcases from the fix).
> 
> As mentioned in my reply to patch 2, I split up the selftests patch and
> massaged things a bit.  Please holler if you disagree with any of the
> changes.
> 
> Thanks much!
> 
> [1/4] KVM: x86: Fix KVM_CAP_SYNC_REGS's sync_regs() TOCTOU issues
>       https://github.com/kvm-x86/linux/commit/0d033770d43a
> [2/4] KVM: selftests: Extend x86's sync_regs_test to check for CR4 races
>       https://github.com/kvm-x86/linux/commit/ae895cbe613a
> [3/4] KVM: selftests: Extend x86's sync_regs_test to check for event vector races
>       https://github.com/kvm-x86/linux/commit/60c4063b4752
> [4/4] KVM: selftests: Extend x86's sync_regs_test to check for exception races
>       https://github.com/kvm-x86/linux/commit/0de704d2d6c8

Argh, apparently I didn't run these on AMD.  The exception injection test hangs
because the vCPU hits triple fault shutdown, and because the VMCB is technically
undefined on shutdown, KVM synthesizes INIT.  That starts the vCPU at the reset
vector and it happily fetches zeroes util being killed.

This fixes the issue, and I confirmed all three testcases repro the KVM bug with
it.  I'll post formally tomorrow.

---
 .../testing/selftests/kvm/x86_64/sync_regs_test.c | 15 ++++++++++++++-
 1 file changed, 14 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..55e9b68e6947 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -94,6 +94,7 @@ static void *race_events_inj_pen(void *arg)
 	for (;;) {
 		WRITE_ONCE(run->kvm_dirty_regs, KVM_SYNC_X86_EVENTS);
 		WRITE_ONCE(events->flags, 0);
+		WRITE_ONCE(events->exception.nr, GP_VECTOR);
 		WRITE_ONCE(events->exception.injected, 1);
 		WRITE_ONCE(events->exception.pending, 1);
 
@@ -115,6 +116,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, GP_VECTOR);
 		WRITE_ONCE(events->exception.pending, 1);
 		WRITE_ONCE(events->exception.nr, 255);
 
@@ -152,6 +154,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;
@@ -178,8 +181,17 @@ static void race_sync_regs(void *racer)
 
 	TEST_ASSERT_EQ(pthread_create(&thread, NULL, racer, (void *)run), 0);
 
+	state = vcpu_save_state(vcpu);
+
 	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 +202,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);
 }
 

base-commit: 722b2afc50abbfaa74accbc52911f9b5e8719c95
--