linux-kernel - Re: [PATCH v5 6/6] KVM: VMX: Move VERW closer to VMentry for MDS mitigation

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] [day] [month] [year] [list]

Date: Thu, 11 Jan 2024 16:02:06 -0800
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Andy Lutomirski <luto@...nel.org>, Jonathan Corbet <corbet@....net>,
	Paolo Bonzini <pbonzini@...hat.com>, tony.luck@...el.com,
	ak@...ux.intel.com, tim.c.chen@...ux.intel.com,
	Andrew Cooper <andrew.cooper3@...rix.com>,
	Nikolay Borisov <nik.borisov@...e.com>,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	kvm@...r.kernel.org,
	Alyssa Milburn <alyssa.milburn@...ux.intel.com>,
	Daniel Sneddon <daniel.sneddon@...ux.intel.com>,
	antonio.gomez.iglesias@...ux.intel.com
Subject: Re: [PATCH  v5 6/6] KVM: VMX: Move VERW closer to VMentry for MDS
 mitigation

On Thu, Jan 11, 2024 at 08:45:13AM -0800, Sean Christopherson wrote:
> On Thu, Jan 11, 2024, Pawan Gupta wrote:
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index bdcf2c041e0c..8defba8e417b 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -387,6 +387,17 @@ static __always_inline void vmx_enable_fb_clear(struct vcpu_vmx *vmx)
> >  
> >  static void vmx_update_fb_clear_dis(struct kvm_vcpu *vcpu, struct vcpu_vmx *vmx)
> >  {
> > +	/*
> > +	 * FB_CLEAR_CTRL is to optimize VERW latency in guests when host is
> > +	 * affected by MMIO Stale Data, but not by MDS/TAA. When
> > +	 * X86_FEATURE_CLEAR_CPU_BUF is enabled, system is likely affected by
> > +	 * MDS/TAA. Skip the optimization for such a case.
> 
> This is unnecessary speculation (ha!), and it'll also be confusing for many readers
> as the code below explicitly checks for MDS/TAA.  We have no idea why the host
> admin forced the mitigation to be enabled, and it doesn't matter.  The important
> thing to capture is that the intent is to keep the mitigation enabled when it
> was forcefully enabled, that should be self-explanatory and doesn't require
> speculating on _why_ the mitigation was forced on.

Agree.

> > +	 */
> > +	if (cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF)) {
> > +		vmx->disable_fb_clear = false;
> > +		return;
> > +	}
> > +
> >  	vmx->disable_fb_clear = (host_arch_capabilities & ARCH_CAP_FB_CLEAR_CTRL) &&
> >  				!boot_cpu_has_bug(X86_BUG_MDS) &&
> >  				!boot_cpu_has_bug(X86_BUG_TAA);
> 
> I would rather include the X86_FEATURE_CLEAR_CPU_BUF check along with all the
> other checks, and then add a common early return. E.g.
> 
> 	/*
> 	 * Disable VERW's behavior of clearing CPU buffers for the guest if the
> 	 * CPU isn't affected MDS/TAA, and the host hasn't forcefully enabled
> 	 * the mitigation.  Disabing the clearing provides a performance boost
> 	 * for guests that aren't aware that manually clearing CPU buffers is
> 	 * unnecessary, at the cost of MSR accesses on VM-Entry and VM-Exit.
> 	 */
> 	vmx->disable_fb_clear = !cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF) &&
> 				(host_arch_capabilities & ARCH_CAP_FB_CLEAR_CTRL) &&
> 				!boot_cpu_has_bug(X86_BUG_MDS) &&
> 				!boot_cpu_has_bug(X86_BUG_TAA);
> 
> 	if (!vmx->disable_fb_clear)
> 		return;

This is better. Thanks.