KVM: nVMX: Don't leak L1 MMIO regions to L2

PATCH Superseded Jim Mattson 2 participants 4 messages Oct 15, 2019

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jim Mattson

author

2019-10-15 00:15 UTC #0 lore raw

PATCH v4 1/1 KVM: nVMX: Don't leak L1 MMIO regions to L2

If the "virtualize APIC accesses" VM-execution control is set in the VMCS, the APIC virtualization hardware is triggered when a page walk in VMX non-root mode terminates at a PTE wherein the address of the 4k page frame matches the APIC-access address specified in the VMCS. On hardware, the APIC-access address may be any valid 4k-aligned physical address.

KVM's nVMX implementation enforces the additional constraint that the APIC-access address specified in the vmcs12 must be backed by a "struct page" in L1. If not, L0 will simply clear the "virtualize APIC accesses" VM-execution control in the vmcs02.

The problem with this approach is that the L1 guest has arranged the vmcs12 EPT tables--or shadow page tables, if the "enable EPT" VM-execution control is clear in the vmcs12--so that the L2 guest physical address(es)--or L2 guest linear address(es)--that reference the L2 APIC map to the APIC-access address specified in the vmcs12. Without the "virtualize APIC accesses" VM-execution control in the vmcs02, the APIC accesses in the L2 guest will directly access the APIC-access page in L1.

When there is no mapping whatsoever for the APIC-access address in L1, the L2 VM just loses the intended APIC virtualization. However, when the APIC-access address is mapped to an MMIO region in L1, the L2 guest gets direct access to the L1 MMIO device. For example, if the APIC-access address specified in the vmcs12 is 0xfee00000, then L2 gets direct access to L1's APIC.

Since this vmcs12 configuration is something that KVM cannot faithfully emulate, the appropriate response is to exit to userspace with KVM_INTERNAL_ERROR_EMULATION.

Fixes fe3ef05c7572 ("KVM: nVMX: Prepare vmcs02 from vmcs01 and vmcs12")

Reported-by Dan Cross <[email protected]>

Signed-off-by Jim Mattson <[email protected]>

Reviewed-by Peter Shier <[email protected]>

---
v3 -> v4: Changed enum enter_vmx_status to enum nvmx_vmentry_status;
clarified debug message in nested_get_vmcs12_pages(); moved nested_vmx_enter_non_root_mode() error handling in nested_vmx_run() out-of-line
v2 -> v3: Added default case to new switch in nested_vmx_run v1 -> v2: Added enum enter_vmx_status

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/vmx/nested.c       | 65 +++++++++++++++++++--------------
 arch/x86/kvm/vmx/nested.h       | 13 ++++++-
 arch/x86/kvm/x86.c              |  8 +++-
 4 files changed, 56 insertions(+), 32 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -1

index 50eb430b0ad8..24d6598dea29 100644
@@ -1189,7 +1189,7 @@ struct kvm_x86_ops {
 	int (*set_nested_state)(struct kvm_vcpu *vcpu,
 				struct kvm_nested_state __user *user_kvm_nested_state,
 				struct kvm_nested_state *kvm_state);
-	void (*get_vmcs12_pages)(struct kvm_vcpu *vcpu);
+	bool (*get_vmcs12_pages)(struct kvm_vcpu *vcpu);
 
 	int (*smi_allowed)(struct kvm_vcpu *vcpu);
 	int (*pre_enter_smm)(struct kvm_vcpu *vcpu, char *smstate);

arch/x86/kvm/vmx/nested.c +37 ~~-28~~

142 lines not on this page · raw message

arch/x86/kvm/vmx/nested.h +12 -1

index 187d39bf0bf1..6280f33e5fa6 100644
@@ -6,6 +6,16 @@
 #include "vmcs12.h"
 #include "vmx.h"
 
+/*
+ * Status returned by nested_vmx_enter_non_root_mode():
+ */
+enum nvmx_vmentry_status {
+	NVMX_VMENTRY_SUCCESS,		/* Entered VMX non-root mode */
+	NVMX_VMENTRY_VMFAIL,		/* Consistency check VMFail */
+	NVMX_VMENTRY_VMEXIT,		/* Consistency check VMExit */
+	NVMX_VMENTRY_KVM_INTERNAL_ERROR,/* KVM internal error */
+};
+
 void vmx_leave_nested(struct kvm_vcpu *vcpu);
 void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
 				bool apicv);

@@ -13,7 +23,8 @@ void nested_vmx_hardware_unsetup(void);
 __init int nested_vmx_hardware_setup(int (*exit_handlers[])(struct kvm_vcpu *));
 void nested_vmx_vcpu_setup(void);
 void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu);
-int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry);
+enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
+						     bool from_vmentry);
 bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason);
 void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 		       u32 exit_intr_info, unsigned long exit_qualification);

arch/x86/kvm/x86.c +6 -2

index 661e2bf38526..2cf26f159071 100644
@@ -7941,8 +7941,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = false;
 
 	if (kvm_request_pending(vcpu)) {
-		if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu))
-			kvm_x86_ops->get_vmcs12_pages(vcpu);
+		if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu)) {
+			if (unlikely(!kvm_x86_ops->get_vmcs12_pages(vcpu))) {
+				r = 0;
+				goto out;
+			}
+		}
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

Signature · 1 line

2.23.0.700.g56cf767bdb-goog

Sean Christopherson

Reviewed-by on v4 1/1

2019-10-15 01:07 UTC #1 lore raw

Quoted 32 lines · Jim Mattson

If the "virtualize APIC accesses" VM-execution control is set in the VMCS, the APIC virtualization hardware is triggered when a page walk in VMX non-root mode terminates at a PTE wherein the address of the 4k page frame matches the APIC-access address specified in the VMCS. On hardware, the APIC-access address may be any valid 4k-aligned physical address.
KVM's nVMX implementation enforces the additional constraint that the APIC-access address specified in the vmcs12 must be backed by a "struct page" in L1. If not, L0 will simply clear the "virtualize APIC accesses" VM-execution control in the vmcs02.
The problem with this approach is that the L1 guest has arranged the vmcs12 EPT tables--or shadow page tables, if the "enable EPT" VM-execution control is clear in the vmcs12--so that the L2 guest physical address(es)--or L2 guest linear address(es)--that reference the L2 APIC map to the APIC-access address specified in the vmcs12. Without the "virtualize APIC accesses" VM-execution control in the vmcs02, the APIC accesses in the L2 guest will directly access the APIC-access page in L1.
When there is no mapping whatsoever for the APIC-access address in L1, the L2 VM just loses the intended APIC virtualization. However, when the APIC-access address is mapped to an MMIO region in L1, the L2 guest gets direct access to the L1 MMIO device. For example, if the APIC-access address specified in the vmcs12 is 0xfee00000, then L2 gets direct access to L1's APIC.
Since this vmcs12 configuration is something that KVM cannot faithfully emulate, the appropriate response is to exit to userspace with KVM_INTERNAL_ERROR_EMULATION.

Quoted 5 lines

Fixes fe3ef05c7572 ("KVM: nVMX: Prepare vmcs02 from vmcs01 and vmcs12")
Reported-by Dan Cross <[email protected]>
Signed-off-by Jim Mattson <[email protected]>
Reviewed-by Peter Shier <[email protected]>
---

With two nits below:

Reviewed-by Sean Christopherson <[email protected]>

arch/x86/kvm/vmx/nested.c static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) · line 3251

Quoted 13 lines

arch/x86/kvm/vmx/nested.c +2 -7

@@ -3244,13 +3247,9 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 	 * the nested entry.
 	 */
 	vmx->nested.nested_run_pending = 1;
-	ret = nested_vmx_enter_non_root_mode(vcpu, true);
-	vmx->nested.nested_run_pending = !ret;
-	if (ret > 0)
-		return 1;
-	else if (ret)
-		return nested_vmx_failValid(vcpu,
-			VMXERR_ENTRY_INVALID_CONTROL_FIELD);
+	status = nested_vmx_enter_non_root_mode(vcpu, true);
+	if (unlikely(status != NVMX_VMENTRY_SUCCESS))

KVM doesn't usually add (un)likely annotations for things that are under L1's control. The "unlikely(vmx->fail)" in nested_vmx_exit_reflected() is there because it's true iff KVM missed a VM-Fail condition that was caught by hardware.

Quoted 8 lines

arch/x86/kvm/vmx/nested.c +1 -0

+		goto vmentry_failed;
 
 	/* Hide L1D cache contents from the nested guest.  */
 	vmx->vcpu.arch.l1tf_flush_l1d = true;

@@ -3281,6 +3280,16 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 		return kvm_vcpu_halt(vcpu);
 	}
 	return 1;

arch/x86/kvm/vmx/nested.c static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) · line 3292

Quoted 10 lines

arch/x86/kvm/vmx/nested.c +10 -0

+
+vmentry_failed:
+	vmx->nested.nested_run_pending = 0;
+	if (status == NVMX_VMENTRY_KVM_INTERNAL_ERROR)
+		return 0;
+	if (status == NVMX_VMENTRY_VMEXIT)
+		return 1;
+	WARN_ON_ONCE(status != NVMX_VMENTRY_VMFAIL);
+	return nested_vmx_failValid(vcpu,
+				    VMXERR_ENTRY_INVALID_CONTROL_FIELD);

This can fit on a single line.

Quoted 3 lines

(quoted diff) +0 -0
 }
 
 /*

Jim Mattson

author

2019-10-15 17:13 UTC #2 lore raw

Quoted 54 lines · Sean Christopherson

Quoted 32 lines · Jim Mattson
If the "virtualize APIC accesses" VM-execution control is set in the VMCS, the APIC virtualization hardware is triggered when a page walk in VMX non-root mode terminates at a PTE wherein the address of the 4k page frame matches the APIC-access address specified in the VMCS. On hardware, the APIC-access address may be any valid 4k-aligned physical address.
KVM's nVMX implementation enforces the additional constraint that the APIC-access address specified in the vmcs12 must be backed by a "struct page" in L1. If not, L0 will simply clear the "virtualize APIC accesses" VM-execution control in the vmcs02.
The problem with this approach is that the L1 guest has arranged the vmcs12 EPT tables--or shadow page tables, if the "enable EPT" VM-execution control is clear in the vmcs12--so that the L2 guest physical address(es)--or L2 guest linear address(es)--that reference the L2 APIC map to the APIC-access address specified in the vmcs12. Without the "virtualize APIC accesses" VM-execution control in the vmcs02, the APIC accesses in the L2 guest will directly access the APIC-access page in L1.
When there is no mapping whatsoever for the APIC-access address in L1, the L2 VM just loses the intended APIC virtualization. However, when the APIC-access address is mapped to an MMIO region in L1, the L2 guest gets direct access to the L1 MMIO device. For example, if the APIC-access address specified in the vmcs12 is 0xfee00000, then L2 gets direct access to L1's APIC.
Since this vmcs12 configuration is something that KVM cannot faithfully emulate, the appropriate response is to exit to userspace with KVM_INTERNAL_ERROR_EMULATION.
Quoted 5 lines
Fixes fe3ef05c7572 ("KVM: nVMX: Prepare vmcs02 from vmcs01 and vmcs12")
Reported-by Dan Cross <[email protected]>
Signed-off-by Jim Mattson <[email protected]>
Reviewed-by Peter Shier <[email protected]>
---
With two nits below:
Reviewed-by Sean Christopherson <[email protected]>
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +0 -6
@@ -3244,13 +3247,9 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
       * the nested entry.
       */
      vmx->nested.nested_run_pending = 1;
-     ret = nested_vmx_enter_non_root_mode(vcpu, true);
-     vmx->nested.nested_run_pending = !ret;
-     if (ret > 0)
-             return 1;
-     else if (ret)
-             return nested_vmx_failValid(vcpu,

Quoted 8 lines

arch/x86/kvm/vmx/nested.c static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) · line 3251
Quoted 3 lines
arch/x86/kvm/vmx/nested.c +2 -1
-                     VMXERR_ENTRY_INVALID_CONTROL_FIELD);
+     status = nested_vmx_enter_non_root_mode(vcpu, true);
+     if (unlikely(status != NVMX_VMENTRY_SUCCESS))
KVM doesn't usually add (un)likely annotations for things that are under L1's control. The "unlikely(vmx->fail)" in nested_vmx_exit_reflected() is there because it's true iff KVM missed a VM-Fail condition that was caught by hardware.

I would argue that it makes sense to optimize for the success path in this case. If L1 is taking the failure path more frequently than the success path, something is wrong. Moreover, you have already indicated that the success path should be statically predicted taken by asking me to move the failure path out-of-line. (Forward conditional branches are statically predicted not taken, per section 3.4.1.3 of the Intel 64 and IA-32 Architectures Optimization Reference Manual.) I'm just asking the compiler not to undo that hint.

Quoted 24 lines

Quoted 8 lines

arch/x86/kvm/vmx/nested.c +1 -0

+             goto vmentry_failed;
 
      /* Hide L1D cache contents from the nested guest.  */
      vmx->vcpu.arch.l1tf_flush_l1d = true;

@@ -3281,6 +3280,16 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
              return kvm_vcpu_halt(vcpu);
      }
      return 1;

arch/x86/kvm/vmx/nested.c static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) · line 3292

Quoted 10 lines

arch/x86/kvm/vmx/nested.c +10 -0

+
+vmentry_failed:
+     vmx->nested.nested_run_pending = 0;
+     if (status == NVMX_VMENTRY_KVM_INTERNAL_ERROR)
+             return 0;
+     if (status == NVMX_VMENTRY_VMEXIT)
+             return 1;
+     WARN_ON_ONCE(status != NVMX_VMENTRY_VMFAIL);
+     return nested_vmx_failValid(vcpu,
+                                 VMXERR_ENTRY_INVALID_CONTROL_FIELD);

This can fit on a single line.

Quoted 3 lines

(quoted diff) +0 -0
 }
 
 /*

Sean Christopherson

2019-10-15 17:32 UTC #3 lore raw

Quoted 7 lines · Jim Mattson

Quoted 4 lines · Sean Christopherson
KVM doesn't usually add (un)likely annotations for things that are under L1's control. The "unlikely(vmx->fail)" in nested_vmx_exit_reflected() is there because it's true iff KVM missed a VM-Fail condition that was caught by hardware.

Quoted 8 lines

I would argue that it makes sense to optimize for the success path in this case. If L1 is taking the failure path more frequently than the success path, something is wrong. Moreover, you have already indicated that the success path should be statically predicted taken by asking me to move the failure path out-of-line. (Forward conditional branches are statically predicted not taken, per section 3.4.1.3 of the Intel 64 and IA-32 Architectures Optimization Reference Manual.) I'm just asking the compiler not to undo that hint.

Fair enough.