lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue,  7 Jun 2022 21:36:01 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Eric Li <ercli@...avis.edu>,
        David Matlack <dmatlack@...gle.com>,
        Oliver Upton <oupton@...gle.com>
Subject: [PATCH v5 12/15] KVM: nVMX: Extend VMX MSRs quirk to CR0/4 fixed1 bits

Extend the VMX MSRs quirk to the CR0/4_FIXED1 MSRs, i.e. when the quirk
is disabled, allow userspace to set the MSRs and do not rewrite the MSRs
during CPUID updates.  The bits that the guest (L2 in this case) is
allowed to set are not directly tied to CPUID.  Enumerating to L1 that it
can set reserved CR0/4 bits is nonsensical and will ultimately result in
a failed nested VM-Entry (KVM enforces guest reserved CR4 bits on top of
the VMX MSRs), but KVM typically doesn't police the vCPU model except
when doing so is necessary to protect the host kernel.

Further restricting CR4 bits is however a reasonable thing to do, e.g. to
work around a bug in nested virtualization, in which case exposing a
feature to L1 is ok, but letting L2 use the feature is not.  Of course,
whether or not the L1 hypervisor will actually _check_ the FIXED1 bits is
another matter entirely, e.g. KVM currently assumes all bits that can be
set in the host can also be set in the guest.

Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 Documentation/virt/kvm/api.rst |  8 ++++++++
 arch/x86/kvm/vmx/nested.c      | 33 ++++++++++++++++++++++++++++++---
 arch/x86/kvm/vmx/vmx.c         |  6 +++---
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 1095692ddab7..88d1bbae031e 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -7391,6 +7391,14 @@ The valid bits in cap.args[0] are:
                                       IA32_VMX_TRUE_EXIT_CTLS[bit 44]
                                       ('load IA32_PERF_GLOBAL_CTRL'). Otherwise,
                                       these corresponding MSR bits are cleared.
+                                    - MSR_IA32_VMX_CR0_FIXED1 is unconditionally
+                                      set to 0xffffffff
+                                    - CR4.PCE is unconditionally set in
+                                      MSR_IA32_VMX_CR4_FIXED1.
+                                    - All CR4 bits with an associated CPUID
+                                      feature flag are set in
+                                      MSR_IA32_VMX_CR4_FIXED1 if the feature is
+                                      reported as supported in guest CPUID.
 
                                     When this quirk is disabled, KVM will not
                                     change the values of the aformentioned VMX
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 5533c2474128..abce74cfefc9 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1385,6 +1385,30 @@ static int vmx_restore_fixed0_msr(struct vcpu_vmx *vmx, u32 msr_index, u64 data)
 	return 0;
 }
 
+static u64 *vmx_get_fixed1_msr(struct nested_vmx_msrs *msrs, u32 msr_index)
+{
+	switch (msr_index) {
+	case MSR_IA32_VMX_CR0_FIXED1:
+		return &msrs->cr0_fixed1;
+	case MSR_IA32_VMX_CR4_FIXED1:
+		return &msrs->cr4_fixed1;
+	default:
+		BUG();
+	}
+}
+
+static int vmx_restore_fixed1_msr(struct vcpu_vmx *vmx, u32 msr_index, u64 data)
+{
+	const u64 *msr = vmx_get_fixed1_msr(&vmcs_config.nested, msr_index);
+
+	/* Bits that "must-be-0" must not be set in the restored value. */
+	if (!is_bitwise_subset(*msr, data, -1ULL))
+		return -EINVAL;
+
+	*vmx_get_fixed1_msr(&vmx->nested.msrs, msr_index) = data;
+	return 0;
+}
+
 /*
  * Called when userspace is restoring VMX MSRs.
  *
@@ -1432,10 +1456,13 @@ int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
 	case MSR_IA32_VMX_CR0_FIXED1:
 	case MSR_IA32_VMX_CR4_FIXED1:
 		/*
-		 * These MSRs are generated based on the vCPU's CPUID, so we
-		 * do not support restoring them directly.
+		 * These MSRs are generated based on the vCPU's CPUID when KVM
+		 * "owns" the VMX MSRs, do not allow restoring them directly.
 		 */
-		return -EINVAL;
+		if (kvm_check_has_quirk(vmx->vcpu.kvm, KVM_X86_QUIRK_TWEAK_VMX_MSRS))
+			return -EINVAL;
+
+		return vmx_restore_fixed1_msr(vmx, msr_index, data);
 	case MSR_IA32_VMX_EPT_VPID_CAP:
 		return vmx_restore_vmx_ept_vpid_cap(vmx, data);
 	case MSR_IA32_VMX_VMCS_ENUM:
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4c31c8f24329..139f365ca6bb 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7520,10 +7520,10 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 			~(FEAT_CTL_VMX_ENABLED_INSIDE_SMX |
 			  FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX);
 
-	if (nested_vmx_allowed(vcpu)) {
+	if (nested_vmx_allowed(vcpu) &&
+	    kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_TWEAK_VMX_MSRS)) {
 		nested_vmx_cr_fixed1_bits_update(vcpu);
-		if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_TWEAK_VMX_MSRS))
-			nested_vmx_entry_exit_ctls_update(vcpu);
+		nested_vmx_entry_exit_ctls_update(vcpu);
 	}
 
 	if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
-- 
2.36.1.255.ge46751e96f-goog

Powered by blists - more mailing lists