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