Re: [PATCH v2 05/16] KVM: VMX: Drop initialization of IA32_FEATURE_CONTROL MSR

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Oct 22, 2019 at 12:51:01PM +0200, Paolo Bonzini wrote:
> On 22/10/19 02:08, Sean Christopherson wrote:
> > Remove the code to initialize IA32_FEATURE_CONTROL MSR when KVM is
> > loaded now that the MSR is initialized during boot on all CPUs that
> > support VMX, i.e. can possibly load kvm_intel.
> > 
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 48 +++++++++++++++++-------------------------
> >  1 file changed, 19 insertions(+), 29 deletions(-)
> 
> I am still not sure about this...  Enabling VMX is adding a possible
> attack vector for the kernel, we should not do it unless we plan to do a
> VMXON.

An attacker would need arbitrary cpl0 access to toggle CR4.VMXE and do
VMXON (and VMLAUNCH), would an extra WRMSR really slow them down?

And practically speaking, how often do you encounter systems whose
firmware leaves IA32_FEATURE_CONTROL unlocked?

> Why is it so important to operate with locked
> IA32_FEATURE_CONTROL (so that KVM can enable VMX and the kernel can
> still enable SGX if desired).

For simplicity.  The alternative that comes to mind is to compute the
desired MSR value and write/lock the MSR on demand, e.g. add a sequence
similar to KVM's hardware_enable_all() for SGX, but that's a fair amount
of complexity for marginal benefit (IMO).

If a user really doesn't want VMX enabled, they can clear the feature bit
via the clearcpuid kernel param. 

That being said, enabling VMX in IA32_FEATURE_CONTROL if and only if
IS_ENABLED(CONFIG_KVM) is true would be an easy enhancement.