| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d9ab95ded2c51ead24ebb4167d8ce77ae4a3594f.camel@intel.com>
Date: Tue, 8 Feb 2022 22:23:33 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
"bsingharora@...il.com" <bsingharora@...il.com>,
"hpa@...or.com" <hpa@...or.com>,
"Syromiatnikov, Eugene" <esyr@...hat.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"rdunlap@...radead.org" <rdunlap@...radead.org>,
"keescook@...omium.org" <keescook@...omium.org>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"Eranian, Stephane" <eranian@...gle.com>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"fweimer@...hat.com" <fweimer@...hat.com>,
"nadav.amit@...il.com" <nadav.amit@...il.com>,
"jannh@...gle.com" <jannh@...gle.com>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
"kcc@...gle.com" <kcc@...gle.com>, "bp@...en8.de" <bp@...en8.de>,
"oleg@...hat.com" <oleg@...hat.com>,
"hjl.tools@...il.com" <hjl.tools@...il.com>,
"Yang, Weijiang" <weijiang.yang@...el.com>,
"Lutomirski, Andy" <luto@...nel.org>,
"pavel@....cz" <pavel@....cz>, "arnd@...db.de" <arnd@...db.de>,
"Moreira, Joao" <joao.moreira@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
"x86@...nel.org" <x86@...nel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"Dave.Martin@....com" <Dave.Martin@....com>,
"john.allen@....com" <john.allen@....com>,
"mingo@...hat.com" <mingo@...hat.com>,
"Hansen, Dave" <dave.hansen@...el.com>,
"corbet@....net" <corbet@....net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
"gorcunov@...il.com" <gorcunov@...il.com>
CC: "Yu, Yu-cheng" <yu-cheng.yu@...el.com>,
"mtk.manpages@...il.com" <mtk.manpages@...il.com>
Subject: Re: [PATCH 06/35] x86/cet: Add control-protection fault handler
On Mon, 2022-02-07 at 15:56 -0800, Dave Hansen wrote:
> On 1/30/22 13:18, Rick Edgecombe 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.
>
> It's not a big deal, but we should probably just remove IBT from the
> changelogs for now.
Makes sense. I should have scrubbed these better for IBT.
>
> > arch/arm/kernel/signal.c | 2 +-
> > arch/arm64/kernel/signal.c | 2 +-
> > arch/arm64/kernel/signal32.c | 2 +-
> > arch/sparc/kernel/signal32.c | 2 +-
> > arch/sparc/kernel/signal_64.c | 2 +-
> > 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 | 62
> > ++++++++++++++++++++++++++++++
> > include/uapi/asm-generic/siginfo.h | 3 +-
> > 10 files changed, 78 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> > index c532a6041066..59aaadce9d52 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 d8aaf4b6f432..d2da57c415b8 100644
> > --- a/arch/arm64/kernel/signal.c
> > +++ b/arch/arm64/kernel/signal.c
> > @@ -983,7 +983,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 d984282b979f..8776a34c6444 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 6cc124a3bb98..dc50b2a78692 100644
> > --- a/arch/sparc/kernel/signal32.c
> > +++ b/arch/sparc/kernel/signal32.c
> > @@ -752,7 +752,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 2a78d2af1265..7fe2bd37bd1a 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/idtentry.h
> > b/arch/x86/include/asm/idtentry.h
> > index 1345088e9902..a90791433152 100644
> > --- a/arch/x86/include/asm/idtentry.h
> > +++ b/arch/x86/include/asm/idtentry.h
> > @@ -562,6 +562,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_SHADOW_STACK
> > +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 df0fa695bb09..9f1bdaabc246 100644
> > --- a/arch/x86/kernel/idt.c
> > +++ b/arch/x86/kernel/idt.c
> > @@ -113,6 +113,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_SHADOW_STACK
> > + 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 b52407c56000..ff50cd978ea5 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 c9d566dcf89a..54b7a146fd5e 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>
> > @@ -641,6 +642,67 @@
> > DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> > cond_local_irq_disable(regs);
> > }
> >
> > +#ifdef CONFIG_X86_SHADOW_STACK
> > +static const char * const control_protection_err[] = {
> > + "unknown",
> > + "near-ret",
> > + "far-ret/iret",
> > + "endbranch",
> > + "rstorssp",
> > + "setssbsy",
> > + "unknown",
> > +};
> > +
> > +static DEFINE_RATELIMIT_STATE(cpf_rate,
> > DEFAULT_RATELIMIT_INTERVAL,
> > + DEFAULT_RATELIMIT_BURST);
> > +
> > +/*
> > + * 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.
> > + */
>
> Please move that last sentence to the code which enforces that
> expectation.
Ok.
>
> > +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> > +{
> > + struct task_struct *tsk;
> > +
> > + if (!user_mode(regs)) {
> > + die("kernel control protection fault", regs,
> > error_code);
> > + panic("Unexpected kernel control protection
> > fault. Machine halted.");
> > + }
>
> s/ Machine halted.//
>
> I think they'll get the point when they see "kernel panic".
Ok.
>
> > +
> > + cond_local_irq_enable(regs);
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > + 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(&cpf_rate)) {
> > + unsigned long ssp;
> > + int cpf_type;
> > +
> > + cpf_type = array_index_nospec(error_code,
> > ARRAY_SIZE(control_protection_err));
>
> Isn't 'error_code' generated by the hardware? Is this defending
> against
> userspace which can somehow get trigger this with an arbitrary
> 'error_code'?
>
> I'm also not sure I like using array_index_nospec() as the *only*
> bounds
> checking on the array. Is that the way folks are using it these
> days?
Yea, I was wondering about that too. It looks like it came from this
comment:
https://lore.kernel.org/lkml/202102041201.C2B93F8D8A@keescook/
...which didn't raise any speculation concerns. What it does do though,
is massage the index to 0 if it is out of bounds, leading to the
"unknown" message being selected for out of bounds error codes. If
that's the purpose though, I'm not sure why "unknown" is also the last
element of the array though.
I think maybe this will not typically be a fast path, and so
conditional logic would be easier to read and better on balance.
I'm now realizing that this is missing the "ENCL" error code bit (15)
which denotes #CP during enclave execution.
> Even the comment above it has a pattern like this:
>
> > * if (index < size) {
> > * index = array_index_nospec(index, size);
> > * val = array[index];
> > * }
>
>
> > + 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[cpf_type]);
> > + print_vma_addr(KERN_CONT " in ", regs->ip);
> > + pr_cont("\n");
> > + }
> > +
> > + force_sig_fault(SIGSEGV, SEGV_CPERR, (void __user *)0);
> > + 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 3ba180f550d7..081f4b37d22c 100644
> > --- a/include/uapi/asm-generic/siginfo.h
> > +++ b/include/uapi/asm-generic/siginfo.h
> > @@ -240,7 +240,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
>
>
Powered by blists - more mailing lists