diff mbox series

[v2,04/39] x86/cpufeatures: Enable CET CR4 bit for shadow stack

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

Commit Message

Edgecombe, Rick P Sept. 29, 2022, 10:29 p.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.
 3. Allows CET to be enabled in guests

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

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>

---

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.

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

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

Comments

Kees Cook Oct. 3, 2022, 5:31 p.m. UTC | #1 
On Thu, Sep 29, 2022 at 03:29:01PM -0700, Rick Edgecombe wrote:
> 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.
>  3. Allows CET to be enabled in guests
> 
> 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 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.
> 
> 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>

Reviewed-by: Kees Cook <keescook@chromium.org>
Andrew Cooper Oct. 5, 2022, 12:55 a.m. UTC | #2 
On 29/09/2022 23:29, Rick Edgecombe wrote:
> 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.
>  3. Allows CET to be enabled in guests

Point 1, yes, but the others, not really.  Guests aren't interesting
because host CR4 != guest CR4.

CET is a tangled mess of control bits.  The MSRs can be configured and
context switched independently CR4.

The 4 main sub-feature enablement conditions are CR4.CET &&
MSR_{U,S}_CET.{SHSTK,ENDBR}_EN.

The WRUSS instruction is keyed on CR4.CET alone.  This is because
CR4.CET is the paging control which changes the interpretation of
R/O+Dirty, and is a prerequisite for any shstk memory accesses.  Most
other shstk instructions have finer grain enablement conditions.

I'd suggest simplifying the commit message massively, to say that
CR4.CET is a prerequisite for all CET operation, so extend setup_cet()
to enable it for user shadow stacks.

It hopefully goes without saying that you cannot do an equivalent piece
of code for supervisor shadow stacks.  If you try, you'll discover that
everything works fine until you try returning from the function which
activated the second of CR4.CET and MSR_S_CET.SHSTK_EN, and the valid
content on the shadow stack underflows.

~Andrew

P.S. There's a fun infoleak.

Userspace can probe for kernel shstk enablement using fault analysis on
the SETSSBUSY instruction.  It takes #UD for !CR4.CET ||
!MSR_S_CET.SHSTK_EN, and then #GP for CPL !=0.
Borislav Petkov Oct. 14, 2022, 5:12 p.m. UTC | #3 
On Thu, Sep 29, 2022 at 03:29:01PM -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);

So I'd love it if we can get rid of that HAS_KERNEL_IBT thing and use
the usual ifdeffery with Kconfig symbols. I wouldn't like for yet
another HAS_XXX feature checking method to proliferate as this is the
only one:

$ git grep -E "\WHAS_" arch/x86/
arch/x86/include/asm/ibt.h:18: * When all the above are satisfied, HAS_KERNEL_IBT will be 1, otherwise 0.
arch/x86/include/asm/ibt.h:22:#define HAS_KERNEL_IBT    1
arch/x86/include/asm/ibt.h:92:#define HAS_KERNEL_IBT    0
arch/x86/include/asm/ibt.h:114:#define ENDBR_INSN_SIZE          (4*HAS_KERNEL_IBT)
arch/x86/include/asm/idtentry.h:8:#define IDT_ALIGN     (8 * (1 + HAS_KERNEL_IBT))
arch/x86/kernel/cpu/common.c:601:       bool kernel_ibt = HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT);
arch/x86/kernel/cpu/common.c:1942:      if (HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT))

>  __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);
>  }
>  
> +

Stray newline.
Edgecombe, Rick P Oct. 14, 2022, 6:15 p.m. UTC | #4 
On Fri, 2022-10-14 at 19:12 +0200, Borislav Petkov wrote:
> On Thu, Sep 29, 2022 at 03:29:01PM -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);
> 
> So I'd love it if we can get rid of that HAS_KERNEL_IBT thing and use
> the usual ifdeffery with Kconfig symbols. I wouldn't like for yet
> another HAS_XXX feature checking method to proliferate as this is the
> only one:

Andrew Cooper has suggested to create some software cpu features to
differentiate user/supervisor CET feature use. It could replace
HAS_KERNEL_IBT. Any objections to that versus Kconfig symbols?

[snip]

> cpu_feature_enabled(X86_FEATURE_IBT))
> 
> >   __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);
> >   }
> >   
> > +
> 
> Stray newline.

Oops, will clean that up. Thanks.
Borislav Petkov Oct. 14, 2022, 7:44 p.m. UTC | #5 
On Fri, Oct 14, 2022 at 06:15:30PM +0000, Edgecombe, Rick P wrote:
> Andrew Cooper has suggested to create some software cpu features to
> differentiate user/supervisor CET feature use. It could replace
> HAS_KERNEL_IBT. Any objections to that versus Kconfig symbols?

Sure, except you can't use them in

arch/x86/include/asm/idtentry.h:8:#define IDT_ALIGN     (8 * (1 + HAS_KERNEL_IBT))

as that gets used in asm code.

But you don't have to do this in this patchset - it is huge already
anyway. I have this thing on my todo so I'll get to it eventually.
Unless you're itching to remove it yourself - then I won't stay in the
way.

:-)

Thx.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 3e508f239098..d7415bb556b2 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -598,16 +598,21 @@  __noendbr void ibt_restore(u64 save)
 
 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 = IS_ENABLED(CONFIG_X86_SHADOW_STACK) &&
+			  cpu_feature_enabled(X86_FEATURE_SHSTK);
+	u64 msr = 0;
 
-	if (!HAS_KERNEL_IBT ||
-	    !cpu_feature_enabled(X86_FEATURE_IBT))
+	if (!kernel_ibt && !user_shstk)
 		return;
 
+	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;
@@ -616,10 +621,15 @@  static __always_inline void setup_cet(struct cpuinfo_x86 *c)
 
 __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);
 }
 
+
 /*
  * Some CPU features depend on higher CPUID levels, which may not always
  * be available due to CPUID level capping or broken virtualization