| 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
| ||
|
Message-ID: <fae8900f-be92-c697-47d0-497363767b9c@redhat.com>
Date: Fri, 22 Jul 2022 11:50:42 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: 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: Re: [PATCH v5 12/15] KVM: nVMX: Extend VMX MSRs quirk to CR0/4 fixed1
bits
On 6/7/22 23:36, Sean Christopherson wrote:
> 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>
I'm leaving this patch out for separate discussion since the quirk is
not needed anymore. I'll post the two reverts after some testing.
Paolo
> ---
> 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) &&
Powered by blists - more mailing lists