[PATCH 0/2] KVM: nVMX: Fix a (benign) overwrite to a userspace buffer

[PATCH 0/2] KVM: nVMX: Fix a (benign) overwrite to a userspace buffer

[Sean Christopherson]

Fix a bug where KVM writes more bytes for KVM_GET_NESTED_STATE than it reports
in the output size if L1 is active and has a valid shadow VMCS pointer.  The
bug is all but guaranteed to be benign in practice, as all VMMs I could find
that support nested virtualization just preallocate the max buffer size.

Sean Christopherson (2):
  KVM: nVMX: Save shadow VMCS when getting nested state iff L2 is active
  KVM: selftests: Add regression test for KVM writing but not reporting
    vmcs12

 arch/x86/kvm/vmx/nested.c                     |  2 +-
 .../testing/selftests/kvm/lib/x86/processor.c |  6 +++++
 tools/testing/selftests/kvm/x86/state_test.c  | 24 ++++++++++---------
 3 files changed, 20 insertions(+), 12 deletions(-)


base-commit: 164405e8e9a0378c7e1aff2254625a6553a291f1
-- 
2.56.0.218.g1ec904b173-goog

[PATCH 1/2] KVM: nVMX: Save shadow VMCS when getting nested state iff L2 is active

[Sean Christopherson]

When getting nested state, copy the shadow VMCS to the userspace buffer if
and only if the vCPU is in L2, as that is the only scenario where the
shadow VMCS is "active" and thus needs to be explicitly save/restored via
KVM_GET_NESTED_STATE.  If the vCPU is in L1, then the shadow VMCS isn't
active/cached by KVM, i.e. is just a pointer to memory, and doesn't need
to be saved/restored (userspace is responsible for saving/restore guest
memory as needed).

This fixes a bug where KVM would save the shadow VMCS, but not account the
size of the shadow VMCS, as the accounting correctly checks for L2 being
active.  Note, the restore path also consumes the shadow VMCS payload if
and only if L2 is active.

In practice, writing more bytes than are reported is likely benign, as KVM
correctly reports the maximum size for KVM_CAP_NESTED_STATE, and no known
VMM invokes KVM_GET_NESTED_STATE twice on every save to precisely allocate
its buffer, e.g. QEMU and Google's VMM both allocate the maximum size when
the vCPU is first created/allocated.

Fixes: fa58a9fa7497 ("KVM: nVMX: include shadow vmcs12 in nested state")
Reported-by: Josef Bacik <josef@toxicpanda.com>
Cc: Chris Mason <mason@kernel.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/nested.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 9b0bfa2f854c..c2586bc9ebb5 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6875,7 +6875,7 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
 	if (copy_to_user(user_vmx_nested_state->vmcs12, vmcs12, VMCS12_SIZE))
 		return -EFAULT;
 
-	if (nested_cpu_has_shadow_vmcs(vmcs12) &&
+	if (is_guest_mode(vcpu) && nested_cpu_has_shadow_vmcs(vmcs12) &&
 	    vmcs12->vmcs_link_pointer != INVALID_GPA) {
 		if (copy_to_user(user_vmx_nested_state->shadow_vmcs12,
 				 get_shadow_vmcs12(vcpu), VMCS12_SIZE))
-- 
2.56.0.218.g1ec904b173-goog

[PATCH 2/2] KVM: selftests: Add regression test for KVM writing but not reporting vmcs12

[Sean Christopherson]

When saving nested vCPU state, verify that KVM didn't write any more bytes
than it reported, and add a sync+save point in the state test to trigger a
save when L1 is active with a valid shadow VMCS.  Before commit <TBD>, KVM
would incorrectly save the shadow VMCS into the userspace buffer without
accounting for the size of the shadow VMCS.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../testing/selftests/kvm/lib/x86/processor.c |  6 +++++
 tools/testing/selftests/kvm/x86/state_test.c  | 24 ++++++++++---------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/x86/processor.c b/tools/testing/selftests/kvm/lib/x86/processor.c
index 8374afd08baf..976d53af6de7 100644
--- a/tools/testing/selftests/kvm/lib/x86/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86/processor.c
@@ -1282,10 +1282,16 @@ struct kvm_x86_state *vcpu_save_state(struct kvm_vcpu *vcpu)
 	if (nested_size) {
 		state->nested.size = sizeof(state->nested_);
 
+		memset(state->nested_, 0xaa, sizeof(state->nested_));
+
 		vcpu_nested_state_get(vcpu, &state->nested);
 		TEST_ASSERT(state->nested.size <= nested_size,
 			    "Nested state size too big, %i (KVM_CHECK_CAP gave %i)",
 			    state->nested.size, nested_size);
+
+		for (i = state->nested.size; i < nested_size; i++)
+			TEST_ASSERT((unsigned char)state->nested_[i] == 0xaa,
+				    "KVM killed the canary at offset %u", i);
 	} else {
 		state->nested.size = 0;
 	}
diff --git a/tools/testing/selftests/kvm/x86/state_test.c b/tools/testing/selftests/kvm/x86/state_test.c
index 500b09762555..5bd9a04a3911 100644
--- a/tools/testing/selftests/kvm/x86/state_test.c
+++ b/tools/testing/selftests/kvm/x86/state_test.c
@@ -60,13 +60,13 @@ void vmx_l2_guest_code(void)
 
 	/* L1 has now set up a shadow VMCS for us.  */
 	GUEST_ASSERT(vmread(GUEST_RIP) == 0xc0ffee);
-	GUEST_SYNC(10);
-	GUEST_ASSERT(vmread(GUEST_RIP) == 0xc0ffee);
-	vmwrite(GUEST_RIP, 0xc0fffee);
 	GUEST_SYNC(11);
+	GUEST_ASSERT(vmread(GUEST_RIP) == 0xc0ffee);
+	vmwrite(GUEST_RIP, 0xc0fffee);
+	GUEST_SYNC(12);
 	GUEST_ASSERT(vmread(GUEST_RIP) == 0xc0fffee);
 	vmwrite(GUEST_RIP, 0xc0ffffee);
-	GUEST_SYNC(12);
+	GUEST_SYNC(13);
 
 	/* Done, exit to L1 and never come back.  */
 	vmcall();
@@ -107,15 +107,17 @@ static void vmx_l1_guest_code(struct vmx_pages *vmx_pages)
 
 	vmwrite(SECONDARY_VM_EXEC_CONTROL, SECONDARY_EXEC_SHADOW_VMCS);
 	vmwrite(VMCS_LINK_POINTER, vmx_pages->shadow_vmcs_gpa);
-
-	vmptrld(vmx_pages->shadow_vmcs_gpa);
-	GUEST_ASSERT(__vmlaunch());
 	GUEST_SYNC(8);
-	GUEST_ASSERT(__vmlaunch());
-	GUEST_ASSERT(__vmresume());
 
-	vmwrite(GUEST_RIP, 0xc0ffee);
+	vmptrld(vmx_pages->shadow_vmcs_gpa);
+	GUEST_ASSERT(__vmlaunch());
 	GUEST_SYNC(9);
+
+	GUEST_ASSERT(__vmlaunch());
+	GUEST_ASSERT(__vmresume());
+
+	vmwrite(GUEST_RIP, 0xc0ffee);
+	GUEST_SYNC(10);
 	GUEST_ASSERT(vmread(GUEST_RIP) == 0xc0ffee);
 
 	vmptrld(vmx_pages->vmcs_gpa);
@@ -126,7 +128,7 @@ static void vmx_l1_guest_code(struct vmx_pages *vmx_pages)
 	GUEST_ASSERT(vmread(GUEST_RIP) == 0xc0ffffee);
 	GUEST_ASSERT(__vmlaunch());
 	GUEST_ASSERT(__vmresume());
-	GUEST_SYNC(13);
+	GUEST_SYNC(14);
 	GUEST_ASSERT(vmread(GUEST_RIP) == 0xc0ffffee);
 	GUEST_ASSERT(__vmlaunch());
 	GUEST_ASSERT(__vmresume());
-- 
2.56.0.218.g1ec904b173-goog