| Message ID | 20230227222957.24501-40-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Mon, Feb 27, 2023 at 02:29:55PM -0800, Rick Edgecombe wrote: > The only downside to not having a generic supervisor xfeature regset, > is that apps need to be enlightened of any new supervisor xfeature > exposed this way (i.e. they can't try to have generic save/restore > logic). But maybe that is a good thing, because they have to think > through each new xfeature instead of encountering issues when new a new Remove the first "new". > supervisor xfeature was added. > > By adding a shadow stack regset, it also has the effect of including the > shadow stack state in a core dump, which could be useful for debugging. > > The shadow stack specific xstate includes the SSP, and the shadow stack > and WRSS enablement status. Enabling shadow stack or wrss in the kernel ^^^^ "WRSS" > involves more than just flipping the bit. The kernel is made aware that > it has to do extra things when cloning or handling signals. That logic > is triggered off of separate feature enablement state kept in the task > struct. So the flipping on HW shadow stack enforcement without notifying > the kernel to change its behavior would severely limit what an application > could do without crashing, and the results would depend on kernel > internal implementation details. There is also no known use for controlling > this state via prtace today. So only expose the SSP, which is something Unknown word [prtace] in commit message. Suggestions: ['ptrace' > that userspace already has indirect control over. > > Tested-by: Pengfei Xu <pengfei.xu@intel.com> > Tested-by: John Allen <john.allen@amd.com> > Tested-by: Kees Cook <keescook@chromium.org> > Acked-by: Mike Rapoport (IBM) <rppt@kernel.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> I think your SOB should come last: ... 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> Pls check whole set. > +#ifdef CONFIG_X86_USER_SHADOW_STACK > +int ssp_active(struct task_struct *target, const struct user_regset *regset) > +{ > + if (target->thread.features & ARCH_SHSTK_SHSTK) > + return regset->n; > + > + return 0; > +} > + > +int ssp_get(struct task_struct *target, const struct user_regset *regset, > + struct membuf to) > +{ > + struct fpu *fpu = &target->thread.fpu; > + struct cet_user_state *cetregs; > + > + if (!boot_cpu_has(X86_FEATURE_USER_SHSTK)) check_for_deprecated_apis: WARNING: arch/x86/kernel/fpu/regset.c:193: Do not use boot_cpu_has() - use cpu_feature_enabled() instead. Check your whole set pls.
On Sat, 2023-03-11 at 16:06 +0100, Borislav Petkov wrote: > On Mon, Feb 27, 2023 at 02:29:55PM -0800, Rick Edgecombe wrote: [...] Tested-by: Pengfei Xu <pengfei.xu@intel.com> > > Tested-by: John Allen <john.allen@amd.com> > > Tested-by: Kees Cook <keescook@chromium.org> > > Acked-by: Mike Rapoport (IBM) <rppt@kernel.org> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > I think your SOB should come last: Right on commit log typos, and yea this is screwed up. I think Dave re- ordered the SOB's already. > > ... > 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> > > Pls check whole set. > > > > +#ifdef CONFIG_X86_USER_SHADOW_STACK > > +int ssp_active(struct task_struct *target, const struct > > user_regset *regset) > > +{ > > + if (target->thread.features & ARCH_SHSTK_SHSTK) > > + return regset->n; > > + > > + return 0; > > +} > > + > > +int ssp_get(struct task_struct *target, const struct user_regset > > *regset, > > + struct membuf to) > > +{ > > + struct fpu *fpu = &target->thread.fpu; > > + struct cet_user_state *cetregs; > > + > > + if (!boot_cpu_has(X86_FEATURE_USER_SHSTK)) > > check_for_deprecated_apis: WARNING: arch/x86/kernel/fpu/regset.c:193: > Do not use boot_cpu_has() - use cpu_feature_enabled() instead. > > Check your whole set pls. Ok. I think the other case is in "x86/fpu: Add helper for initing features" where it was moved code.
diff --git a/arch/x86/include/asm/fpu/regset.h b/arch/x86/include/asm/fpu/regset.h index 4f928d6a367b..697b77e96025 100644 --- a/arch/x86/include/asm/fpu/regset.h +++ b/arch/x86/include/asm/fpu/regset.h @@ -7,11 +7,12 @@ #include <linux/regset.h> -extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active; +extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active, + ssp_active; extern user_regset_get2_fn fpregs_get, xfpregs_get, fpregs_soft_get, - xstateregs_get; + xstateregs_get, ssp_get; extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set, - xstateregs_set; + xstateregs_set, ssp_set; /* * xstateregs_active == regset_fpregs_active. Please refer to the comment diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c index 6d056b68f4ed..c806952d9496 100644 --- a/arch/x86/kernel/fpu/regset.c +++ b/arch/x86/kernel/fpu/regset.c @@ -8,6 +8,7 @@ #include <asm/fpu/api.h> #include <asm/fpu/signal.h> #include <asm/fpu/regset.h> +#include <asm/prctl.h> #include "context.h" #include "internal.h" @@ -174,6 +175,91 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset, return ret; } +#ifdef CONFIG_X86_USER_SHADOW_STACK +int ssp_active(struct task_struct *target, const struct user_regset *regset) +{ + if (target->thread.features & ARCH_SHSTK_SHSTK) + return regset->n; + + return 0; +} + +int ssp_get(struct task_struct *target, const struct user_regset *regset, + struct membuf to) +{ + struct fpu *fpu = &target->thread.fpu; + struct cet_user_state *cetregs; + + if (!boot_cpu_has(X86_FEATURE_USER_SHSTK)) + return -ENODEV; + + sync_fpstate(fpu); + cetregs = get_xsave_addr(&fpu->fpstate->regs.xsave, XFEATURE_CET_USER); + if (!cetregs) { + /* + * The registers are the in the init state. The init values for + * these regs are zero, so just zero the output buffer. + */ + membuf_zero(&to, sizeof(cetregs->user_ssp)); + return 0; + } + + return membuf_write(&to, (unsigned long *)&cetregs->user_ssp, + sizeof(cetregs->user_ssp)); +} + +int ssp_set(struct task_struct *target, const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + struct fpu *fpu = &target->thread.fpu; + struct xregs_state *xsave = &fpu->fpstate->regs.xsave; + struct cet_user_state *cetregs; + unsigned long user_ssp; + int r; + + if (!boot_cpu_has(X86_FEATURE_USER_SHSTK) || + !ssp_active(target, regset)) + return -ENODEV; + + r = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &user_ssp, 0, -1); + if (r) + return r; + + /* + * Some kernel instructions (IRET, etc) can cause exceptions in the case + * of disallowed CET register values. Just prevent invalid values. + */ + if ((user_ssp >= TASK_SIZE_MAX) || !IS_ALIGNED(user_ssp, 8)) + return -EINVAL; + + fpu_force_restore(fpu); + + /* + * Don't want to init the xfeature until the kernel will definitely + * overwrite it, otherwise if it inits and then fails out, it would + * end up initing it to random data. + */ + if (!xfeature_saved(xsave, XFEATURE_CET_USER) && + WARN_ON(init_xfeature(xsave, XFEATURE_CET_USER))) + return -ENODEV; + + cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER); + if (WARN_ON(!cetregs)) { + /* + * This shouldn't ever be NULL because it was successfully + * inited above if needed. The only scenario would be if an + * xfeature was somehow saved in a buffer, but not enabled in + * xsave. + */ + return -ENODEV; + } + + cetregs->user_ssp = user_ssp; + return 0; +} +#endif /* CONFIG_X86_USER_SHADOW_STACK */ + #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION /* diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index dfaa270a7cc9..095f04bdabdc 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -58,6 +58,7 @@ enum x86_regset_64 { REGSET64_FP, REGSET64_IOPERM, REGSET64_XSTATE, + REGSET64_SSP, }; #define REGSET_GENERAL \ @@ -1267,6 +1268,17 @@ static struct user_regset x86_64_regsets[] __ro_after_init = { .active = ioperm_active, .regset_get = ioperm_get }, +#ifdef CONFIG_X86_USER_SHADOW_STACK + [REGSET64_SSP] = { + .core_note_type = NT_X86_SHSTK, + .n = 1, + .size = sizeof(u64), + .align = sizeof(u64), + .active = ssp_active, + .regset_get = ssp_get, + .set = ssp_set + }, +#endif }; static const struct user_regset_view user_x86_64_view = { diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index 68de6f4c4eee..103a1f2da86e 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -406,6 +406,8 @@ typedef struct elf64_shdr { #define NT_386_TLS 0x200 /* i386 TLS slots (struct user_desc) */ #define NT_386_IOPERM 0x201 /* x86 io permission bitmap (1=deny) */ #define NT_X86_XSTATE 0x202 /* x86 extended state using xsave */ +/* Old binutils treats 0x203 as a CET state */ +#define NT_X86_SHSTK 0x204 /* x86 SHSTK state */ #define NT_S390_HIGH_GPRS 0x300 /* s390 upper register halves */ #define NT_S390_TIMER 0x301 /* s390 timer register */ #define NT_S390_TODCMP 0x302 /* s390 TOD clock comparator register */