Re: [PATCH v4 30/39] x86/shstk: Introduce map_shadow_stack syscall

From: Edgecombe, Rick P
Date: Mon Dec 05 2022 - 17:19:29 EST

Next message: Hans de Goede: "Re: [PATCH v3 0/9] fix reset line polarity for Goodix touchscreen controllers"
Previous message: Rob Herring: "Re: [PATCH 3/3] dt-bindings: leds: irled: ir-spi-led: convert to DT schema"
In reply to: Kees Cook: "Re: [PATCH v4 30/39] x86/shstk: Introduce map_shadow_stack syscall"
Next in thread: Rick Edgecombe: "[PATCH v4 31/39] x86/shstk: Support wrss for userspace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 2022-12-02 at 18:51 -0800, Kees Cook wrote:
> On Fri, Dec 02, 2022 at 04:35:57PM -0800, Rick Edgecombe wrote:
> > When operating with shadow stacks enabled, the kernel will
> > automatically
> > allocate shadow stacks for new threads, however in some cases
> > userspace
> > will need additional shadow stacks. The main example of this is the
> > ucontext family of functions, which require userspace allocating
> > and
> > pivoting to userspace managed stacks.
> >
> > Unlike most other user memory permissions, shadow stacks need to be
> > provisioned with special data in order to be useful. They need to
> > be setup
> > with a restore token so that userspace can pivot to them via the
> > RSTORSSP
> > instruction. But, the security design of shadow stack's is that
> > they
> > should not be written to except in limited circumstances. This
> > presents a
> > problem for userspace, as to how userspace can provision this
> > special
> > data, without allowing for the shadow stack to be generally
> > writable.
> >
> > Previously, a new PROT_SHADOW_STACK was attempted, which could be
> > mprotect()ed from RW permissions after the data was provisioned.
> > This was
> > found to not be secure enough, as other thread's could write to the
> > shadow stack during the writable window.
> >
> > The kernel can use a special instruction, WRUSS, to write directly
> > to
> > userspace shadow stacks. So the solution can be that memory can be
> > mapped
> > as shadow stack permissions from the beginning (never generally
> > writable
> > in userspace), and the kernel itself can write the restore token.
> >
> > First, a new madvise() flag was explored, which could operate on
> > the
> > PROT_SHADOW_STACK memory. This had a couple downsides:
> > 1. Extra checks were needed in mprotect() to prevent writable
> > memory from
> > ever becoming PROT_SHADOW_STACK.
> > 2. Extra checks/vma state were needed in the new madvise() to
> > prevent
> > restore tokens being written into the middle of pre-used shadow
> > stacks.
> > It is ideal to prevent restore tokens being added at arbitrary
> > locations, so the check was to make sure the shadow stack had
> > never been
> > written to.
> > 3. It stood out from the rest of the madvise flags, as more of
> > direct
> > action than a hint at future desired behavior.
> >
> > So rather than repurpose two existing syscalls (mmap, madvise) that
> > don't
> > quite fit, just implement a new map_shadow_stack syscall to allow
> > userspace to map and setup new shadow stacks in one step. While
> > ucontext
> > is the primary motivator, userspace may have other unforeseen
> > reasons to
> > setup it's own shadow stacks using the WRSS instruction. Towards
> > this
> > provide a flag so that stacks can be optionally setup securely for
> > the
> > common case of ucontext without enabling WRSS. Or potentially have
> > the
> > kernel set up the shadow stack in some new way.
> >
> > The following example demonstrates how to create a new shadow stack
> > with
> > map_shadow_stack:
> > void *shstk = map_shadow_stack(addr, stack_size,
> > SHADOW_STACK_SET_TOKEN);
> >
> > Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> > Tested-by: John Allen <john.allen@xxxxxxx>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > ---
> >
> > v3:
> > - Change syscall common -> 64 (Kees)
> > - Use bit shift notation instead of 0x1 for uapi header (Kees)
> > - Call do_mmap() with MAP_FIXED_NOREPLACE (Kees)
> > - Block unsupported flags (Kees)
> > - Require size >= 8 to set token (Kees)
> >
> > v2:
> > - Change syscall to take address like mmap() for CRIU's usage
> >
> > v1:
> > - New patch (replaces PROT_SHADOW_STACK).
> >
> > arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> > arch/x86/include/uapi/asm/mman.h | 3 ++
> > arch/x86/kernel/shstk.c | 56
> > ++++++++++++++++++++++----
> > include/linux/syscalls.h | 1 +
> > include/uapi/asm-generic/unistd.h | 2 +-
> > kernel/sys_ni.c | 1 +
> > 6 files changed, 55 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
> > b/arch/x86/entry/syscalls/syscall_64.tbl
> > index c84d12608cd2..f65c671ce3b1 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -372,6 +372,7 @@
> > 448 common process_mrelease sys_process_mreleas
> > e
> > 449 common futex_waitv sys_futex_waitv
> > 450 common set_mempolicy_home_node sys_set_mempolicy_h
> > ome_node
> > +451 64 map_shadow_stack sys_map_shadow_stack
> >
> > #
> > # Due to a historical design error, certain syscalls are numbered
> > differently
> > diff --git a/arch/x86/include/uapi/asm/mman.h
> > b/arch/x86/include/uapi/asm/mman.h
> > index 775dbd3aff73..15c5a1c4fc29 100644
> > --- a/arch/x86/include/uapi/asm/mman.h
> > +++ b/arch/x86/include/uapi/asm/mman.h
> > @@ -12,6 +12,9 @@
> > ((key) & 0x8 ? VM_PKEY_BIT3 : 0))
> > #endif
> >
> > +/* Flags for map_shadow_stack(2) */
> > +#define SHADOW_STACK_SET_TOKEN (1ULL << 0) /* Set up a restore
> > token in the shadow stack */
> > +
> > #include <asm-generic/mman.h>
> >
> > #endif /* _ASM_X86_MMAN_H */
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > index e53225a8d39e..8f329c22728a 100644
> > --- a/arch/x86/kernel/shstk.c
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -17,6 +17,7 @@
> > #include <linux/compat.h>
> > #include <linux/sizes.h>
> > #include <linux/user.h>
> > +#include <linux/syscalls.h>
> > #include <asm/msr.h>
> > #include <asm/fpu/xstate.h>
> > #include <asm/fpu/types.h>
> > @@ -71,19 +72,31 @@ static int create_rstor_token(unsigned long
> > ssp, unsigned long *token_addr)
> > return 0;
> > }
> >
> > -static unsigned long alloc_shstk(unsigned long size)
> > +static unsigned long alloc_shstk(unsigned long addr, unsigned long
> > size,
> > + unsigned long token_offset, bool
> > set_res_tok)
> > {
> > int flags = MAP_ANONYMOUS | MAP_PRIVATE;
> > struct mm_struct *mm = current->mm;
> > - unsigned long addr, unused;
> > + unsigned long mapped_addr, unused;
> >
> > - mmap_write_lock(mm);
> > - addr = do_mmap(NULL, 0, size, PROT_READ, flags,
> > - VM_SHADOW_STACK | VM_WRITE, 0, &unused, NULL);
> > + if (addr)
> > + flags |= MAP_FIXED_NOREPLACE;
> >
> > + mmap_write_lock(mm);
> > + mapped_addr = do_mmap(NULL, addr, size, PROT_READ, flags,
> > + VM_SHADOW_STACK | VM_WRITE, 0, &unused,
> > NULL);
> > mmap_write_unlock(mm);
> >
> > - return addr;
> > + if (!set_res_tok || IS_ERR_VALUE(addr))
>
> Should this be IS_ERR_VALUE(mapped_addr) (i.e. the result of the
> do_mmap)?

Oops, yes. Thanks for pointing that.

>
> > + goto out;
> > +
> > + if (create_rstor_token(mapped_addr + token_offset, NULL)) {
> > + vm_munmap(mapped_addr, size);
> > + return -EINVAL;
> > + }
> > +
> > +out:
> > + return mapped_addr;
> > }
> >
> > static unsigned long adjust_shstk_size(unsigned long size)
> > @@ -134,7 +147,7 @@ static int shstk_setup(void)
> > return -EOPNOTSUPP;
> >
> > size = adjust_shstk_size(0);
> > - addr = alloc_shstk(size);
> > + addr = alloc_shstk(0, size, 0, false);
> > if (IS_ERR_VALUE(addr))
> > return PTR_ERR((void *)addr);
> >
> > @@ -179,7 +192,7 @@ int shstk_alloc_thread_stack(struct task_struct
> > *tsk, unsigned long clone_flags,
> >
> >
> > size = adjust_shstk_size(stack_size);
> > - addr = alloc_shstk(size);
> > + addr = alloc_shstk(0, size, 0, false);
> > if (IS_ERR_VALUE(addr))
> > return PTR_ERR((void *)addr);
> >
> > @@ -373,6 +386,33 @@ static int shstk_disable(void)
> > return 0;
> > }
> >
> > +SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned
> > long, size, unsigned int, flags)
> > +{
> > + bool set_tok = flags & SHADOW_STACK_SET_TOKEN;
> > + unsigned long aligned_size;
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
> > + return -ENOSYS;
>
> Using -ENOSYS means there's no way to tell the difference between
> "kernel doesn't support it" and "CPU doesn't support it". Should
> this,
> perhaps return -ENOTSUP?

Hmm, sure.

>
> > +
> > + if (flags & ~SHADOW_STACK_SET_TOKEN)
> > + return -EINVAL;
> > +
> > + /* If there isn't space for a token */
> > + if (set_tok && size < 8)
> > + return -EINVAL;
> > +
> > + /*
> > + * An overflow would result in attempting to write the restore
> > token
> > + * to the wrong location. Not catastrophic, but just return the
> > right
> > + * error code and block it.
> > + */
> > + aligned_size = PAGE_ALIGN(size);
> > + if (aligned_size < size)
> > + return -EOVERFLOW;
> > +
> > + return alloc_shstk(addr, aligned_size, size, set_tok);
> > +}
> > +
> > long shstk_prctl(struct task_struct *task, int option, unsigned
> > long features)
> > {
> > if (option == ARCH_SHSTK_LOCK) {
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 33a0ee3bcb2e..392dc11e3556 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -1058,6 +1058,7 @@ asmlinkage long sys_memfd_secret(unsigned int
> > flags);
> > asmlinkage long sys_set_mempolicy_home_node(unsigned long start,
> > unsigned long len,
> > unsigned long home_node,
> > unsigned long flags);
> > +asmlinkage long sys_map_shadow_stack(unsigned long addr, unsigned
> > long size, unsigned int flags);
> >
> > /*
> > * Architecture-specific system calls
> > diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-
> > generic/unistd.h
> > index 45fa180cc56a..b12940ec5926 100644
> > --- a/include/uapi/asm-generic/unistd.h
> > +++ b/include/uapi/asm-generic/unistd.h
> > @@ -887,7 +887,7 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv)
> > __SYSCALL(__NR_set_mempolicy_home_node,
> > sys_set_mempolicy_home_node)
> >
> > #undef __NR_syscalls
> > -#define __NR_syscalls 451
> > +#define __NR_syscalls 452
> >
> > /*
> > * 32 bit systems traditionally used different
> > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> > index 860b2dcf3ac4..cb9aebd34646 100644
> > --- a/kernel/sys_ni.c
> > +++ b/kernel/sys_ni.c
> > @@ -381,6 +381,7 @@ COND_SYSCALL(vm86old);
> > COND_SYSCALL(modify_ldt);
> > COND_SYSCALL(vm86);
> > COND_SYSCALL(kexec_file_load);
> > +COND_SYSCALL(map_shadow_stack);
> >
> > /* s390 */
> > COND_SYSCALL(s390_pci_mmio_read);
> > --
> > 2.17.1
> >
>
> Otherwise, looks good!
>

Thanks for this and the reviewed-bys on other patches!

Next message: Hans de Goede: "Re: [PATCH v3 0/9] fix reset line polarity for Goodix touchscreen controllers"
Previous message: Rob Herring: "Re: [PATCH 3/3] dt-bindings: leds: irled: ir-spi-led: convert to DT schema"
In reply to: Kees Cook: "Re: [PATCH v4 30/39] x86/shstk: Introduce map_shadow_stack syscall"
Next in thread: Rick Edgecombe: "[PATCH v4 31/39] x86/shstk: Support wrss for userspace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]