| Message ID | 20221203003606.6838-8-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Fri, Dec 02, 2022 at 04:35:34PM -0800, Rick Edgecombe wrote: > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > A control-protection fault is triggered when a control-flow transfer > attempt violates Shadow Stack or Indirect Branch Tracking constraints. > For example, the return address for a RET instruction differs from the copy > on the shadow stack. > > There already exists a control-protection fault handler for handling kernel > IBT. Refactor this fault handler into sparate user and kernel handlers, > like the page fault handler. Add a control-protection handler for usermode. > > Keep the same behavior for the kernel side of the fault handler, except for > converting a BUG to a WARN in the case of a #CP happening when > !cpu_feature_enabled(). This unifies the behavior with the new shadow stack > code, and also prevents the kernel from crashing under this situation which > is potentially recoverable. > > The control-protection fault handler works in a similar way as the general > protection fault handler. It provides the si_code SEGV_CPERR to the signal > handler. > > 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> This looks nice cleaner to me. Thanks! Reviewed-by: Kees Cook <keescook@chromium.org>
On Fri, Dec 02, 2022 at 04:35:34PM -0800, Rick Edgecombe wrote: > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > A control-protection fault is triggered when a control-flow transfer > attempt violates Shadow Stack or Indirect Branch Tracking constraints. > For example, the return address for a RET instruction differs from the copy > on the shadow stack. > > There already exists a control-protection fault handler for handling kernel > IBT. Refactor this fault handler into sparate user and kernel handlers, Unknown word [sparate] in commit message. Suggestions: ['separate', You could use a spellchecker with your commit messages so that it catches all those typos. ... > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index 8b83d8fbce71..e35c70dc1afb 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -213,12 +213,7 @@ DEFINE_IDTENTRY(exc_overflow) > do_error_trap(regs, 0, "overflow", X86_TRAP_OF, SIGSEGV, 0, NULL); > } > > -#ifdef CONFIG_X86_KERNEL_IBT > - > -static __ro_after_init bool ibt_fatal = true; > - > -extern void ibt_selftest_ip(void); /* code label defined in asm below */ > - > +#ifdef CONFIG_X86_CET > enum cp_error_code { > CP_EC = (1 << 15) - 1, > > @@ -231,15 +226,87 @@ enum cp_error_code { > CP_ENCL = 1 << 15, > }; > > -DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) > +static const char control_protection_err[][10] = { You already use the "cp_" prefix for the other things, might as well use it here too. > + [0] = "unknown", > + [1] = "near ret", > + [2] = "far/iret", > + [3] = "endbranch", > + [4] = "rstorssp", > + [5] = "setssbsy", > +}; > + > +static const char *cp_err_string(unsigned long error_code) > +{ > + unsigned int cpec = error_code & CP_EC; > + > + if (cpec >= ARRAY_SIZE(control_protection_err)) > + cpec = 0; > + return control_protection_err[cpec]; > +} > + > +static void do_unexpected_cp(struct pt_regs *regs, unsigned long error_code) > +{ > + WARN_ONCE(1, "Unexpected %s #CP, error_code: %s\n", > + user_mode(regs) ? "user mode" : "kernel mode", > + cp_err_string(error_code)); > +} > +#endif /* CONFIG_X86_CET */ > + > +void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code); What's that forward declaration for? In any case, push it into traps.h pls. I gotta say, I'm not a big fan of that ifdeffery here. Do we really really need it? > +#ifdef CONFIG_X86_USER_SHADOW_STACK > +static DEFINE_RATELIMIT_STATE(cpf_rate, DEFAULT_RATELIMIT_INTERVAL, > + DEFAULT_RATELIMIT_BURST); > + > +void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code) > { > - if (!cpu_feature_enabled(X86_FEATURE_IBT)) { > - pr_err("Unexpected #CP\n"); > - BUG(); > + struct task_struct *tsk; > + unsigned long ssp; > + > + /* > + * An exception was just taken from userspace. Since interrupts are disabled > + * here, no scheduling should have messed with the registers yet and they > + * will be whatever is live in userspace. So read the SSP before enabling > + * interrupts so locking the fpregs to do it later is not required. > + */ > + rdmsrl(MSR_IA32_PL3_SSP, ssp); > + > + cond_local_irq_enable(regs); > + > + tsk = current; > + tsk->thread.error_code = error_code; > + tsk->thread.trap_nr = X86_TRAP_CP; > + > + /* Ratelimit to prevent log spamming. */ > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && > + __ratelimit(&cpf_rate)) { > + pr_emerg("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)%s", > + tsk->comm, task_pid_nr(tsk), > + regs->ip, regs->sp, ssp, error_code, > + cp_err_string(error_code), > + error_code & CP_ENCL ? " in enclave" : ""); > + print_vma_addr(KERN_CONT " in ", regs->ip); > + pr_cont("\n"); > } > > - if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) != CP_ENDBR)) > + force_sig_fault(SIGSEGV, SEGV_CPERR, (void __user *)0); > + cond_local_irq_disable(regs); > +} > +#endif > + > +void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code); > + > +#ifdef CONFIG_X86_KERNEL_IBT > +static __ro_after_init bool ibt_fatal = true; > + > +extern void ibt_selftest_ip(void); /* code label defined in asm below */ Yeah, pls put that comment above the function. Side comments are nasty. Thx.
On Fri, Dec 02, 2022 at 04:35:34PM -0800, Rick Edgecombe wrote: > Keep the same behavior for the kernel side of the fault handler, except for > converting a BUG to a WARN in the case of a #CP happening when > !cpu_feature_enabled(). ^^^^^^^^^^^^^^^^^^^^^^^^^ Yeah, let's stick to plain english in the commit message pls, instead of pseudo code. Thx.
On Tue, 2022-12-20 at 17:19 +0100, Borislav Petkov wrote: > On Fri, Dec 02, 2022 at 04:35:34PM -0800, Rick Edgecombe wrote: > > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > > A control-protection fault is triggered when a control-flow > > transfer > > attempt violates Shadow Stack or Indirect Branch Tracking > > constraints. > > For example, the return address for a RET instruction differs from > > the copy > > on the shadow stack. > > > > There already exists a control-protection fault handler for > > handling kernel > > IBT. Refactor this fault handler into separate user and kernel > > handlers, > > Unknown word [separate] in commit message. > Suggestions: ['separate', > > You could use a spellchecker with your commit messages so that it > catches all those typos. Argh, sorry. I'll upgrade my tools. > > ... > > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > index 8b83d8fbce71..e35c70dc1afb 100644 > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > @@ -213,12 +213,7 @@ DEFINE_IDTENTRY(exc_overflow) > > do_error_trap(regs, 0, "overflow", X86_TRAP_OF, SIGSEGV, 0, > > NULL); > > } > > > > -#ifdef CONFIG_X86_KERNEL_IBT > > - > > -static __ro_after_init bool ibt_fatal = true; > > - > > -extern void ibt_selftest_ip(void); /* code label defined in asm > > below */ > > - > > +#ifdef CONFIG_X86_CET > > enum cp_error_code { > > CP_EC = (1 << 15) - 1, > > > > @@ -231,15 +226,87 @@ enum cp_error_code { > > CP_ENCL = 1 << 15, > > }; > > > > -DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) > > +static const char control_protection_err[][10] = { > > You already use the "cp_" prefix for the other things, might as well > use > it here too. Sure. > > > + [0] = "unknown", > > + [1] = "near ret", > > + [2] = "far/iret", > > + [3] = "endbranch", > > + [4] = "rstorssp", > > + [5] = "setssbsy", > > +}; > > + > > +static const char *cp_err_string(unsigned long error_code) > > +{ > > + unsigned int cpec = error_code & CP_EC; > > + > > + if (cpec >= ARRAY_SIZE(control_protection_err)) > > + cpec = 0; > > + return control_protection_err[cpec]; > > +} > > + > > +static void do_unexpected_cp(struct pt_regs *regs, unsigned long > > error_code) > > +{ > > + WARN_ONCE(1, "Unexpected %s #CP, error_code: %s\n", > > + user_mode(regs) ? "user mode" : "kernel mode", > > + cp_err_string(error_code)); > > +} > > +#endif /* CONFIG_X86_CET */ > > + > > +void do_user_cp_fault(struct pt_regs *regs, unsigned long > > error_code); > > What's that forward declaration for? The reason is cpu_feature_enabled(X86_FEATURE_USER_SHSTK) will compile- time evaluate to false when user shadow stack is not configured, so a do_user_cp_fault() implementation is not needed in that case. The reference in exec_control_protection will get optimized out, but it still needs to be defined. Otherwise it could have a stub implementation in an #else block, but this seemed cleaner. > > In any case, push it into traps.h pls. It's not really supposed to be called from outside traps.c. The only reason it is not static it because it doesn't work with these IS_ENABLED()-style solutions for compiling out code. Now I'm wondering if these are not preferred... > > I gotta say, I'm not a big fan of that ifdeffery here. Do we really > really need it? You mean having separate paths for kernel IBT and user shadow stack that compile out? I guess it could just all be in place if CONFIG_X86_CET is in place. I don't know, I thought it was relatively clean, but I can remove it. > > > +#ifdef CONFIG_X86_USER_SHADOW_STACK > > +static DEFINE_RATELIMIT_STATE(cpf_rate, > > DEFAULT_RATELIMIT_INTERVAL, > > + DEFAULT_RATELIMIT_BURST); > > + > > +void do_user_cp_fault(struct pt_regs *regs, unsigned long > > error_code) > > { > > - if (!cpu_feature_enabled(X86_FEATURE_IBT)) { > > - pr_err("Unexpected #CP\n"); > > - BUG(); > > + struct task_struct *tsk; > > + unsigned long ssp; > > + > > + /* > > + * An exception was just taken from userspace. Since > > interrupts are disabled > > + * here, no scheduling should have messed with the > > registers yet and they > > + * will be whatever is live in userspace. So read the SSP > > before enabling > > + * interrupts so locking the fpregs to do it later is not > > required. > > + */ > > + rdmsrl(MSR_IA32_PL3_SSP, ssp); > > + > > + cond_local_irq_enable(regs); > > + > > + tsk = current; > > + tsk->thread.error_code = error_code; > > + tsk->thread.trap_nr = X86_TRAP_CP; > > + > > + /* Ratelimit to prevent log spamming. */ > > + if (show_unhandled_signals && unhandled_signal(tsk, > > SIGSEGV) && > > + __ratelimit(&cpf_rate)) { > > + pr_emerg("%s[%d] control protection ip:%lx sp:%lx > > ssp:%lx error:%lx(%s)%s", > > + tsk->comm, task_pid_nr(tsk), > > + regs->ip, regs->sp, ssp, error_code, > > + cp_err_string(error_code), > > + error_code & CP_ENCL ? " in enclave" : > > ""); > > + print_vma_addr(KERN_CONT " in ", regs->ip); > > + pr_cont("\n"); > > } > > > > - if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) != > > CP_ENDBR)) > > + force_sig_fault(SIGSEGV, SEGV_CPERR, (void __user *)0); > > + cond_local_irq_disable(regs); > > +} > > +#endif > > + > > +void do_kernel_cp_fault(struct pt_regs *regs, unsigned long > > error_code); > > + > > +#ifdef CONFIG_X86_KERNEL_IBT > > +static __ro_after_init bool ibt_fatal = true; > > + > > +extern void ibt_selftest_ip(void); /* code label defined in asm > > below */ > > Yeah, pls put that comment above the function. Side comments are > nasty. > > Thx. > Sure. Thanks.
On Tue, 2022-12-20 at 22:21 +0100, Borislav Petkov wrote: > On Fri, Dec 02, 2022 at 04:35:34PM -0800, Rick Edgecombe wrote: > > Keep the same behavior for the kernel side of the fault handler, > > except for > > converting a BUG to a WARN in the case of a #CP happening when > > !cpu_feature_enabled(). > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > Yeah, let's stick to plain english in the commit message pls, instead > of > pseudo code. Sure, I'll change it. Thanks.
On Wed, Dec 21, 2022 at 12:37:51AM +0000, Edgecombe, Rick P wrote: > You mean having separate paths for kernel IBT and user shadow stack > that compile out? I guess it could just all be in place if > CONFIG_X86_CET is in place. > > I don't know, I thought it was relatively clean, but I can remove it. Yeah, I'm wondering if we really need the ifdeffery. I always question ifdeffery because it is a) ugly, b) a mess to deal with and having it is not really worth it. Yeah, we save a couple of KBs, big deal. What would practically happen is, shadow stack will be default-enabled on the majority of kernels out there - distro ones - so it will be enabled practically everywhere. And it'll be off only in some self-built kernels which are the very small minority. And how much are the space savings with the whole set applied, with and without the Kconfig item enabled? Probably only a couple of KBs. And if so, I'm thinking we could at least make the traps.c stuff unconditional - it'll be there but won't run. Unless we get some weird #CP but it'll be caught by do_unexpected_cp(). And you have feature tests everywhere so it's not like it'll get "misused". And when you do that, you'll have everything a lot simpler, a lot less Kconfig items to build-test and all good. Right? Or am I completely way off into the weeds here and am missing an important aspect...? Thx.
On Wed, 2022-12-21 at 11:41 +0100, Borislav Petkov wrote: > On Wed, Dec 21, 2022 at 12:37:51AM +0000, Edgecombe, Rick P wrote: > > You mean having separate paths for kernel IBT and user shadow stack > > that compile out? I guess it could just all be in place if > > CONFIG_X86_CET is in place. > > > > I don't know, I thought it was relatively clean, but I can remove > > it. > > Yeah, I'm wondering if we really need the ifdeffery. I always > question > ifdeffery because it is a) ugly, b) a mess to deal with and having it > is > not really worth it. Yeah, we save a couple of KBs, big deal. > > What would practically happen is, shadow stack will be default- > enabled > on the majority of kernels out there - distro ones - so it will be > enabled practically everywhere. > > And it'll be off only in some self-built kernels which are the very > small minority. > > And how much are the space savings with the whole set applied, with > and > without the Kconfig item enabled? Probably only a couple of KBs. Oh, you mean the whole Kconfig thing. Yea, I mean I see the point about typical configs. But at least CONFIG_X86_CET seems consistent with CONFIG_INTEL_TDX_GUEST, CONFIG_IOMMU_SVA, etc. What about moving it out of traps.c to a cet.c, like exc_vmm_communication for CONFIG_AMD_MEM_ENCRT? Then the inclusion logic lives in the build files, instead of an ifdef. > > And if so, I'm thinking we could at least make the traps.c stuff > unconditional - it'll be there but won't run. Unless we get some > weird > #CP but it'll be caught by do_unexpected_cp(). > > And you have feature tests everywhere so it's not like it'll get > "misused". > > And when you do that, you'll have everything a lot simpler, a lot > less > Kconfig items to build-test and all good. > > Right? > > Or am I completely way off into the weeds here and am missing an > important aspect...? > One aspect that has come up a couple of times, is how closely related all these CET features are (or aren't). Shadow stack and IBT are mostly separate, but do share an xfeature and an exception type. Similarly for supervisor and user mode support for either of the CET features. So maybe that is what is unusual here. There are some aspects that make them look like separate features, which leads people to think they should be separate in the code. But actually separating them leads to excess ifdefery. To me, putting the whole handler in even if there are no CET features seems like too much. Leaving it in unconditionally is apparently also inconsistent with some of the previous traps.c decisions. So I'd leave CONFIG_X86_CET at least and it can help anyone building super stripped down kernels. But I'm ok with whatever people think.
On Wed, Dec 21, 2022 at 09:42:50PM +0000, Edgecombe, Rick P wrote: > Oh, you mean the whole Kconfig thing. Yea, I mean I see the point about > typical configs. But at least CONFIG_X86_CET seems consistent with > CONFIG_INTEL_TDX_GUEST, CONFIG_IOMMU_SVA, etc. > > What about moving it out of traps.c to a cet.c, like > exc_vmm_communication for CONFIG_AMD_MEM_ENCRT? Then the inclusion > logic lives in the build files, instead of an ifdef. Yeah, that definitely sounds cleaner. Another example would be the #MC handler being in mce code and not in traps.c. So yeah, the reason why I'm even mentioning this is that I get an allergic reaction when I see unwieldy ifdeffery in one screen worth of code. But this is just me. :) > One aspect that has come up a couple of times, is how closely related > all these CET features are (or aren't). Shadow stack and IBT are mostly > separate, but do share an xfeature and an exception type. Similarly for > supervisor and user mode support for either of the CET features. So > maybe that is what is unusual here. There are some aspects that make > them look like separate features, which leads people to think they > should be separate in the code. But actually separating them leads to > excess ifdefery. Yeah, I think you solved that correctly by having the common X86_CET symbol selected by the two. Thx.
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c index e07f359254c3..9a3c9de5ac5e 100644 --- a/arch/arm/kernel/signal.c +++ b/arch/arm/kernel/signal.c @@ -681,7 +681,7 @@ asmlinkage void do_rseq_syscall(struct pt_regs *regs) */ static_assert(NSIGILL == 11); static_assert(NSIGFPE == 15); -static_assert(NSIGSEGV == 9); +static_assert(NSIGSEGV == 10); static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 9ad911f1647c..81b13a21046e 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -1166,7 +1166,7 @@ void __init minsigstksz_setup(void) */ static_assert(NSIGILL == 11); static_assert(NSIGFPE == 15); -static_assert(NSIGSEGV == 9); +static_assert(NSIGSEGV == 10); static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index 4700f8522d27..bbd542704730 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c @@ -460,7 +460,7 @@ void compat_setup_restart_syscall(struct pt_regs *regs) */ static_assert(NSIGILL == 11); static_assert(NSIGFPE == 15); -static_assert(NSIGSEGV == 9); +static_assert(NSIGSEGV == 10); static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); diff --git a/arch/sparc/kernel/signal32.c b/arch/sparc/kernel/signal32.c index dad38960d1a8..82da8a2d769d 100644 --- a/arch/sparc/kernel/signal32.c +++ b/arch/sparc/kernel/signal32.c @@ -751,7 +751,7 @@ asmlinkage int do_sys32_sigstack(u32 u_ssptr, u32 u_ossptr, unsigned long sp) */ static_assert(NSIGILL == 11); static_assert(NSIGFPE == 15); -static_assert(NSIGSEGV == 9); +static_assert(NSIGSEGV == 10); static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); diff --git a/arch/sparc/kernel/signal_64.c b/arch/sparc/kernel/signal_64.c index 570e43e6fda5..b4e410976e0d 100644 --- a/arch/sparc/kernel/signal_64.c +++ b/arch/sparc/kernel/signal_64.c @@ -562,7 +562,7 @@ void do_notify_resume(struct pt_regs *regs, unsigned long orig_i0, unsigned long */ static_assert(NSIGILL == 11); static_assert(NSIGFPE == 15); -static_assert(NSIGSEGV == 9); +static_assert(NSIGSEGV == 10); static_assert(NSIGBUS == 5); static_assert(NSIGTRAP == 6); static_assert(NSIGCHLD == 6); diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h index 7a2954a16cb7..b7646b471537 100644 --- a/arch/x86/include/asm/disabled-features.h +++ b/arch/x86/include/asm/disabled-features.h @@ -105,6 +105,12 @@ #define DISABLE_USER_SHSTK (1 << (X86_FEATURE_USER_SHSTK & 31)) #endif +#ifdef CONFIG_X86_KERNEL_IBT +#define DISABLE_IBT 0 +#else +#define DISABLE_IBT (1 << (X86_FEATURE_IBT & 31)) +#endif + /* * Make sure to add features to the correct mask */ @@ -128,7 +134,7 @@ #define DISABLED_MASK16 (DISABLE_PKU|DISABLE_OSPKE|DISABLE_LA57|DISABLE_UMIP| \ DISABLE_ENQCMD) #define DISABLED_MASK17 0 -#define DISABLED_MASK18 0 +#define DISABLED_MASK18 (DISABLE_IBT) #define DISABLED_MASK19 0 #define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 20) diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h index 72184b0b2219..69e26f48d027 100644 --- a/arch/x86/include/asm/idtentry.h +++ b/arch/x86/include/asm/idtentry.h @@ -618,7 +618,7 @@ DECLARE_IDTENTRY_RAW_ERRORCODE(X86_TRAP_DF, xenpv_exc_double_fault); #endif /* #CP */ -#ifdef CONFIG_X86_KERNEL_IBT +#ifdef CONFIG_X86_CET DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_CP, exc_control_protection); #endif diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c index a58c6bc1cd68..5074b8420359 100644 --- a/arch/x86/kernel/idt.c +++ b/arch/x86/kernel/idt.c @@ -107,7 +107,7 @@ static const __initconst struct idt_data def_idts[] = { ISTG(X86_TRAP_MC, asm_exc_machine_check, IST_INDEX_MCE), #endif -#ifdef CONFIG_X86_KERNEL_IBT +#ifdef CONFIG_X86_CET INTG(X86_TRAP_CP, asm_exc_control_protection), #endif diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c index 879ef8c72f5c..d441804443d5 100644 --- a/arch/x86/kernel/signal_compat.c +++ b/arch/x86/kernel/signal_compat.c @@ -27,7 +27,7 @@ static inline void signal_compat_build_tests(void) */ BUILD_BUG_ON(NSIGILL != 11); BUILD_BUG_ON(NSIGFPE != 15); - BUILD_BUG_ON(NSIGSEGV != 9); + BUILD_BUG_ON(NSIGSEGV != 10); BUILD_BUG_ON(NSIGBUS != 5); BUILD_BUG_ON(NSIGTRAP != 6); BUILD_BUG_ON(NSIGCHLD != 6); diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 8b83d8fbce71..e35c70dc1afb 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -213,12 +213,7 @@ DEFINE_IDTENTRY(exc_overflow) do_error_trap(regs, 0, "overflow", X86_TRAP_OF, SIGSEGV, 0, NULL); } -#ifdef CONFIG_X86_KERNEL_IBT - -static __ro_after_init bool ibt_fatal = true; - -extern void ibt_selftest_ip(void); /* code label defined in asm below */ - +#ifdef CONFIG_X86_CET enum cp_error_code { CP_EC = (1 << 15) - 1, @@ -231,15 +226,87 @@ enum cp_error_code { CP_ENCL = 1 << 15, }; -DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) +static const char control_protection_err[][10] = { + [0] = "unknown", + [1] = "near ret", + [2] = "far/iret", + [3] = "endbranch", + [4] = "rstorssp", + [5] = "setssbsy", +}; + +static const char *cp_err_string(unsigned long error_code) +{ + unsigned int cpec = error_code & CP_EC; + + if (cpec >= ARRAY_SIZE(control_protection_err)) + cpec = 0; + return control_protection_err[cpec]; +} + +static void do_unexpected_cp(struct pt_regs *regs, unsigned long error_code) +{ + WARN_ONCE(1, "Unexpected %s #CP, error_code: %s\n", + user_mode(regs) ? "user mode" : "kernel mode", + cp_err_string(error_code)); +} +#endif /* CONFIG_X86_CET */ + +void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code); + +#ifdef CONFIG_X86_USER_SHADOW_STACK +static DEFINE_RATELIMIT_STATE(cpf_rate, DEFAULT_RATELIMIT_INTERVAL, + DEFAULT_RATELIMIT_BURST); + +void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code) { - if (!cpu_feature_enabled(X86_FEATURE_IBT)) { - pr_err("Unexpected #CP\n"); - BUG(); + struct task_struct *tsk; + unsigned long ssp; + + /* + * An exception was just taken from userspace. Since interrupts are disabled + * here, no scheduling should have messed with the registers yet and they + * will be whatever is live in userspace. So read the SSP before enabling + * interrupts so locking the fpregs to do it later is not required. + */ + rdmsrl(MSR_IA32_PL3_SSP, ssp); + + cond_local_irq_enable(regs); + + tsk = current; + tsk->thread.error_code = error_code; + tsk->thread.trap_nr = X86_TRAP_CP; + + /* Ratelimit to prevent log spamming. */ + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && + __ratelimit(&cpf_rate)) { + pr_emerg("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)%s", + tsk->comm, task_pid_nr(tsk), + regs->ip, regs->sp, ssp, error_code, + cp_err_string(error_code), + error_code & CP_ENCL ? " in enclave" : ""); + print_vma_addr(KERN_CONT " in ", regs->ip); + pr_cont("\n"); } - if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) != CP_ENDBR)) + force_sig_fault(SIGSEGV, SEGV_CPERR, (void __user *)0); + cond_local_irq_disable(regs); +} +#endif + +void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code); + +#ifdef CONFIG_X86_KERNEL_IBT +static __ro_after_init bool ibt_fatal = true; + +extern void ibt_selftest_ip(void); /* code label defined in asm below */ + +void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code) +{ + if ((error_code & CP_EC) != CP_ENDBR) { + do_unexpected_cp(regs, error_code); return; + } if (unlikely(regs->ip == (unsigned long)&ibt_selftest_ip)) { regs->ax = 0; @@ -285,9 +352,25 @@ static int __init ibt_setup(char *str) } __setup("ibt=", ibt_setup); - #endif /* CONFIG_X86_KERNEL_IBT */ +#ifdef CONFIG_X86_CET +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) +{ + if (user_mode(regs)) { + if (cpu_feature_enabled(X86_FEATURE_USER_SHSTK)) + do_user_cp_fault(regs, error_code); + else + do_unexpected_cp(regs, error_code); + } else { + if (cpu_feature_enabled(X86_FEATURE_IBT)) + do_kernel_cp_fault(regs, error_code); + else + do_unexpected_cp(regs, error_code); + } +} +#endif /* CONFIG_X86_CET */ + #ifdef CONFIG_X86_F00F_BUG void handle_invalid_op(struct pt_regs *regs) #else diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index a7d83c7800e4..e58d6cd30853 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -639,7 +639,7 @@ static struct trap_array_entry trap_array[] = { TRAP_ENTRY(exc_coprocessor_error, false ), TRAP_ENTRY(exc_alignment_check, false ), TRAP_ENTRY(exc_simd_coprocessor_error, false ), -#ifdef CONFIG_X86_KERNEL_IBT +#ifdef CONFIG_X86_CET TRAP_ENTRY(exc_control_protection, false ), #endif }; diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S index 4a184f6e4e4d..7cdcb4ce6976 100644 --- a/arch/x86/xen/xen-asm.S +++ b/arch/x86/xen/xen-asm.S @@ -148,7 +148,7 @@ xen_pv_trap asm_exc_page_fault xen_pv_trap asm_exc_spurious_interrupt_bug xen_pv_trap asm_exc_coprocessor_error xen_pv_trap asm_exc_alignment_check -#ifdef CONFIG_X86_KERNEL_IBT +#ifdef CONFIG_X86_CET xen_pv_trap asm_exc_control_protection #endif #ifdef CONFIG_X86_MCE diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h index ffbe4cec9f32..0f52d0ac47c5 100644 --- a/include/uapi/asm-generic/siginfo.h +++ b/include/uapi/asm-generic/siginfo.h @@ -242,7 +242,8 @@ typedef struct siginfo { #define SEGV_ADIPERR 7 /* Precise MCD exception */ #define SEGV_MTEAERR 8 /* Asynchronous ARM MTE error */ #define SEGV_MTESERR 9 /* Synchronous ARM MTE exception */ -#define NSIGSEGV 9 +#define SEGV_CPERR 10 /* Control protection fault */ +#define NSIGSEGV 10 /* * SIGBUS si_codes