diff mbox series

[03/19] x86/cpufeatures: Enable CET CR4 bit for shadow stack

Message ID 20220616084643.19564-4-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series Refresh queued CET virtualization series | expand

Commit Message

Yang, Weijiang June 16, 2022, 8:46 a.m. UTC 
From: Yu-cheng Yu <yu-cheng.yu@intel.com>

Utilizing CET features requires a CR4 bit to be enabled as well as bits
to be set in CET MSRs. Setting the CR4 bit does two things:
 1. Enables the usage of WRUSS instruction, which the kernel can use to
    write to userspace shadow stacks.
 2. Allows those individual aspects of CET to be enabled later via the MSR.

While future patches will allow the MSR values to be saved and restored
per task, the CR4 bit will allow for WRUSS to be used regardless of if a
tasks CET MSRs have been restored.

Kernel IBT already enables the CR4 bit. Modify the logic to enable it for
when the kernel is configured with and detects shadow stack support, as
well.

Rename cet_disable() to ibt_disable() since it no longer applies to all
CET features in the kernel.

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>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Cc: Kees Cook <keescook@chromium.org>

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

Yu-cheng v25:
 - Remove software-defined X86_FEATURE_CET.

Yu-cheng v24:
 - Update #ifdef placement to reflect Kconfig changes of splitting shadow stack
   and ibt.

 arch/x86/include/asm/cpu.h         |  2 +-
 arch/x86/kernel/cpu/common.c       | 14 +++++++-------
 arch/x86/kernel/machine_kexec_64.c |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

Comments

Peter Zijlstra June 16, 2022, 10:24 a.m. UTC | #1 
On Thu, Jun 16, 2022 at 04:46:27AM -0400, Yang Weijiang wrote:
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -74,7 +74,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
>  static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {}
>  #endif
>  
> -extern __noendbr void cet_disable(void);
> +extern __noendbr void ibt_disable(void);
>  
>  struct ucode_cpu_info;
>  
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index c296cb1c0113..86102a8d451e 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -598,23 +598,23 @@ __noendbr void ibt_restore(u64 save)

>  
> -__noendbr void cet_disable(void)
> +__noendbr void ibt_disable(void)
>  {
>  	if (cpu_feature_enabled(X86_FEATURE_IBT))
>  		wrmsrl(MSR_IA32_S_CET, 0);

Not sure about this rename; it really disables all of (S) CET.

Specifically, once we do S-SHSTK (after FRED) we might also very much
need to kill that for kexec.

> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 0611fd83858e..745024654fcd 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -311,7 +311,7 @@ void machine_kexec(struct kimage *image)
>  	/* Interrupts aren't acceptable while we reboot */
>  	local_irq_disable();
>  	hw_breakpoint_disable();
> -	cet_disable();
> +	ibt_disable();
>  
>  	if (image->preserve_context) {
>  #ifdef CONFIG_X86_IO_APIC
> -- 
> 2.27.0
>
Peter Zijlstra June 16, 2022, 10:25 a.m. UTC | #2 
On Thu, Jun 16, 2022 at 04:46:27AM -0400, Yang Weijiang wrote:

>  static __always_inline void setup_cet(struct cpuinfo_x86 *c)
>  {
> +	bool kernel_ibt = HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT);
>  	u64 msr = CET_ENDBR_EN;
>  
> +	if (kernel_ibt)
> +		wrmsrl(MSR_IA32_S_CET, msr);
>  
> +	if (kernel_ibt || cpu_feature_enabled(X86_FEATURE_SHSTK))
> +		cr4_set_bits(X86_CR4_CET);

Does flipping the CR4 and S_CET MSR write not result in simpler code?

>  
> +	if (kernel_ibt && !ibt_selftest()) {
>  		pr_err("IBT selftest: Failed!\n");
>  		setup_clear_cpu_cap(X86_FEATURE_IBT);

Looking at this error path; I think I forgot to clear S_CET here.

>  		return;
>  	}
>  }
Edgecombe, Rick P June 16, 2022, 5:12 p.m. UTC | #3 
On Thu, 2022-06-16 at 12:24 +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2022 at 04:46:27AM -0400, Yang Weijiang wrote:
> > --- a/arch/x86/include/asm/cpu.h
> > +++ b/arch/x86/include/asm/cpu.h
> > @@ -74,7 +74,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
> >   static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {}
> >   #endif
> >   
> > -extern __noendbr void cet_disable(void);
> > +extern __noendbr void ibt_disable(void);
> >   
> >   struct ucode_cpu_info;
> >   
> > diff --git a/arch/x86/kernel/cpu/common.c
> > b/arch/x86/kernel/cpu/common.c
> > index c296cb1c0113..86102a8d451e 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -598,23 +598,23 @@ __noendbr void ibt_restore(u64 save)
> >   
> > -__noendbr void cet_disable(void)
> > +__noendbr void ibt_disable(void)
> >   {
> >        if (cpu_feature_enabled(X86_FEATURE_IBT))
> >                wrmsrl(MSR_IA32_S_CET, 0);
> 
> Not sure about this rename; it really disables all of (S) CET.
> 
> Specifically, once we do S-SHSTK (after FRED) we might also very much
> need to kill that for kexec.

Sure, what about something like sup_cet_disable()?
Edgecombe, Rick P June 16, 2022, 5:36 p.m. UTC | #4 
On Thu, 2022-06-16 at 12:25 +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2022 at 04:46:27AM -0400, Yang Weijiang wrote:
> 
> >   static __always_inline void setup_cet(struct cpuinfo_x86 *c)
> >   {
> > +     bool kernel_ibt = HAS_KERNEL_IBT &&
> > cpu_feature_enabled(X86_FEATURE_IBT);
> >        u64 msr = CET_ENDBR_EN;
> >   
> > +     if (kernel_ibt)
> > +             wrmsrl(MSR_IA32_S_CET, msr);
> >   
> > +     if (kernel_ibt || cpu_feature_enabled(X86_FEATURE_SHSTK))
> > +             cr4_set_bits(X86_CR4_CET);
> 
> Does flipping the CR4 and S_CET MSR write not result in simpler code?

I thought it was more defensive to reset S_CET before turning it on
with CR4. Of course CR4.CET could have been left on as well, but if CET
features were actually fully turned on, then we probably wouldn't have
gotten here. Seem reasonable?

> 
> >   
> > +     if (kernel_ibt && !ibt_selftest()) {
> >                pr_err("IBT selftest: Failed!\n");
> >                setup_clear_cpu_cap(X86_FEATURE_IBT);
> 
> Looking at this error path; I think I forgot to clear S_CET here.
> 

Yea. I can fix it in the next version of this if you want.

> >                return;
> >        }
> >   }
Peter Zijlstra June 17, 2022, 11:38 a.m. UTC | #5 
On Thu, Jun 16, 2022 at 05:12:47PM +0000, Edgecombe, Rick P wrote:
> On Thu, 2022-06-16 at 12:24 +0200, Peter Zijlstra wrote:
> > On Thu, Jun 16, 2022 at 04:46:27AM -0400, Yang Weijiang wrote:
> > > --- a/arch/x86/include/asm/cpu.h
> > > +++ b/arch/x86/include/asm/cpu.h
> > > @@ -74,7 +74,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
> > >   static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {}
> > >   #endif
> > >   
> > > -extern __noendbr void cet_disable(void);
> > > +extern __noendbr void ibt_disable(void);
> > >   
> > >   struct ucode_cpu_info;
> > >   
> > > diff --git a/arch/x86/kernel/cpu/common.c
> > > b/arch/x86/kernel/cpu/common.c
> > > index c296cb1c0113..86102a8d451e 100644
> > > --- a/arch/x86/kernel/cpu/common.c
> > > +++ b/arch/x86/kernel/cpu/common.c
> > > @@ -598,23 +598,23 @@ __noendbr void ibt_restore(u64 save)
> > >   
> > > -__noendbr void cet_disable(void)
> > > +__noendbr void ibt_disable(void)
> > >   {
> > >        if (cpu_feature_enabled(X86_FEATURE_IBT))
> > >                wrmsrl(MSR_IA32_S_CET, 0);
> > 
> > Not sure about this rename; it really disables all of (S) CET.
> > 
> > Specifically, once we do S-SHSTK (after FRED) we might also very much
> > need to kill that for kexec.
> 
> Sure, what about something like sup_cet_disable()?

Why bother? Arguably kexec should clear U_CET too.
Edgecombe, Rick P June 17, 2022, 9:18 p.m. UTC | #6 
+kexec people

On Fri, 2022-06-17 at 13:38 +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2022 at 05:12:47PM +0000, Edgecombe, Rick P wrote:
> > On Thu, 2022-06-16 at 12:24 +0200, Peter Zijlstra wrote:
> > > On Thu, Jun 16, 2022 at 04:46:27AM -0400, Yang Weijiang wrote:
> > > > --- a/arch/x86/include/asm/cpu.h
> > > > +++ b/arch/x86/include/asm/cpu.h
> > > > @@ -74,7 +74,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86
> > > > *c);
> > > >    static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> > > > {}
> > > >    #endif
> > > >    
> > > > -extern __noendbr void cet_disable(void);
> > > > +extern __noendbr void ibt_disable(void);
> > > >    
> > > >    struct ucode_cpu_info;
> > > >    
> > > > diff --git a/arch/x86/kernel/cpu/common.c
> > > > b/arch/x86/kernel/cpu/common.c
> > > > index c296cb1c0113..86102a8d451e 100644
> > > > --- a/arch/x86/kernel/cpu/common.c
> > > > +++ b/arch/x86/kernel/cpu/common.c
> > > > @@ -598,23 +598,23 @@ __noendbr void ibt_restore(u64 save)
> > > >    
> > > > -__noendbr void cet_disable(void)
> > > > +__noendbr void ibt_disable(void)
> > > >    {
> > > >         if (cpu_feature_enabled(X86_FEATURE_IBT))
> > > >                 wrmsrl(MSR_IA32_S_CET, 0);
> > > 
> > > Not sure about this rename; it really disables all of (S) CET.
> > > 
> > > Specifically, once we do S-SHSTK (after FRED) we might also very
> > > much
> > > need to kill that for kexec.
> > 
> > Sure, what about something like sup_cet_disable()?
> 
> Why bother? Arguably kexec should clear U_CET too.

Hmm, I think you're right. It doesn't look the fpu stuff actually would
reset unknown xfeatures to init. So kernels with Kernel IBT would set
CR4.CET and then MSR_IA32_U_CET might make it to the point where
userspace would run with CET enabled.

It seems like a general kexec problem for when the kernel enables new
xfeatures. I suppose having vector instruction type data stick around
is not going to show up the same way as having new enforcement rules
applied.

But also, looking at this, the existing clearing of MSR_IA32_S_CET is
not sufficient, since it only does it on the cpu doing the kexec. I
think something like the below might be needed. Since per the other
discussion we are going to need to start setting CR4.CET whenever the
HW supports it, for KVM's benefit. So those other CPUs might get
supervisor IBT enabled if we don't clear the msr from every CPU.

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9730c88530fc..eb57d7f4fa6a 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -96,6 +96,12 @@ static void kdump_nmi_callback(int cpu, struct
pt_regs *regs)
        cpu_emergency_stop_pt();
 
        disable_local_APIC();
+
+       /*
+        * Make sure to disable CET features before kexec so the new
kernel
+        * doesn't get surprised by the enforcement.
+        */
+       cet_disable();
 }
 
 void kdump_nmi_shootdown_cpus(void)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 738226472468..de65bac0ae02 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -787,6 +787,12 @@ void __noreturn stop_this_cpu(void *dummy)
         */
        if (cpuid_eax(0x8000001f) & BIT(0))
                native_wbinvd();
+
+       /*
+        * Make sure to disable CET features before kexec so the new
kernel
+        * doesn't get surprised by the enforcement.
+        */
+       cet_disable();
        for (;;) {
                /*
                 * Use native_halt() so that memory contents don't
change
diff mbox series

Patch

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 8cbf623f0ecf..a56270838435 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -74,7 +74,7 @@  void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
 static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {}
 #endif
 
-extern __noendbr void cet_disable(void);
+extern __noendbr void ibt_disable(void);
 
 struct ucode_cpu_info;
 
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c296cb1c0113..86102a8d451e 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -598,23 +598,23 @@  __noendbr void ibt_restore(u64 save)
 
 static __always_inline void setup_cet(struct cpuinfo_x86 *c)
 {
+	bool kernel_ibt = HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT);
 	u64 msr = CET_ENDBR_EN;
 
-	if (!HAS_KERNEL_IBT ||
-	    !cpu_feature_enabled(X86_FEATURE_IBT))
-		return;
+	if (kernel_ibt)
+		wrmsrl(MSR_IA32_S_CET, msr);
 
-	wrmsrl(MSR_IA32_S_CET, msr);
-	cr4_set_bits(X86_CR4_CET);
+	if (kernel_ibt || cpu_feature_enabled(X86_FEATURE_SHSTK))
+		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;
 	}
 }
 
-__noendbr void cet_disable(void)
+__noendbr void ibt_disable(void)
 {
 	if (cpu_feature_enabled(X86_FEATURE_IBT))
 		wrmsrl(MSR_IA32_S_CET, 0);
diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 0611fd83858e..745024654fcd 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -311,7 +311,7 @@  void machine_kexec(struct kimage *image)
 	/* Interrupts aren't acceptable while we reboot */
 	local_irq_disable();
 	hw_breakpoint_disable();
-	cet_disable();
+	ibt_disable();
 
 	if (image->preserve_context) {
 #ifdef CONFIG_X86_IO_APIC