| Message ID | 20210203225547.32221-7-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Shadow Stack | expand |
On Wed, Feb 03, 2021 at 02:55:28PM -0800, Yu-cheng Yu wrote: > 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; or an indirect JMP instruction, without the NOTRACK > prefix, arrives at a non-ENDBR opcode. > > 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. > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Cc: Michael Kerrisk <mtk.manpages@gmail.com> > --- > arch/x86/include/asm/idtentry.h | 4 ++ > arch/x86/kernel/idt.c | 4 ++ > arch/x86/kernel/signal_compat.c | 2 +- > arch/x86/kernel/traps.c | 60 ++++++++++++++++++++++++++++++ > include/uapi/asm-generic/siginfo.h | 3 +- > 5 files changed, 71 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h > index f656aabd1545..ff4b3bf634da 100644 > --- a/arch/x86/include/asm/idtentry.h > +++ b/arch/x86/include/asm/idtentry.h > @@ -574,6 +574,10 @@ DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_SS, exc_stack_segment); > DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_GP, exc_general_protection); > DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_AC, exc_alignment_check); > > +#ifdef CONFIG_X86_CET > +DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_CP, exc_control_protection); > +#endif > + > /* Raw exception entries which need extra work */ > DECLARE_IDTENTRY_RAW(X86_TRAP_UD, exc_invalid_op); > DECLARE_IDTENTRY_RAW(X86_TRAP_BP, exc_int3); > diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c > index ee1a283f8e96..e8166d9bbb10 100644 > --- a/arch/x86/kernel/idt.c > +++ b/arch/x86/kernel/idt.c > @@ -105,6 +105,10 @@ static const __initconst struct idt_data def_idts[] = { > #elif defined(CONFIG_X86_32) > SYSG(IA32_SYSCALL_VECTOR, entry_INT80_32), > #endif > + > +#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 a5330ff498f0..dd92490b1e7f 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 != 5); > BUILD_BUG_ON(NSIGCHLD != 6); > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index 7f5aec758f0e..f5354c35df32 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -606,6 +606,66 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection) > cond_local_irq_disable(regs); > } > > +#ifdef CONFIG_X86_CET > +static const char * const control_protection_err[] = { > + "unknown", > + "near-ret", > + "far-ret/iret", > + "endbranch", > + "rstorssp", > + "setssbsy", > +}; > + > +/* > + * When a control protection exception occurs, send a signal to the responsible > + * application. Currently, control protection is only enabled for user mode. > + * This exception should not come from kernel mode. > + */ > +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) > +{ > + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, > + DEFAULT_RATELIMIT_BURST); > + struct task_struct *tsk; > + > + if (!user_mode(regs)) { > + pr_emerg("PANIC: unexpected kernel control protection fault\n"); > + die("kernel control protection fault", regs, error_code); > + panic("Machine halted."); > + } > + > + cond_local_irq_enable(regs); > + > + if (!boot_cpu_has(X86_FEATURE_CET)) > + WARN_ONCE(1, "Control protection fault with CET support disabled\n"); > + > + tsk = current; > + tsk->thread.error_code = error_code; > + tsk->thread.trap_nr = X86_TRAP_CP; > + > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && > + __ratelimit(&rs)) { > + unsigned int max_err; > + unsigned long ssp; > + > + max_err = ARRAY_SIZE(control_protection_err) - 1; > + if (error_code < 0 || error_code > max_err) > + error_code = 0; Do you want to mask the error_code here before printing its value? > + > + rdmsrl(MSR_IA32_PL3_SSP, ssp); > + pr_emerg("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)", > + tsk->comm, task_pid_nr(tsk), > + regs->ip, regs->sp, ssp, error_code, > + control_protection_err[error_code]); Instead, you could clamp error_code to ARRAY_SIZE(control_protection_err), and add another "unknown" to the end of the strings: control_protection_err[ array_index_nospec(error_code, ARRAY_SIZE(control_protection_err))] Everything else looks good. > + print_vma_addr(KERN_CONT " in ", regs->ip); > + pr_cont("\n"); > + } > + > + force_sig_fault(SIGSEGV, SEGV_CPERR, > + (void __user *)uprobe_get_trap_addr(regs)); > + cond_local_irq_disable(regs); > +} > +#endif > + > static bool do_int3(struct pt_regs *regs) > { > int res; > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > index d2597000407a..1c2ea91284a0 100644 > --- a/include/uapi/asm-generic/siginfo.h > +++ b/include/uapi/asm-generic/siginfo.h > @@ -231,7 +231,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 > -- > 2.21.0 >
On 2/4/2021 12:09 PM, Kees Cook wrote: > On Wed, Feb 03, 2021 at 02:55:28PM -0800, Yu-cheng Yu wrote: [...] >> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c >> index 7f5aec758f0e..f5354c35df32 100644 >> --- a/arch/x86/kernel/traps.c >> +++ b/arch/x86/kernel/traps.c >> @@ -606,6 +606,66 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection) >> cond_local_irq_disable(regs); >> } >> >> +#ifdef CONFIG_X86_CET >> +static const char * const control_protection_err[] = { >> + "unknown", >> + "near-ret", >> + "far-ret/iret", >> + "endbranch", >> + "rstorssp", >> + "setssbsy", >> +}; >> + >> +/* >> + * When a control protection exception occurs, send a signal to the responsible >> + * application. Currently, control protection is only enabled for user mode. >> + * This exception should not come from kernel mode. >> + */ >> +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) >> +{ >> + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, >> + DEFAULT_RATELIMIT_BURST); >> + struct task_struct *tsk; >> + >> + if (!user_mode(regs)) { >> + pr_emerg("PANIC: unexpected kernel control protection fault\n"); >> + die("kernel control protection fault", regs, error_code); >> + panic("Machine halted."); >> + } >> + >> + cond_local_irq_enable(regs); >> + >> + if (!boot_cpu_has(X86_FEATURE_CET)) >> + WARN_ONCE(1, "Control protection fault with CET support disabled\n"); >> + >> + tsk = current; >> + tsk->thread.error_code = error_code; >> + tsk->thread.trap_nr = X86_TRAP_CP; >> + >> + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && >> + __ratelimit(&rs)) { >> + unsigned int max_err; >> + unsigned long ssp; >> + >> + max_err = ARRAY_SIZE(control_protection_err) - 1; >> + if (error_code < 0 || error_code > max_err) >> + error_code = 0; > > Do you want to mask the error_code here before printing its value? > >> + >> + rdmsrl(MSR_IA32_PL3_SSP, ssp); >> + pr_emerg("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)", >> + tsk->comm, task_pid_nr(tsk), >> + regs->ip, regs->sp, ssp, error_code, >> + control_protection_err[error_code]); > > Instead, you could clamp error_code to ARRAY_SIZE(control_protection_err), > and add another "unknown" to the end of the strings: > > control_protection_err[ > array_index_nospec(error_code, > ARRAY_SIZE(control_protection_err))] > > Everything else looks good. > I will update it. Thanks! [...]
On Wed, Feb 03, 2021 at 02:55:28PM -0800, Yu-cheng Yu wrote: > +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) > +{ > + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, > + DEFAULT_RATELIMIT_BURST); > + struct task_struct *tsk; > + > + if (!user_mode(regs)) { > + pr_emerg("PANIC: unexpected kernel control protection fault\n"); > + die("kernel control protection fault", regs, error_code); > + panic("Machine halted."); > + } > + > + cond_local_irq_enable(regs); > + > + if (!boot_cpu_has(X86_FEATURE_CET)) > + WARN_ONCE(1, "Control protection fault with CET support disabled\n"); > + > + tsk = current; > + tsk->thread.error_code = error_code; > + tsk->thread.trap_nr = X86_TRAP_CP; > + > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && > + __ratelimit(&rs)) { I can't find it written down anywhere why the ratelimiting is needed at all?
On 2/5/2021 5:59 AM, Borislav Petkov wrote: > On Wed, Feb 03, 2021 at 02:55:28PM -0800, Yu-cheng Yu wrote: >> +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) >> +{ >> + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, >> + DEFAULT_RATELIMIT_BURST); >> + struct task_struct *tsk; >> + >> + if (!user_mode(regs)) { >> + pr_emerg("PANIC: unexpected kernel control protection fault\n"); >> + die("kernel control protection fault", regs, error_code); >> + panic("Machine halted."); >> + } >> + >> + cond_local_irq_enable(regs); >> + >> + if (!boot_cpu_has(X86_FEATURE_CET)) >> + WARN_ONCE(1, "Control protection fault with CET support disabled\n"); >> + >> + tsk = current; >> + tsk->thread.error_code = error_code; >> + tsk->thread.trap_nr = X86_TRAP_CP; >> + >> + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && >> + __ratelimit(&rs)) { > > I can't find it written down anywhere why the ratelimiting is needed at > all? > The ratelimit here is only for #CP, and its rate is not counted together with other types of faults. If a task gets here, it will exit. The only condition the ratelimit will trigger is when multiple tasks hit #CP at once, which is unlikely. Are you suggesting that we do not need the ratelimit here? Thanks! -- Yu-cheng
On Fri, Feb 05, 2021 at 10:00:21AM -0800, Yu, Yu-cheng wrote: > On 2/5/2021 5:59 AM, Borislav Petkov wrote: > > On Wed, Feb 03, 2021 at 02:55:28PM -0800, Yu-cheng Yu wrote: > > > +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) > > > +{ > > > + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, > > > + DEFAULT_RATELIMIT_BURST); > > > + struct task_struct *tsk; > > > + > > > + if (!user_mode(regs)) { > > > + pr_emerg("PANIC: unexpected kernel control protection fault\n"); > > > + die("kernel control protection fault", regs, error_code); > > > + panic("Machine halted."); > > > + } > > > + > > > + cond_local_irq_enable(regs); > > > + > > > + if (!boot_cpu_has(X86_FEATURE_CET)) > > > + WARN_ONCE(1, "Control protection fault with CET support disabled\n"); > > > + > > > + tsk = current; > > > + tsk->thread.error_code = error_code; > > > + tsk->thread.trap_nr = X86_TRAP_CP; > > > + > > > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && > > > + __ratelimit(&rs)) { > > > > I can't find it written down anywhere why the ratelimiting is needed at > > all? > > > > The ratelimit here is only for #CP, and its rate is not counted together > with other types of faults. If a task gets here, it will exit. The only > condition the ratelimit will trigger is when multiple tasks hit #CP at once, > which is unlikely. Are you suggesting that we do not need the ratelimit > here? Since this is a potentially unprivileged-userspace-triggerable condition, I tend to prefer having a ratelimit. I don't feel _strongly_ about it, but I find it better to be defensive against log spamming (whether malicious or accidental).
On Fri, Feb 05, 2021 at 10:00:21AM -0800, Yu, Yu-cheng wrote: > The ratelimit here is only for #CP, and its rate is not counted together > with other types of faults. If a task gets here, it will exit. The only > condition the ratelimit will trigger is when multiple tasks hit #CP at once, > which is unlikely. Are you suggesting that we do not need the ratelimit > here? I'm trying to first find out why is it there. Is this something you've hit during testing and thought, oh well, this needs a ratelimit or was it added just because?
On 2/8/2021 10:20 AM, Borislav Petkov wrote: > On Fri, Feb 05, 2021 at 10:00:21AM -0800, Yu, Yu-cheng wrote: >> The ratelimit here is only for #CP, and its rate is not counted together >> with other types of faults. If a task gets here, it will exit. The only >> condition the ratelimit will trigger is when multiple tasks hit #CP at once, >> which is unlikely. Are you suggesting that we do not need the ratelimit >> here? > > I'm trying to first find out why is it there. > > Is this something you've hit during testing and thought, oh well, this > needs a ratelimit or was it added just because? > I have not run into the situation. Initially it was there because other faults have it. When you asked, I went through it and put out my reasoning. I think it still makes sense to keep it. -- Yu-cheng
On Mon, Feb 08, 2021 at 10:50:07AM -0800, Yu, Yu-cheng wrote: > I have not run into the situation. Initially it was there because other > faults have it. Which other faults? > When you asked, I went through it and put out my reasoning. What does that mean? > I think it still makes sense to keep it. Because you have a hunch or you actually have an objective reason why?
On 2/8/2021 10:53 AM, Borislav Petkov wrote: > On Mon, Feb 08, 2021 at 10:50:07AM -0800, Yu, Yu-cheng wrote: >> I have not run into the situation. Initially it was there because other >> faults have it. > > Which other faults? exc_general_protection() and do_trap() both call show_signal(), which then calls printk_ratelimit(). > >> When you asked, I went through it and put out my reasoning. > > What does that mean? > I went through my patch and check if ratelimit is necessary, and then describe the finding. >> I think it still makes sense to keep it. > > Because you have a hunch or you actually have an objective reason why? > For example, if a shell script, in a loop re-starts an app when it exits, and the app is causing control-protection fault. The log messages should be rate limited.
On Mon, Feb 08, 2021 at 11:23:18AM -0800, Yu, Yu-cheng wrote: > exc_general_protection() and do_trap() both call show_signal(), which > then calls printk_ratelimit(). You could've done some git archeology and could've found abd4f7505baf ("x86: i386-show-unhandled-signals-v3") which explains why that ratelimiting is needed. > For example, if a shell script, in a loop re-starts an app when it > exits, and the app is causing control-protection fault. The log > messages should be rate limited. I think you should be able to get where I'm going with this, by now: please put a comment over the ratelimiting to explain why it is there, just like the above commit explains. Thx.
On 2/8/2021 11:48 AM, Borislav Petkov wrote: > On Mon, Feb 08, 2021 at 11:23:18AM -0800, Yu, Yu-cheng wrote: >> exc_general_protection() and do_trap() both call show_signal(), which >> then calls printk_ratelimit(). > > You could've done some git archeology and could've found > > abd4f7505baf ("x86: i386-show-unhandled-signals-v3") > > which explains why that ratelimiting is needed. > >> For example, if a shell script, in a loop re-starts an app when it >> exits, and the app is causing control-protection fault. The log >> messages should be rate limited. > > I think you should be able to get where I'm going with this, by now: > please put a comment over the ratelimiting to explain why it is there, > just like the above commit explains. I will add that. > > Thx. >
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h index f656aabd1545..ff4b3bf634da 100644 --- a/arch/x86/include/asm/idtentry.h +++ b/arch/x86/include/asm/idtentry.h @@ -574,6 +574,10 @@ DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_SS, exc_stack_segment); DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_GP, exc_general_protection); DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_AC, exc_alignment_check); +#ifdef CONFIG_X86_CET +DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_CP, exc_control_protection); +#endif + /* Raw exception entries which need extra work */ DECLARE_IDTENTRY_RAW(X86_TRAP_UD, exc_invalid_op); DECLARE_IDTENTRY_RAW(X86_TRAP_BP, exc_int3); diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c index ee1a283f8e96..e8166d9bbb10 100644 --- a/arch/x86/kernel/idt.c +++ b/arch/x86/kernel/idt.c @@ -105,6 +105,10 @@ static const __initconst struct idt_data def_idts[] = { #elif defined(CONFIG_X86_32) SYSG(IA32_SYSCALL_VECTOR, entry_INT80_32), #endif + +#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 a5330ff498f0..dd92490b1e7f 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 != 5); BUILD_BUG_ON(NSIGCHLD != 6); diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 7f5aec758f0e..f5354c35df32 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -606,6 +606,66 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection) cond_local_irq_disable(regs); } +#ifdef CONFIG_X86_CET +static const char * const control_protection_err[] = { + "unknown", + "near-ret", + "far-ret/iret", + "endbranch", + "rstorssp", + "setssbsy", +}; + +/* + * When a control protection exception occurs, send a signal to the responsible + * application. Currently, control protection is only enabled for user mode. + * This exception should not come from kernel mode. + */ +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection) +{ + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, + DEFAULT_RATELIMIT_BURST); + struct task_struct *tsk; + + if (!user_mode(regs)) { + pr_emerg("PANIC: unexpected kernel control protection fault\n"); + die("kernel control protection fault", regs, error_code); + panic("Machine halted."); + } + + cond_local_irq_enable(regs); + + if (!boot_cpu_has(X86_FEATURE_CET)) + WARN_ONCE(1, "Control protection fault with CET support disabled\n"); + + tsk = current; + tsk->thread.error_code = error_code; + tsk->thread.trap_nr = X86_TRAP_CP; + + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && + __ratelimit(&rs)) { + unsigned int max_err; + unsigned long ssp; + + max_err = ARRAY_SIZE(control_protection_err) - 1; + if (error_code < 0 || error_code > max_err) + error_code = 0; + + rdmsrl(MSR_IA32_PL3_SSP, ssp); + pr_emerg("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)", + tsk->comm, task_pid_nr(tsk), + regs->ip, regs->sp, ssp, error_code, + control_protection_err[error_code]); + print_vma_addr(KERN_CONT " in ", regs->ip); + pr_cont("\n"); + } + + force_sig_fault(SIGSEGV, SEGV_CPERR, + (void __user *)uprobe_get_trap_addr(regs)); + cond_local_irq_disable(regs); +} +#endif + static bool do_int3(struct pt_regs *regs) { int res; diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h index d2597000407a..1c2ea91284a0 100644 --- a/include/uapi/asm-generic/siginfo.h +++ b/include/uapi/asm-generic/siginfo.h @@ -231,7 +231,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
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; or an indirect JMP instruction, without the NOTRACK prefix, arrives at a non-ENDBR opcode. 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. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Cc: Michael Kerrisk <mtk.manpages@gmail.com> --- arch/x86/include/asm/idtentry.h | 4 ++ arch/x86/kernel/idt.c | 4 ++ arch/x86/kernel/signal_compat.c | 2 +- arch/x86/kernel/traps.c | 60 ++++++++++++++++++++++++++++++ include/uapi/asm-generic/siginfo.h | 3 +- 5 files changed, 71 insertions(+), 2 deletions(-)