| Message ID | 20210217222730.15819-7-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Shadow Stack | expand |
On Wed, Feb 17, 2021 at 02:27:10PM -0800, Yu-cheng Yu wrote: > +/* > + * 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); Pls move that out of the function - those "static" qualifiers get missed easily when inside a function. > + 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; > + > + /* > + * Ratelimit to prevent log spamming. > + */ > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && > + __ratelimit(&rs)) { > + unsigned long ssp; > + int err; > + > + err = array_index_nospec(error_code, ARRAY_SIZE(control_protection_err)); "err" as an automatic variable is confusing - we use those to denote whether the function returned an error or not. Call yours "cpf_type" or so. > + > + 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[err]); > + print_vma_addr(KERN_CONT " in ", regs->ip); > + pr_cont("\n"); > + } > + > + force_sig_fault(SIGSEGV, SEGV_CPERR, > + (void __user *)uprobe_get_trap_addr(regs)); Why is this calling an uprobes function? Also, do not break that line even if it is longer than 80. > + 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 I still don't see the patch adding this to the manpage of sigaction(2). There's a git repo there: https://www.kernel.org/doc/man-pages/ and I'm pretty sure Michael takes patches.
On 2/24/2021 8:13 AM, Borislav Petkov wrote: > On Wed, Feb 17, 2021 at 02:27:10PM -0800, Yu-cheng Yu wrote: >> +/* >> + * 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); [...] >> + >> + 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[err]); >> + print_vma_addr(KERN_CONT " in ", regs->ip); >> + pr_cont("\n"); >> + } >> + >> + force_sig_fault(SIGSEGV, SEGV_CPERR, >> + (void __user *)uprobe_get_trap_addr(regs)); > > Why is this calling an uprobes function? > I will change it to error_get_trap_addr(). > Also, do not break that line even if it is longer than 80. > >> + 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 > > I still don't see the patch adding this to the manpage of sigaction(2). > > There's a git repo there: https://www.kernel.org/doc/man-pages/ > > and I'm pretty sure Michael takes patches. > I will send a patch. -- Yu-cheng
On Wed, Feb 24, 2021 at 08:44:45AM -0800, Yu, Yu-cheng wrote: > > > + force_sig_fault(SIGSEGV, SEGV_CPERR, > > > + (void __user *)uprobe_get_trap_addr(regs)); > > > > Why is this calling an uprobes function? > > > > I will change it to error_get_trap_addr(). "/* * Posix requires to provide the address of the faulting instruction for * SIGILL (#UD) and SIGFPE (#DE) in the si_addr member of siginfo_t. ..." Is yours SIGILL or SIGFPE?
On 2/24/2021 8:53 AM, Borislav Petkov wrote: > On Wed, Feb 24, 2021 at 08:44:45AM -0800, Yu, Yu-cheng wrote: >>>> + force_sig_fault(SIGSEGV, SEGV_CPERR, >>>> + (void __user *)uprobe_get_trap_addr(regs)); >>> >>> Why is this calling an uprobes function? >>> >> >> I will change it to error_get_trap_addr(). > > "/* > * Posix requires to provide the address of the faulting instruction for > * SIGILL (#UD) and SIGFPE (#DE) in the si_addr member of siginfo_t. > ..." > > Is yours SIGILL or SIGFPE? > No. Maybe I am doing too much. The GP fault sets si_addr to zero, for example. So maybe do the same here?
On Wed, Feb 24, 2021 at 09:56:13AM -0800, Yu, Yu-cheng wrote: > No. Maybe I am doing too much. The GP fault sets si_addr to zero, for > example. So maybe do the same here? No, you're looking at this from the wrong angle. This is going to be user-visible and the moment it gets upstream, it is cast in stone. So the whole use case of what luserspace needs to do or is going to do or wants to do on a SEGV_CPERR, needs to be described, agreed upon by people etc before it goes out. And thus clarified whether the address gets copied out or not. Thx.
On Wed, Feb 24, 2021 at 11:20 AM Borislav Petkov <bp@alien8.de> wrote: > > On Wed, Feb 24, 2021 at 09:56:13AM -0800, Yu, Yu-cheng wrote: > > No. Maybe I am doing too much. The GP fault sets si_addr to zero, for > > example. So maybe do the same here? > > No, you're looking at this from the wrong angle. This is going to be > user-visible and the moment it gets upstream, it is cast in stone. > > So the whole use case of what luserspace needs to do or is going to do > or wants to do on a SEGV_CPERR, needs to be described, agreed upon by > people etc before it goes out. And thus clarified whether the address > gets copied out or not. I vote 0. The address is in ucontext->gregs[REG_RIP] [0] regardless. Why do we need to stick a copy somewhere else? [0] or however it's spelled. i can never remember. > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Feb 24, 2021 at 11:30:34AM -0800, Andy Lutomirski wrote: > On Wed, Feb 24, 2021 at 11:20 AM Borislav Petkov <bp@alien8.de> wrote: > > > > On Wed, Feb 24, 2021 at 09:56:13AM -0800, Yu, Yu-cheng wrote: > > > No. Maybe I am doing too much. The GP fault sets si_addr to zero, for > > > example. So maybe do the same here? > > > > No, you're looking at this from the wrong angle. This is going to be > > user-visible and the moment it gets upstream, it is cast in stone. > > > > So the whole use case of what luserspace needs to do or is going to do > > or wants to do on a SEGV_CPERR, needs to be described, agreed upon by > > people etc before it goes out. And thus clarified whether the address > > gets copied out or not. > > I vote 0. The address is in ucontext->gregs[REG_RIP] [0] regardless. > Why do we need to stick a copy somewhere else? > > [0] or however it's spelled. i can never remember. Fine with me. Let's have this documented in the manpage and then we can move forward with this. Thx.
On 2/24/2021 11:42 AM, Borislav Petkov wrote: > On Wed, Feb 24, 2021 at 11:30:34AM -0800, Andy Lutomirski wrote: >> On Wed, Feb 24, 2021 at 11:20 AM Borislav Petkov <bp@alien8.de> wrote: >>> >>> On Wed, Feb 24, 2021 at 09:56:13AM -0800, Yu, Yu-cheng wrote: >>>> No. Maybe I am doing too much. The GP fault sets si_addr to zero, for >>>> example. So maybe do the same here? >>> >>> No, you're looking at this from the wrong angle. This is going to be >>> user-visible and the moment it gets upstream, it is cast in stone. >>> >>> So the whole use case of what luserspace needs to do or is going to do >>> or wants to do on a SEGV_CPERR, needs to be described, agreed upon by >>> people etc before it goes out. And thus clarified whether the address >>> gets copied out or not. >> >> I vote 0. The address is in ucontext->gregs[REG_RIP] [0] regardless. >> Why do we need to stick a copy somewhere else? >> >> [0] or however it's spelled. i can never remember. > > Fine with me. Let's have this documented in the manpage and then we can > move forward with this. > > Thx. > The man page at https://man7.org/linux/man-pages/man2/sigaction.2.html says, SIGILL, SIGFPE, SIGSEGV, SIGBUS, and SIGTRAP fill in si_addr with the address of the fault. But it is not entirely true. I will send a patch to update it, and another patch for the si_code. -- Yu-cheng
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..8c7fa91a57c9 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -39,6 +39,7 @@ #include <linux/io.h> #include <linux/hardirq.h> #include <linux/atomic.h> +#include <linux/nospec.h> #include <asm/stacktrace.h> #include <asm/processor.h> @@ -606,6 +607,68 @@ 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", + "unknown", +}; + +/* + * 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; + + /* + * Ratelimit to prevent log spamming. + */ + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && + __ratelimit(&rs)) { + unsigned long ssp; + int err; + + err = array_index_nospec(error_code, ARRAY_SIZE(control_protection_err)); + + 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[err]); + 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