Re: [PATCH v4 37/39] x86: Add PTRACE interface for shadow stack

Mike Rapoport
2022-12-09T17:07:07Z

Hi Rick,

On Fri, Dec 02, 2022 at 04:36:04PM -0800, Rick Edgecombe wrote:
> From: Yu-cheng Yu <yu-cheng.yu@intel.com>
>
> Some applications (like GDB) would like to tweak shadow stack state via
> ptrace. This allows for existing functionality to continue to work for
> seized shadow stack applications. Provide an regset interface for
> manipulating the shadow stack pointer (SSP).
>
> There is already ptrace functionality for accessing xstate, but this
> does not include supervisor xfeatures. So there is not a completely
> clear place for where to put the shadow stack state. Adding it to the
> user xfeatures regset would complicate that code, as it currently shares
> logic with signals which should not have supervisor features.
>
> Don't add a general supervisor xfeature regset like the user one,
> because it is better to maintain flexibility for other supervisor
> xfeatures to define their own interface. For example, an xfeature may
> decide not to expose all of it's state to userspace, as is actually the
> case for shadow stack ptrace functionality. A lot of enum values remain
> to be used, so just put it in dedicated shadow stack regset.
>
> 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
> 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
> 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
> that userspace already has indirect control over.
>
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> 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>
> ---
>
> v4:
> - Make shadow stack only. Reduce to only supporting SSP register, and
> remove CET references (peterz)
> - Add comment to not use 0x203, becuase binutils already looks for it in
> coredumps. (Christina Schimpe)
>
> v3:
> - Drop dependence on thread.shstk.size, and use thread.features bits
> - Drop 32 bit support
>
> v2:
> - Check alignment on ssp.
> - Block IBT bits.
> - Handle init states instead of returning error.
> - Add verbose commit log justifying the design.
>
> Yu-Cheng v12:
> - Return -ENODEV when CET registers are in INIT state.
> - Check reserved/non-support bits from user input.
>
> arch/x86/include/asm/fpu/regset.h | 7 +--
> arch/x86/kernel/fpu/regset.c | 87 +++++++++++++++++++++++++++++++
> arch/x86/kernel/ptrace.c | 12 +++++
> include/uapi/linux/elf.h | 2 +
> 4 files changed, 105 insertions(+), 3 deletions(-)
>
> 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..00f3d5c9b682 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,92 @@ 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 (shstk_enabled())

This is not going to work with ptrace as shstk_enabled() checks current
rather than target.

> + 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 definetely
> + * 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 c7b056af9ef0..e9283f0641c4 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 */
> --
> 2.17.1
>

--
Sincerely yours,
Mike.