linux-kernel - Re: [PATCH v2 05/16] KVM: VMX: Drop initialization of IA32_FEATURE

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, 22 Oct 2019 08:16:22 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        linux-kernel@...r.kernel.org,
        Radim Krčmář <rkrcmar@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>, kvm@...r.kernel.org
Subject: Re: [PATCH v2 05/16] KVM: VMX: Drop initialization of
 IA32_FEATURE_CONTROL MSR

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@...gle.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.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.