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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> 




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux