diff mbox series

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

Message ID 20221104223604.29615-5-rick.p.edgecombe@intel.com (mailing list archive)
State New
Headers show
Series Shadow stacks for userspace | expand

Commit Message

Edgecombe, Rick P Nov. 4, 2022, 10:35 p.m. UTC 
From: Yu-cheng Yu <yu-cheng.yu@intel.com>

Setting CR4.CET is a prerequisite for utilizing any CET features, most of
which also require setting MSRs.

Kernel IBT already enables the CET CR4 bit when it detects IBT HW support
and is configured with kernel IBT. However, future patches that enable
userspace shadow stack support will need the bit set as well. So change
the logic to enable it in either case.

Clear MSR_IA32_U_CET in cet_disable() so that it can't live to see
userspace in a new kexec-ed kernel that has CR4.CET set from kernel IBT.

Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Kees Cook <keescook@chromium.org>

---

v3:
 - Remove stay new line (Boris)
 - Simplify commit log (Andrew Cooper)

v2:
 - In the shadow stack case, go back to only setting CR4.CET if the
   kernel is compiled with user shadow stack support.
 - Clear MSR_IA32_U_CET as well. (PeterZ)

KVM refresh:
 - Set CR4.CET if SHSTK or IBT are supported by HW, so that KVM can
   support CET even if IBT is disabled.
 - Drop no_user_shstk (Dave Hansen)
 - Elaborate on what the CR4 bit does in the commit log
 - Integrate with Kernel IBT logic

v1:
 - Moved kernel-parameters.txt changes here from patch 1.

 arch/x86/kernel/cpu/common.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

Comments

Borislav Petkov Nov. 7, 2022, 6 p.m. UTC | #1 
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? 

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);

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.
Edgecombe, Rick P Nov. 7, 2022, 6:19 p.m. UTC | #2 
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.

>
Borislav Petkov Nov. 7, 2022, 6:37 p.m. UTC | #3 
On Mon, Nov 07, 2022 at 06:19:48PM +0000, Edgecombe, Rick P wrote:
> It was to catch if the software user shadow stack feature gets disabled
> at boot with the "clearcpuid" command.

I don't understand. clearcpuid does setup_clear_cpu_cap() too. It would
eventually clear the bit in boot_cpu_data.x86_capability's AFAICT.

cpu_feature_enabled() looks at boot_cpu_data too.

So what's the problem?

Oh, and also, you've added that clearcpuid thing to the help docs.
Please remove it. clearcpuid= taints the kernel and we've left it in
because some of your colleagues really wanted it for testing or whatnot.
But it is crap and it was on its way out at some point so we better not
proliferate its use any more.

> Is there a better way to do this?

Yeah, cpu_feature_enabled() should be enough and if it isn't, then we
need to fix it to be.

Which reminds me, I'd need to take Maxim's patch too:

https://lore.kernel.org/r/20220718141123.136106-3-mlevitsk@redhat.com

as it is a simplification.

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

Hmm, I was under the impression you'll have the usual chicken bit
"noshstk" which gets added with every big feature. So it'll call that
thing here.

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

Thanks!
Edgecombe, Rick P Nov. 7, 2022, 7:19 p.m. UTC | #4 
On Mon, 2022-11-07 at 19:37 +0100, Borislav Petkov wrote:
> On Mon, Nov 07, 2022 at 06:19:48PM +0000, Edgecombe, Rick P wrote:
> > It was to catch if the software user shadow stack feature gets
> > disabled
> > at boot with the "clearcpuid" command.
> 
> I don't understand. clearcpuid does setup_clear_cpu_cap() too. It
> would
> eventually clear the bit in boot_cpu_data.x86_capability's AFAICT.
> 
> cpu_feature_enabled() looks at boot_cpu_data too.
> 
> So what's the problem?

You are right, there actually is no problem. I thought the
apply_forced_caps() was happening too late, but it is not. So this
check you highlighted would not be needed if we kept the clearcpuid
method.

Thanks.

> 
> Oh, and also, you've added that clearcpuid thing to the help docs.
> Please remove it. clearcpuid= taints the kernel and we've left it in
> because some of your colleagues really wanted it for testing or
> whatnot.
> But it is crap and it was on its way out at some point so we better
> not
> proliferate its use any more.
> 
> > Is there a better way to do this?
> 
> Yeah, cpu_feature_enabled() should be enough and if it isn't, then we
> need to fix it to be.
> 
> Which reminds me, I'd need to take Maxim's patch too:
> 
> https://lore.kernel.org/r/20220718141123.136106-3-mlevitsk@redhat.com
> 
> as it is a simplification.
> 
> > > 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?
> 
> Hmm, I was under the impression you'll have the usual chicken bit
> "noshstk" which gets added with every big feature. So it'll call that
> thing here.

There was at one point, but there was a suggestion to remove in favor
of clearcpuid. I can add it back.
Borislav Petkov Nov. 7, 2022, 7:30 p.m. UTC | #5 
On Mon, Nov 07, 2022 at 07:19:21PM +0000, Edgecombe, Rick P wrote:
> There was at one point, but there was a suggestion to remove in favor
> of clearcpuid. I can add it back.

Sounds like I need to school that "suggestor" about clearcpuid. :)

For example, when doing this:

  1625c833db93 ("x86/cpu: Allow feature bit names from /proc/cpuinfo in clearcpuid=")

I even managed to prevent the kernel from booting. So clearcpuid= is
definitely not for general consumption.

So as said, please remove it from your whole patchset's text.

Thx.
Edgecombe, Rick P Nov. 7, 2022, 7:33 p.m. UTC | #6 
On Mon, 2022-11-07 at 20:30 +0100, Borislav Petkov wrote:
> On Mon, Nov 07, 2022 at 07:19:21PM +0000, Edgecombe, Rick P wrote:
> > There was at one point, but there was a suggestion to remove in
> > favor
> > of clearcpuid. I can add it back.
> 
> Sounds like I need to school that "suggestor" about clearcpuid. :)
> 
> For example, when doing this:
> 
>   1625c833db93 ("x86/cpu: Allow feature bit names from /proc/cpuinfo
> in clearcpuid=")
> 
> I even managed to prevent the kernel from booting. So clearcpuid= is
> definitely not for general consumption.
> 
> So as said, please remove it from your whole patchset's text.

Will do, thanks.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 3e508f239098..0ba0a136adcb 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -596,28 +596,51 @@  __noendbr void ibt_restore(u64 save)
 
 #endif
 
+#ifdef CONFIG_X86_CET
 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);
+
+	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);
 }
 
 /*