linux-kernel - Re: [PATCH v3 04/37] x86/cpufeatures: Enable CET CR4 bit for shadow stack

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: <14b4c6e3d5b7b259e832ff44e64597f1cf344ffe.camel@intel.com>
Date:   Mon, 7 Nov 2022 18:19:48 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "bp@...en8.de" <bp@...en8.de>
CC:     "bsingharora@...il.com" <bsingharora@...il.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "Syromiatnikov, Eugene" <esyr@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "Yu, Yu-cheng" <yu-cheng.yu@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Eranian, Stephane" <eranian@...gle.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "dethoma@...rosoft.com" <dethoma@...rosoft.com>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "pavel@....cz" <pavel@....cz>, "oleg@...hat.com" <oleg@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "jamorris@...ux.microsoft.com" <jamorris@...ux.microsoft.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "john.allen@....com" <john.allen@....com>,
        "rppt@...nel.org" <rppt@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "gorcunov@...il.com" <gorcunov@...il.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH v3 04/37] x86/cpufeatures: Enable CET CR4 bit for shadow
 stack

On Mon, 2022-11-07 at 19:00 +0100, Borislav Petkov wrote:
> On Fri, Nov 04, 2022 at 03:35:31PM -0700, Rick Edgecombe wrote:
> >  static __always_inline void setup_cet(struct cpuinfo_x86 *c)
> >  {
> > -	u64 msr = CET_ENDBR_EN;
> > +	bool kernel_ibt = HAS_KERNEL_IBT &&
> > cpu_feature_enabled(X86_FEATURE_IBT);
> > +	bool user_shstk;
> > +	u64 msr = 0;
> >  
> > -	if (!HAS_KERNEL_IBT ||
> > -	    !cpu_feature_enabled(X86_FEATURE_IBT))
> > +	/*
> > +	 * Enable user shadow stack only if the Linux defined user
> > shadow stack
> > +	 * cap was not cleared by command line.
> > +	 */
> > +	user_shstk = cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> > +		     IS_ENABLED(CONFIG_X86_USER_SHADOW_STACK) &&
> > +		     !test_bit(X86_FEATURE_USER_SHSTK, (unsigned long
> > *)cpu_caps_cleared);
> 
> Huh, why poke at cpu_caps_cleared? 

It was to catch if the software user shadow stack feature gets disabled
at boot with the "clearcpuid" command. Is there a better way to do
this?

> 
> Look below:
> 
> > +	if (!kernel_ibt && !user_shstk)
> >  		return;
> >  
> > +	if (user_shstk)
> > +		set_cpu_cap(c, X86_FEATURE_USER_SHSTK);
> > +
> > +	if (kernel_ibt)
> > +		msr = CET_ENDBR_EN;
> > +
> >  	wrmsrl(MSR_IA32_S_CET, msr);
> >  	cr4_set_bits(X86_CR4_CET);
> >  
> > -	if (!ibt_selftest()) {
> > +	if (kernel_ibt && !ibt_selftest()) {
> >  		pr_err("IBT selftest: Failed!\n");
> >  		setup_clear_cpu_cap(X86_FEATURE_IBT);
> >  		return;
> >  	}
> >  }
> > +#else /* CONFIG_X86_CET */
> > +static inline void setup_cet(struct cpuinfo_x86 *c) {}
> > +#endif
> >  
> >  __noendbr void cet_disable(void)
> >  {
> > -	if (cpu_feature_enabled(X86_FEATURE_IBT))
> > -		wrmsrl(MSR_IA32_S_CET, 0);
> > +	if (!(cpu_feature_enabled(X86_FEATURE_IBT) ||
> > +	      cpu_feature_enabled(X86_FEATURE_SHSTK)))
> > +		return;
> > +
> > +	wrmsrl(MSR_IA32_S_CET, 0);
> > +	wrmsrl(MSR_IA32_U_CET, 0);
> 
> Here you need to do
> 
> 	setup_clear_cpu_cap(X86_FEATURE_IBT);
> 	setup_clear_cpu_cap(X86_FEATURE_SHSTK);

This only gets called by kexec way after boot, as kexec is prepping to
transition to the new kernel. Do we want to be clearing feature bits at
that time?

> 
> and then the cpu_feature_enabled() test above alone should suffice.
> 
> But, before you do that, I'd like to ask you to update your patchset
> ontop of tip/master because the conflicts are getting non-trivial.
> This
> one doesn't even want to apply with a large fuzz:
> 
> $ patch -p1 --dry-run -F20 -i /tmp/new
> checking file arch/x86/kernel/cpu/common.c
> Hunk #1 FAILED at 596.
> 1 out of 1 hunk FAILED
> 
> Thx.

Sure, sorry about that. I'll target tip for the next version.

>