All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/7]  KVM: nVMX: Bug fixes and cleanup
@ 2020-09-23 18:44 Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 1/7] KVM: nVMX: Reset the segment cache when stuffing guest segs Sean Christopherson
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

Fix for a brutal segment caching bug that manifested as random nested
VM-Enter failures when running with unrestricted guest disabled.  A few
more bug fixes and cleanups for stuff found by inspection when hunting
down the caching issue.

v2:
  - Rebased to kvm/queue, commit e1ba1a15af73 ("KVM: SVM: Enable INVPCID
    feature on AMD").

Sean Christopherson (7):
  KVM: nVMX: Reset the segment cache when stuffing guest segs
  KVM: nVMX: Reload vmcs01 if getting vmcs12's pages fails
  KVM: nVMX: Explicitly check for valid guest state for !unrestricted
    guest
  KVM: nVMX: Move free_nested() below vmx_switch_vmcs()
  KVM: nVMX: Ensure vmcs01 is the loaded VMCS when freeing nested state
  KVM: nVMX: Drop redundant VMCS switch and free_nested() call
  KVM: nVMX: WARN on attempt to switch the currently loaded VMCS

 arch/x86/kvm/vmx/nested.c | 103 ++++++++++++++++++++------------------
 arch/x86/kvm/vmx/vmx.c    |   8 +--
 arch/x86/kvm/vmx/vmx.h    |   9 ++++
 3 files changed, 65 insertions(+), 55 deletions(-)

-- 
2.28.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/7] KVM: nVMX: Reset the segment cache when stuffing guest segs
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
@ 2020-09-23 18:44 ` Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 2/7] KVM: nVMX: Reload vmcs01 if getting vmcs12's pages fails Sean Christopherson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

Explicitly reset the segment cache after stuffing guest segment regs in
prepare_vmcs02_rare().  Although the cache is reset when switching to
vmcs02, there is nothing that prevents KVM from re-populating the cache
prior to writing vmcs02 with vmcs12's values.  E.g. if the vCPU is
preempted after switching to vmcs02 but before prepare_vmcs02_rare(),
kvm_arch_vcpu_put() will dereference GUEST_SS_AR_BYTES via .get_cpl()
and cache the stale vmcs02 value.  While the current code base only
caches stale data in the preemption case, it's theoretically possible
future code could read a segment register during the nested flow itself,
i.e. this isn't technically illegal behavior in kvm_arch_vcpu_put(),
although it did introduce the bug.

This manifests as an unexpected nested VM-Enter failure when running
with unrestricted guest disabled if the above preemption case coincides
with L1 switching L2's CPL, e.g. when switching from a L2 vCPU at CPL3
to to a L2 vCPU at CPL0.  stack_segment_valid() will see the new SS_SEL
but the old SS_AR_BYTES and incorrectly mark the guest state as invalid
due to SS.dpl != SS.rpl.

Don't bother updating the cache even though prepare_vmcs02_rare() writes
every segment.  With unrestricted guest, guest segments are almost never
read, let alone L2 guest segments.  On the other hand, populating the
cache requires a large number of memory writes, i.e. it's unlikely to be
a net win.  Updating the cache would be a win when unrestricted guest is
not supported, as guest_state_valid() will immediately cache all segment
registers.  But, nested virtualization without unrestricted guest is
dirt slow, saving some VMREADs won't change that, and every CPU
manufactured in the last decade supports unrestricted guest.  In other
words, the extra (minor) complexity isn't worth the trouble.

Note, kvm_arch_vcpu_put() may see stale data when querying guest CPL
depending on when preemption occurs.  This is "ok" in that the usage is
imperfect by nature, i.e. it's used heuristically to improve performance
but doesn't affect functionality.  kvm_arch_vcpu_put() could be "fixed"
by also disabling preemption while loading segments, but that's
pointless and misleading as reading state from kvm_sched_{in,out}() is
guaranteed to see stale data in one form or another.  E.g. even if all
the usage of regs_avail is fixed to call kvm_register_mark_available()
after the associated state is set, the individual state might still be
stale with respect to the overall vCPU state.  I.e. making functional
decisions in an asynchronous hook is doomed from the get go.  Thankfully
KVM doesn't do that.

Fixes: de63ad4cf4973 ("KVM: X86: implement the logic for spinlock optimization")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b6ce9ce91029..04441663a631 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2408,6 +2408,8 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
 		vmcs_writel(GUEST_TR_BASE, vmcs12->guest_tr_base);
 		vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
 		vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
+
+		vmx->segment_cache.bitmask = 0;
 	}
 
 	if (!hv_evmcs || !(hv_evmcs->hv_clean_fields &
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/7] KVM: nVMX: Reload vmcs01 if getting vmcs12's pages fails
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 1/7] KVM: nVMX: Reset the segment cache when stuffing guest segs Sean Christopherson
@ 2020-09-23 18:44 ` Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 3/7] KVM: nVMX: Explicitly check for valid guest state for !unrestricted guest Sean Christopherson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

Reload vmcs01 when bailing from nested_vmx_enter_non_root_mode() as KVM
expects vmcs01 to be loaded when is_guest_mode() is false.

Fixes: 671ddc700fd08 ("KVM: nVMX: Don't leak L1 MMIO regions to L2")
Cc: stable@vger.kernel.org
Cc: Dan Cross <dcross@google.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: Peter Shier <pshier@google.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 04441663a631..171e34286908 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3346,8 +3346,10 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
 	prepare_vmcs02_early(vmx, vmcs12);
 
 	if (from_vmentry) {
-		if (unlikely(!nested_get_vmcs12_pages(vcpu)))
+		if (unlikely(!nested_get_vmcs12_pages(vcpu))) {
+			vmx_switch_vmcs(vcpu, &vmx->vmcs01);
 			return NVMX_VMENTRY_KVM_INTERNAL_ERROR;
+		}
 
 		if (nested_vmx_check_vmentry_hw(vcpu)) {
 			vmx_switch_vmcs(vcpu, &vmx->vmcs01);
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/7] KVM: nVMX: Explicitly check for valid guest state for !unrestricted guest
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 1/7] KVM: nVMX: Reset the segment cache when stuffing guest segs Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 2/7] KVM: nVMX: Reload vmcs01 if getting vmcs12's pages fails Sean Christopherson
@ 2020-09-23 18:44 ` Sean Christopherson
  2020-09-25 21:33   ` Paolo Bonzini
  2020-09-23 18:44 ` [PATCH v2 4/7] KVM: nVMX: Move free_nested() below vmx_switch_vmcs() Sean Christopherson
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

Call guest_state_valid() directly instead of querying emulation_required
when checking if L1 is attempting VM-Enter with invalid guest state.
If emulate_invalid_guest_state is false, KVM will fixup segment regs to
avoid emulation and will never set emulation_required, i.e. KVM will
incorrectly miss the associated consistency checks because the nested
path stuffs segments directly into vmcs02.

Opportunsitically add Consistency Check tracing to make future debug
suck a little less.

Fixes: 2bb8cafea80bf ("KVM: vVMX: signal failure for nested VMEntry if emulation_required")
Fixes: 3184a995f782c ("KVM: nVMX: fix vmentry failure code when L2 state would require emulation")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 2 +-
 arch/x86/kvm/vmx/vmx.c    | 8 ++------
 arch/x86/kvm/vmx/vmx.h    | 9 +++++++++
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 171e34286908..a50714a86dde 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2573,7 +2573,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	 * which means L1 attempted VMEntry to L2 with invalid state.
 	 * Fail the VMEntry.
 	 */
-	if (vmx->emulation_required) {
+	if (CC(!vmx_guest_state_valid(vcpu))) {
 		*entry_failure_code = ENTRY_FAIL_DEFAULT;
 		return -EINVAL;
 	}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6f9a0c6d5dc5..e8480dbef881 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -341,7 +341,6 @@ static const struct kernel_param_ops vmentry_l1d_flush_ops = {
 };
 module_param_cb(vmentry_l1d_flush, &vmentry_l1d_flush_ops, NULL, 0644);
 
-static bool guest_state_valid(struct kvm_vcpu *vcpu);
 static u32 vmx_segment_access_rights(struct kvm_segment *var);
 static __always_inline void vmx_disable_intercept_for_msr(unsigned long *msr_bitmap,
 							  u32 msr, int type);
@@ -1415,7 +1414,7 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)
 
 static bool emulation_required(struct kvm_vcpu *vcpu)
 {
-	return emulate_invalid_guest_state && !guest_state_valid(vcpu);
+	return emulate_invalid_guest_state && !vmx_guest_state_valid(vcpu);
 }
 
 unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)
@@ -3471,11 +3470,8 @@ static bool cs_ss_rpl_check(struct kvm_vcpu *vcpu)
  * not.
  * We assume that registers are always usable
  */
-static bool guest_state_valid(struct kvm_vcpu *vcpu)
+bool __vmx_guest_state_valid(struct kvm_vcpu *vcpu)
 {
-	if (enable_unrestricted_guest)
-		return true;
-
 	/* real mode guest state checks */
 	if (!is_protmode(vcpu) || (vmx_get_rflags(vcpu) & X86_EFLAGS_VM)) {
 		if (!rmode_segment_valid(vcpu, VCPU_SREG_CS))
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index d7ec66db5eb8..e147f180350f 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -343,6 +343,15 @@ void vmx_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
 void vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
 u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa,
 		   int root_level);
+
+bool __vmx_guest_state_valid(struct kvm_vcpu *vcpu);
+static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
+{
+	if (enable_unrestricted_guest)
+		return true;
+
+	return __vmx_guest_state_valid(vcpu);
+}
 void update_exception_bitmap(struct kvm_vcpu *vcpu);
 void vmx_update_msr_bitmap(struct kvm_vcpu *vcpu);
 bool vmx_nmi_blocked(struct kvm_vcpu *vcpu);
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/7] KVM: nVMX: Move free_nested() below vmx_switch_vmcs()
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
                   ` (2 preceding siblings ...)
  2020-09-23 18:44 ` [PATCH v2 3/7] KVM: nVMX: Explicitly check for valid guest state for !unrestricted guest Sean Christopherson
@ 2020-09-23 18:44 ` Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 5/7] KVM: nVMX: Ensure vmcs01 is the loaded VMCS when freeing nested state Sean Christopherson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

Move free_nested() down below vmx_switch_vmcs() so that a future patch
can do an "emergency" invocation of vmx_switch_vmcs() if vmcs01 is not
the loaded VMCS when freeing nested resources.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 88 +++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index a50714a86dde..03dddf1b6009 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -233,50 +233,6 @@ static inline void nested_release_evmcs(struct kvm_vcpu *vcpu)
 	vmx->nested.hv_evmcs = NULL;
 }
 
-/*
- * Free whatever needs to be freed from vmx->nested when L1 goes down, or
- * just stops using VMX.
- */
-static void free_nested(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
-
-	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
-		return;
-
-	kvm_clear_request(KVM_REQ_GET_VMCS12_PAGES, vcpu);
-
-	vmx->nested.vmxon = false;
-	vmx->nested.smm.vmxon = false;
-	free_vpid(vmx->nested.vpid02);
-	vmx->nested.posted_intr_nv = -1;
-	vmx->nested.current_vmptr = -1ull;
-	if (enable_shadow_vmcs) {
-		vmx_disable_shadow_vmcs(vmx);
-		vmcs_clear(vmx->vmcs01.shadow_vmcs);
-		free_vmcs(vmx->vmcs01.shadow_vmcs);
-		vmx->vmcs01.shadow_vmcs = NULL;
-	}
-	kfree(vmx->nested.cached_vmcs12);
-	vmx->nested.cached_vmcs12 = NULL;
-	kfree(vmx->nested.cached_shadow_vmcs12);
-	vmx->nested.cached_shadow_vmcs12 = NULL;
-	/* Unpin physical memory we referred to in the vmcs02 */
-	if (vmx->nested.apic_access_page) {
-		kvm_release_page_clean(vmx->nested.apic_access_page);
-		vmx->nested.apic_access_page = NULL;
-	}
-	kvm_vcpu_unmap(vcpu, &vmx->nested.virtual_apic_map, true);
-	kvm_vcpu_unmap(vcpu, &vmx->nested.pi_desc_map, true);
-	vmx->nested.pi_desc = NULL;
-
-	kvm_mmu_free_roots(vcpu, &vcpu->arch.guest_mmu, KVM_MMU_ROOTS_ALL);
-
-	nested_release_evmcs(vcpu);
-
-	free_loaded_vmcs(&vmx->nested.vmcs02);
-}
-
 static void vmx_sync_vmcs_host_state(struct vcpu_vmx *vmx,
 				     struct loaded_vmcs *prev)
 {
@@ -315,6 +271,50 @@ static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs)
 	vmx_register_cache_reset(vcpu);
 }
 
+/*
+ * Free whatever needs to be freed from vmx->nested when L1 goes down, or
+ * just stops using VMX.
+ */
+static void free_nested(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
+		return;
+
+	kvm_clear_request(KVM_REQ_GET_VMCS12_PAGES, vcpu);
+
+	vmx->nested.vmxon = false;
+	vmx->nested.smm.vmxon = false;
+	free_vpid(vmx->nested.vpid02);
+	vmx->nested.posted_intr_nv = -1;
+	vmx->nested.current_vmptr = -1ull;
+	if (enable_shadow_vmcs) {
+		vmx_disable_shadow_vmcs(vmx);
+		vmcs_clear(vmx->vmcs01.shadow_vmcs);
+		free_vmcs(vmx->vmcs01.shadow_vmcs);
+		vmx->vmcs01.shadow_vmcs = NULL;
+	}
+	kfree(vmx->nested.cached_vmcs12);
+	vmx->nested.cached_vmcs12 = NULL;
+	kfree(vmx->nested.cached_shadow_vmcs12);
+	vmx->nested.cached_shadow_vmcs12 = NULL;
+	/* Unpin physical memory we referred to in the vmcs02 */
+	if (vmx->nested.apic_access_page) {
+		kvm_release_page_clean(vmx->nested.apic_access_page);
+		vmx->nested.apic_access_page = NULL;
+	}
+	kvm_vcpu_unmap(vcpu, &vmx->nested.virtual_apic_map, true);
+	kvm_vcpu_unmap(vcpu, &vmx->nested.pi_desc_map, true);
+	vmx->nested.pi_desc = NULL;
+
+	kvm_mmu_free_roots(vcpu, &vcpu->arch.guest_mmu, KVM_MMU_ROOTS_ALL);
+
+	nested_release_evmcs(vcpu);
+
+	free_loaded_vmcs(&vmx->nested.vmcs02);
+}
+
 /*
  * Ensure that the current vmcs of the logical processor is the
  * vmcs01 of the vcpu before calling free_nested().
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 5/7] KVM: nVMX: Ensure vmcs01 is the loaded VMCS when freeing nested state
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
                   ` (3 preceding siblings ...)
  2020-09-23 18:44 ` [PATCH v2 4/7] KVM: nVMX: Move free_nested() below vmx_switch_vmcs() Sean Christopherson
@ 2020-09-23 18:44 ` Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 6/7] KVM: nVMX: Drop redundant VMCS switch and free_nested() call Sean Christopherson
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

Add a WARN in free_nested() to ensure vmcs01 is loaded prior to freeing
vmcs02 and friends, and explicitly switch to vmcs01 if it's not.  KVM is
supposed to keep is_guest_mode() and loaded_vmcs==vmcs02 synchronized,
but bugs happen and freeing vmcs02 while it's in use will escalate a KVM
error to a use-after-free and potentially crash the kernel.

Do the WARN and switch even in the !vmxon case to help detect latent
bugs.  free_nested() is not a hot path, and the check is cheap.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 03dddf1b6009..3e6cc0d7090e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -279,6 +279,9 @@ static void free_nested(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
+	if (WARN_ON_ONCE(vmx->loaded_vmcs != &vmx->vmcs01))
+		vmx_switch_vmcs(vcpu, &vmx->vmcs01);
+
 	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
 		return;
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 6/7] KVM: nVMX: Drop redundant VMCS switch and free_nested() call
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
                   ` (4 preceding siblings ...)
  2020-09-23 18:44 ` [PATCH v2 5/7] KVM: nVMX: Ensure vmcs01 is the loaded VMCS when freeing nested state Sean Christopherson
@ 2020-09-23 18:44 ` Sean Christopherson
  2020-09-23 18:44 ` [PATCH v2 7/7] KVM: nVMX: WARN on attempt to switch the currently loaded VMCS Sean Christopherson
  2020-09-25 21:35 ` [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Paolo Bonzini
  7 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

Remove the explicit switch to vmcs01 and the call to free_nested() in
nested_vmx_free_vcpu().  free_nested(), which is called unconditionally
by vmx_leave_nested(), ensures vmcs01 is loaded prior to freeing vmcs02
and friends.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 3e6cc0d7090e..63550dcf6b9f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -326,8 +326,6 @@ void nested_vmx_free_vcpu(struct kvm_vcpu *vcpu)
 {
 	vcpu_load(vcpu);
 	vmx_leave_nested(vcpu);
-	vmx_switch_vmcs(vcpu, &to_vmx(vcpu)->vmcs01);
-	free_nested(vcpu);
 	vcpu_put(vcpu);
 }
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 7/7] KVM: nVMX: WARN on attempt to switch the currently loaded VMCS
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
                   ` (5 preceding siblings ...)
  2020-09-23 18:44 ` [PATCH v2 6/7] KVM: nVMX: Drop redundant VMCS switch and free_nested() call Sean Christopherson
@ 2020-09-23 18:44 ` Sean Christopherson
  2020-09-25 21:35 ` [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Paolo Bonzini
  7 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2020-09-23 18:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel, Dan Cross, Peter Shier

WARN if KVM attempts to switch to the currently loaded VMCS.  Now that
nested_vmx_free_vcpu() doesn't blindly call vmx_switch_vmcs(), all paths
that lead to vmx_switch_vmcs() are implicitly guarded by guest vs. host
mode, e.g. KVM should never emulate VMX instructions when guest mode is
active, and nested_vmx_vmexit() should never be called when host mode is
active.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.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 63550dcf6b9f..4bddda078370 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -258,7 +258,7 @@ static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs)
 	struct loaded_vmcs *prev;
 	int cpu;
 
-	if (vmx->loaded_vmcs == vmcs)
+	if (WARN_ON_ONCE(vmx->loaded_vmcs == vmcs))
 		return;
 
 	cpu = get_cpu();
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/7] KVM: nVMX: Explicitly check for valid guest state for !unrestricted guest
  2020-09-23 18:44 ` [PATCH v2 3/7] KVM: nVMX: Explicitly check for valid guest state for !unrestricted guest Sean Christopherson
@ 2020-09-25 21:33   ` Paolo Bonzini
  0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-09-25 21:33 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel, Dan Cross, Peter Shier

On 23/09/20 20:44, Sean Christopherson wrote:
> +bool __vmx_guest_state_valid(struct kvm_vcpu *vcpu)
>  {
> -	if (enable_unrestricted_guest)
> -		return true;
> -
>  	/* real mode guest state checks */
>  	if (!is_protmode(vcpu) || (vmx_get_rflags(vcpu) & X86_EFLAGS_VM)) {
>  		if (!rmode_segment_valid(vcpu, VCPU_SREG_CS))
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index d7ec66db5eb8..e147f180350f 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -343,6 +343,15 @@ void vmx_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
>  void vmx_set_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
>  u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa,
>  		   int root_level);
> +
> +bool __vmx_guest_state_valid(struct kvm_vcpu *vcpu);
> +static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
> +{
> +	if (enable_unrestricted_guest)
> +		return true;
> +
> +	return __vmx_guest_state_valid(vcpu);

This is now "if (is_unrestricted_guest (vcpu))", but the patch otherwise
applies.

Paolo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup
  2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
                   ` (6 preceding siblings ...)
  2020-09-23 18:44 ` [PATCH v2 7/7] KVM: nVMX: WARN on attempt to switch the currently loaded VMCS Sean Christopherson
@ 2020-09-25 21:35 ` Paolo Bonzini
  7 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-09-25 21:35 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel, Dan Cross, Peter Shier

On 23/09/20 20:44, Sean Christopherson wrote:
> Fix for a brutal segment caching bug that manifested as random nested
> VM-Enter failures when running with unrestricted guest disabled.  A few
> more bug fixes and cleanups for stuff found by inspection when hunting
> down the caching issue.
> 
> v2:
>   - Rebased to kvm/queue, commit e1ba1a15af73 ("KVM: SVM: Enable INVPCID
>     feature on AMD").
> 
> Sean Christopherson (7):
>   KVM: nVMX: Reset the segment cache when stuffing guest segs
>   KVM: nVMX: Reload vmcs01 if getting vmcs12's pages fails
>   KVM: nVMX: Explicitly check for valid guest state for !unrestricted
>     guest
>   KVM: nVMX: Move free_nested() below vmx_switch_vmcs()
>   KVM: nVMX: Ensure vmcs01 is the loaded VMCS when freeing nested state
>   KVM: nVMX: Drop redundant VMCS switch and free_nested() call
>   KVM: nVMX: WARN on attempt to switch the currently loaded VMCS
> 
>  arch/x86/kvm/vmx/nested.c | 103 ++++++++++++++++++++------------------
>  arch/x86/kvm/vmx/vmx.c    |   8 +--
>  arch/x86/kvm/vmx/vmx.h    |   9 ++++
>  3 files changed, 65 insertions(+), 55 deletions(-)
> 

Queued, thanks.

Paolo


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-25 21:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 18:44 [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Sean Christopherson
2020-09-23 18:44 ` [PATCH v2 1/7] KVM: nVMX: Reset the segment cache when stuffing guest segs Sean Christopherson
2020-09-23 18:44 ` [PATCH v2 2/7] KVM: nVMX: Reload vmcs01 if getting vmcs12's pages fails Sean Christopherson
2020-09-23 18:44 ` [PATCH v2 3/7] KVM: nVMX: Explicitly check for valid guest state for !unrestricted guest Sean Christopherson
2020-09-25 21:33   ` Paolo Bonzini
2020-09-23 18:44 ` [PATCH v2 4/7] KVM: nVMX: Move free_nested() below vmx_switch_vmcs() Sean Christopherson
2020-09-23 18:44 ` [PATCH v2 5/7] KVM: nVMX: Ensure vmcs01 is the loaded VMCS when freeing nested state Sean Christopherson
2020-09-23 18:44 ` [PATCH v2 6/7] KVM: nVMX: Drop redundant VMCS switch and free_nested() call Sean Christopherson
2020-09-23 18:44 ` [PATCH v2 7/7] KVM: nVMX: WARN on attempt to switch the currently loaded VMCS Sean Christopherson
2020-09-25 21:35 ` [PATCH v2 0/7] KVM: nVMX: Bug fixes and cleanup Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.