[PATCH] Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"

[PATCH] Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"

[Paolo Bonzini]

Since commit 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls
when guest MPX disabled"), KVM has taken ownership of the "load
IA32_BNDCFGS" and "clear IA32_BNDCFGS" VMX entry/exit controls,
trying to set these bits in the IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS
MSRs if the guest's CPUID supports MPX, and clear otherwise.

The intent of the patch was to apply it to L0 in order to work around
L1 kernels that lack the fix in commit 691bd4340bef ("kvm: vmx: allow
host to access guest MSR_IA32_BNDCFGS", 2017-07-04): by hiding the
control bits from L0, L1 hides BNDCFGS from KVM_GET_MSR_INDEX_LIST,
and the L1 bug is neutralized even in the lack of commit 691bd4340bef.

This was perhaps a sensible kludge at the time, but a horrible
idea in the long term and in fact it has not been extended to
other CPUID bits like these:

  X86_FEATURE_LM => VM_EXIT_HOST_ADDR_SPACE_SIZE, VM_ENTRY_IA32E_MODE,
                    VMX_MISC_SAVE_EFER_LMA

  X86_FEATURE_TSC => CPU_BASED_RDTSC_EXITING, CPU_BASED_USE_TSC_OFFSETTING,
                     SECONDARY_EXEC_TSC_SCALING

  X86_FEATURE_INVPCID_SINGLE => SECONDARY_EXEC_ENABLE_INVPCID

  X86_FEATURE_MWAIT => CPU_BASED_MONITOR_EXITING, CPU_BASED_MWAIT_EXITING

  X86_FEATURE_INTEL_PT => SECONDARY_EXEC_PT_CONCEAL_VMX, SECONDARY_EXEC_PT_USE_GPA,
                          VM_EXIT_CLEAR_IA32_RTIT_CTL, VM_ENTRY_LOAD_IA32_RTIT_CTL

  X86_FEATURE_XSAVES => SECONDARY_EXEC_XSAVES

These days it's sort of common knowledge that any MSR in
KVM_GET_MSR_INDEX_LIST must allow *at least* setting it with KVM_SET_MSR
to a default value, so it is unlikely that something like commit
5f76f6f5ff96 will be needed again.  So revert it, at the potential cost
of breaking L1s with a 6 year old kernel.  While in principle the L0 owner
doesn't control what runs on L1, such an old hypervisor would probably
have many other bugs.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 75ee509f0b7b..4fd25e1d6ec9 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7457,23 +7457,6 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
 #undef cr4_fixed1_update
 }
 
-static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
-
-	if (kvm_mpx_supported()) {
-		bool mpx_enabled = guest_cpuid_has(vcpu, X86_FEATURE_MPX);
-
-		if (mpx_enabled) {
-			vmx->nested.msrs.entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
-			vmx->nested.msrs.exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
-		} else {
-			vmx->nested.msrs.entry_ctls_high &= ~VM_ENTRY_LOAD_BNDCFGS;
-			vmx->nested.msrs.exit_ctls_high &= ~VM_EXIT_CLEAR_BNDCFGS;
-		}
-	}
-}
-
 static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -7565,10 +7548,8 @@ 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))
 		nested_vmx_cr_fixed1_bits_update(vcpu);
-		nested_vmx_entry_exit_ctls_update(vcpu);
-	}
 
 	if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
 			guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT))
-- 
2.31.1

Re: [PATCH] Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"

[Sean Christopherson]

On Fri, Jul 22, 2022, Paolo Bonzini wrote:
> Since commit 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls
> when guest MPX disabled"), KVM has taken ownership of the "load
> IA32_BNDCFGS" and "clear IA32_BNDCFGS" VMX entry/exit controls,
> trying to set these bits in the IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS
> MSRs if the guest's CPUID supports MPX, and clear otherwise.
> 
> The intent of the patch was to apply it to L0 in order to work around
> L1 kernels that lack the fix in commit 691bd4340bef ("kvm: vmx: allow
> host to access guest MSR_IA32_BNDCFGS", 2017-07-04): by hiding the
> control bits from L0, L1 hides BNDCFGS from KVM_GET_MSR_INDEX_LIST,
> and the L1 bug is neutralized even in the lack of commit 691bd4340bef.
> 
> This was perhaps a sensible kludge at the time, but a horrible
> idea in the long term and in fact it has not been extended to
> other CPUID bits like these:
> 
>   X86_FEATURE_LM => VM_EXIT_HOST_ADDR_SPACE_SIZE, VM_ENTRY_IA32E_MODE,
>                     VMX_MISC_SAVE_EFER_LMA
> 
>   X86_FEATURE_TSC => CPU_BASED_RDTSC_EXITING, CPU_BASED_USE_TSC_OFFSETTING,
>                      SECONDARY_EXEC_TSC_SCALING
> 
>   X86_FEATURE_INVPCID_SINGLE => SECONDARY_EXEC_ENABLE_INVPCID
> 
>   X86_FEATURE_MWAIT => CPU_BASED_MONITOR_EXITING, CPU_BASED_MWAIT_EXITING
> 
>   X86_FEATURE_INTEL_PT => SECONDARY_EXEC_PT_CONCEAL_VMX, SECONDARY_EXEC_PT_USE_GPA,
>                           VM_EXIT_CLEAR_IA32_RTIT_CTL, VM_ENTRY_LOAD_IA32_RTIT_CTL
> 
>   X86_FEATURE_XSAVES => SECONDARY_EXEC_XSAVES
> 
> These days it's sort of common knowledge that any MSR in
> KVM_GET_MSR_INDEX_LIST must allow *at least* setting it with KVM_SET_MSR
> to a default value, so it is unlikely that something like commit
> 5f76f6f5ff96 will be needed again.  So revert it, at the potential cost
> of breaking L1s with a 6 year old kernel.  

I would further qualify this with "breaking L1s with an _unpatched_ 6 year old
kernel".  That fix was tagged for stable and made it way to at least the 4.9 and
4.4 LTS releases.

> While in principle the L0 owner doesn't control what runs on L1, such an old
> hypervisor would probably have many other bugs.

And patching KVM to workaround L1 Linux bugs never ends well, e.g. see also the
hypercall patching snafu.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH] Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"

[Paolo Bonzini]

On 7/22/22 17:27, Sean Christopherson wrote:
>> So revert it, at the potential cost
>> of breaking L1s with a 6 year old kernel.
> I would further qualify this with "breaking L1s with an_unpatched_  6 year old
> kernel".  That fix was tagged for stable and made it way to at least the 4.9 and
> 4.4 LTS releases.
> 

Well, there _are_ people that use very old kernels and keep them 
up-to-date with fixes for only critical CVEs (for example by, ehm, 
paying my employer to do so).  But still it's way way unlikely for them 
to be used as L1 in a nested setup, whether on their own hardware or in 
the cloud.

I pushed everything to kvm/queue, but depending on what you post it may 
be deferred to 5.21.

Paolo

Re: [PATCH] Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"

[Sean Christopherson]

On Fri, Jul 22, 2022, Paolo Bonzini wrote:
> On 7/22/22 17:27, Sean Christopherson wrote:
> > > So revert it, at the potential cost
> > > of breaking L1s with a 6 year old kernel.
> > I would further qualify this with "breaking L1s with an_unpatched_  6 year old
> > kernel".  That fix was tagged for stable and made it way to at least the 4.9 and
> > 4.4 LTS releases.
> > 
> 
> Well, there _are_ people that use very old kernels and keep them up-to-date
> with fixes for only critical CVEs (for example by, ehm, paying my employer
> to do so).

Heh, I'm sure that's a winning strategy.

> But still it's way way unlikely for them to be used as L1 in a nested setup,
> whether on their own hardware or in the cloud.
>
> I pushed everything to kvm/queue, but depending on what you post it may be
> deferred to 5.21.

Can you drop the PERF_GLOBAL_CTRL revert?  I figured out how to achieve what you
intended, but in a more robust (and IMO more logical) manner.

If you don't drop it before I concoct the series, I'll just include a throwaway
patch to revert it.

Re: [PATCH] Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"

[Paolo Bonzini]

On 7/22/22 19:23, Sean Christopherson wrote:
> Can you drop the PERF_GLOBAL_CTRL revert?  I figured out how to achieve what you
> intended, but in a more robust (and IMO more logical) manner.
> 
> If you don't drop it before I concoct the series, I'll just include a throwaway
> patch to revert it.

I didn't drop it in the end (pushed before reading this), but it's 
intended to be temporary and I'll of course drop it if your series 
supersedes it instead of building upon it.

You don't need to include the revert but you can also do that if you prefer.

Paolo