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: <20191025163858.GF6483@zn.tnic>
Date:   Fri, 25 Oct 2019 18:38:58 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
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 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.

> +			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...

> +
> +	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