linux-kernel - Re: [PATCH v8 3/7] KVM: VMX: Pass through CET related MSRs

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191211015052.GF23765@linux.intel.com>
Date:   Tue, 10 Dec 2019 17:50:52 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Yang Weijiang <weijiang.yang@...el.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, jmattson@...gle.com,
        yu.c.zhang@...ux.intel.com, yu-cheng.yu@...el.com
Subject: Re: [PATCH v8 3/7] KVM: VMX: Pass through CET related MSRs

On Wed, Dec 11, 2019 at 09:32:07AM +0800, Yang Weijiang wrote:
> On Tue, Dec 10, 2019 at 01:18:21PM -0800, Sean Christopherson wrote:
> > On Fri, Nov 01, 2019 at 04:52:18PM +0800, Yang Weijiang wrote:
> > > CET MSRs pass through Guest directly to enhance performance.
> > > CET runtime control settings are stored in MSR_IA32_{U,S}_CET,
> > > Shadow Stack Pointer(SSP) are stored in MSR_IA32_PL{0,1,2,3}_SSP,
> > > SSP table base address is stored in MSR_IA32_INT_SSP_TAB,
> > > these MSRs are defined in kernel and re-used here.
> > > 
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index dd387a785c1e..4166c4fcad1e 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -371,13 +371,13 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
> > >  		F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
> > >  		F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
> > >  		F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
> > > -		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B);
> > > +		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | F(SHSTK);
> > >  
> > >  	/* cpuid 7.0.edx*/
> > >  	const u32 kvm_cpuid_7_0_edx_x86_features =
> > >  		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
> > >  		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP) |
> > > -		F(MD_CLEAR);
> > > +		F(MD_CLEAR) | F(IBT);
> > 
> > Advertising CET to userspace/guest needs to be done at the end of the
> > series, or at least after CR4.CET is no longer reserved, e.g. KVM_SET_SREGS
> > will fail and the guest will get a #GP when trying to set CR4.CET.
> > 
> > I'm pretty sure I've said this at least twice in previous versions of
> > this series...
> 
> Thanks Sean for picking these up!
> The reason is, starting from this patch, I'm using guest_cpuid_has(CET)
> to check the availability of guest CET CPUID, so logically I would like to let
> the readers understand CET related CPUID word is
> defined as above. But no problem, I can move these definitions to a
> latter patch as the patchset only meaningful as a whole. 

Adding usage of guest_cpuid_has(CET) without advertising CET is perfectly
ok from a functionality perspective.  Having a user without a consumer
isn't ideal, but it's better than having one gigantic patch.

The problem with advertising CET when it's not fully supported is that it
will break bisection, e.g. trying to boot a CET-enabled guest would get a
#GP during boot and likely crash.  Whether or not a series is useful when
taken as a whole is orthogonal to the integrity of each invidiual patch.