linux-kernel - Re: [PATCH v2 06/16] x86/cpu: Clear VMX feature flag if VMX is not fully enabled

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]

Message-ID: <20191114183238.GH24045@linux.intel.com>
Date:   Thu, 14 Nov 2019 10:32:38 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        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 06/16] x86/cpu: Clear VMX feature flag if VMX is not
 fully enabled

On Fri, Oct 25, 2019 at 06:38:58PM +0200, Borislav Petkov wrote:
> On Mon, Oct 21, 2019 at 05:08:36PM -0700, Sean Christopherson wrote:
> > Now that the IA32_FEATURE_CONTROL MSR is guaranteed to be configured and
> > locked, clear the VMX capability flag if the IA32_FEATURE_CONTROL MSR is
> > not supported or if BIOS disabled VMX, i.e. locked IA32_FEATURE_CONTROL
> > and did not set the appropriate VMX enable bit.
> > 
> > Cc: Paolo Bonzini <pbonzini@...hat.com>
> > Cc: Radim Krčmář <rkrcmar@...hat.com>
> > Cc: Vitaly Kuznetsov <vkuznets@...hat.com>
> > Cc: Wanpeng Li <wanpengli@...cent.com>
> > Cc: Jim Mattson <jmattson@...gle.com>
> > Cc: kvm@...r.kernel.org
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> > ---
> >  arch/x86/kernel/cpu/feature_control.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c
> > index 57b928e64cf5..74c76159a046 100644
> > --- a/arch/x86/kernel/cpu/feature_control.c
> > +++ b/arch/x86/kernel/cpu/feature_control.c
> > @@ -7,13 +7,19 @@
> >  
> >  void init_feature_control_msr(struct cpuinfo_x86 *c)
> >  {
> > +	bool tboot = tboot_enabled();
> >  	u64 msr;
> >  
> > -	if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr))
> > +	if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) {
> > +		if (cpu_has(c, X86_FEATURE_VMX)) {
> > +			pr_err_once("x86/cpu: VMX disabled, IA32_FEATURE_CONTROL MSR unsupported\n");
> 				     ^^^^^^^^
> 
> pr_fmt
> 
> But, before that: do we really wanna know about this or there's nothing
> the user can do? If she can reenable VMX in the BIOS, or otherwise do
> something about it, maybe we should say that above... Otherwise, this
> message is useless.

My thought for having the print was to alert the user that something is
royally borked with their system.  There's nothing the user can do to fix
it per se, but it does indicate that either their hardware or the VMM
hosting their virtual machine is broken.  So maybe be more explicit about
it being a likely hardware/VMM issue?

> > +			clear_cpu_cap(c, X86_FEATURE_VMX);
> > +		}
> >  		return;
> > +	}
> >  
> >  	if (msr & FEATURE_CONTROL_LOCKED)
> > -		return;
> > +		goto update_caps;
> >  
> >  	/*
> >  	 * Ignore whatever value BIOS left in the MSR to avoid enabling random
> > @@ -23,8 +29,19 @@ void init_feature_control_msr(struct cpuinfo_x86 *c)
> >  
> >  	if (cpu_has(c, X86_FEATURE_VMX)) {
> >  		msr |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> > -		if (tboot_enabled())
> > +		if (tboot)
> >  			msr |= FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX;
> >  	}
> >  	wrmsrl(MSR_IA32_FEATURE_CONTROL, msr);
> > +
> > +update_caps:
> > +	if (!cpu_has(c, X86_FEATURE_VMX))
> > +		return;
> 
> If this test is just so we can save us the below code, I'd say remove it
> for the sake of having less code in that function. The test is cheap and
> not on a fast path so who cares if we clear an alrady cleared bit. But
> maybe this evolves in the later patches...

I didn't want to print the "VMX disabled by BIOS..." message if VMX isn't
supported in the first place.  Later patches also add more code in this
flow, but avoiding the print message is the main motiviation.
 
> > +
> > +	if ((tboot && !(msr & FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX)) ||
> > +	    (!tboot && !(msr & FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX))) {
> > +		pr_err_once("x86/cpu: VMX disabled by BIOS (TXT %s)\n",
> > +			    tboot ? "enabled" : "disabled");
> > +		clear_cpu_cap(c, X86_FEATURE_VMX);
> > +	}
> >  }
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette