public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: vmx/nested: avoid blindly setting SECONDARY_EXEC_ENCLS_EXITING when sgx is enabled
@ 2022-10-25 12:37 Emanuele Giuseppe Esposito
  2022-10-25 17:21 ` Sean Christopherson
  0 siblings, 1 reply; 6+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-10-25 12:37 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Bandan Das,
	linux-kernel, Emanuele Giuseppe Esposito, stable

Currently vmx enables SECONDARY_EXEC_ENCLS_EXITING even when sgx
is not set in the host MSR.

When booting a guest, KVM checks that the cpuid bit is actually set
in vmx.c, and if not, it does not enable the feature.

However, in nesting this control bit is blindly set, and will be
propagated to VMCS12 and VMCS02. Therefore, when L1 tries to boot
the guest, the host will try to execute VMLOAD with VMCS02 containing
a feature that the hardware does not support, making it fail with
hardware error 0x7.

According to section "Secondary Processor-Based VM-Execution Controls"
in the Intel SDM, software should *always* check the value in the
actual MSR_IA32_VMX_PROCBASED_CTLS2 before enabling this bit.

Not updating enable_sgx is responsible for a second bug:
vmx_set_cpu_caps() doesn't clear the SGX bits when hardware support is
unavailable.  This is a much less problematic bug as it only pops up
if SGX is soft-disabled (the case being handled by cpu_has_sgx()) or if
SGX is supported for bare metal but not in the VMCS (will never happen
when running on bare metal, but can theoertically happen when running in
a VM).

Last but not least, KVM should ideally have module params reflect KVM's
actual configuration.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2127128

Fixes: 72add915fbd5 ("KVM: VMX: Enable SGX virtualization for SGX1, SGX2 and LC")
Cc: stable@vger.kernel.org

Suggested-by: Sean Christopherson <seanjc@google.com>
Suggested-by: Bandan Das <bsd@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9dba04b6b019..ea0c65d3c08a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8263,6 +8263,11 @@ static __init int hardware_setup(void)
 	if (!cpu_has_virtual_nmis())
 		enable_vnmi = 0;
 
+	#ifdef CONFIG_X86_SGX_KVM
+		if (!cpu_has_vmx_encls_vmexit())
+			enable_sgx = false;
+	#endif
+
 	/*
 	 * set_apic_access_page_addr() is used to reload apic access
 	 * page upon invalidation.  No need to do anything if not
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] KVM: vmx/nested: avoid blindly setting SECONDARY_EXEC_ENCLS_EXITING when sgx is enabled
@ 2022-10-24 12:48 Emanuele Giuseppe Esposito
  2022-10-24 16:52 ` Sean Christopherson
  0 siblings, 1 reply; 6+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-10-24 12:48 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Bandan Das,
	linux-kernel, Emanuele Giuseppe Esposito

Currently vmx enables SECONDARY_EXEC_ENCLS_EXITING even when sgx
is not set in the host MSR.
This was probably introduced when sgx was not yet fully supported, and
we wanted to trap guests trying to use the feature.

When booting a guest, KVM checks that the cpuid bit is actually set
in vmx.c, and if not, it does not enable the feature.

However, in nesting this control bit is blindly copied, and will be
propagated to VMCS12 and VMCS02. Therefore, when L1 tries to boot
the guest, the host will try to execute VMLOAD with VMCS02 containing
a feature that the hardware does not support, making it fail with
hardware error 0x7.

According with section A.3.3 of Intel System Programming Guide,
we should *always* check the value in the actual
MSR_IA32_VMX_PROCBASED_CTLS2 before enabling this bit.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2127128

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8f67a9c4a287..f651084010cc 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6678,6 +6678,25 @@ static u64 nested_vmx_calc_vmcs_enum_msr(void)
 	return (u64)max_idx << VMCS_FIELD_INDEX_SHIFT;
 }
 
+/*
+ * According with section A.3.3 of Intel System Programming Guide,
+ * we *can* set the guest MSR control X (in our case
+ * SECONDARY_EXEC_ENCLS_EXITING) *iff* bit 32+X of
+ * MSR_IA32_VMX_PROCBASED_CTLS2 is set to 1.
+ * Otherwise it must remain zero.
+ */
+static void nested_vmx_setup_encls_exiting(struct nested_vmx_msrs *msrs)
+{
+	u32 vmx_msr_procb_low, vmx_msr_procb_high;
+
+	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2, vmx_msr_procb_low, vmx_msr_procb_high);
+
+	WARN_ON(vmx_msr_procb_low & SECONDARY_EXEC_ENCLS_EXITING);
+
+	if (enable_sgx && (vmx_msr_procb_high & SECONDARY_EXEC_ENCLS_EXITING))
+		msrs->secondary_ctls_high |= SECONDARY_EXEC_ENCLS_EXITING;
+}
+
 /*
  * nested_vmx_setup_ctls_msrs() sets up variables containing the values to be
  * returned for the various VMX controls MSRs when nested VMX is enabled.
@@ -6874,8 +6893,7 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
 		msrs->secondary_ctls_high |=
 			SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
 
-	if (enable_sgx)
-		msrs->secondary_ctls_high |= SECONDARY_EXEC_ENCLS_EXITING;
+	nested_vmx_setup_encls_exiting(msrs);
 
 	/* miscellaneous data */
 	msrs->misc_low = (u32)vmcs_conf->misc & VMX_MISC_SAVE_EFER_LMA;
-- 
2.31.1


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

end of thread, other threads:[~2022-10-27 10:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-25 12:37 [PATCH] KVM: vmx/nested: avoid blindly setting SECONDARY_EXEC_ENCLS_EXITING when sgx is enabled Emanuele Giuseppe Esposito
2022-10-25 17:21 ` Sean Christopherson
2022-10-27 10:29   ` Paolo Bonzini
  -- strict thread matches above, loose matches on Subject: below --
2022-10-24 12:48 Emanuele Giuseppe Esposito
2022-10-24 16:52 ` Sean Christopherson
2022-10-25 12:36   ` Emanuele Giuseppe Esposito

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox