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

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:

quoted

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 <redacted>
Tested-by: John Allen <john.allen@amd.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---

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.

quoted

+		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.

quoted

+
+	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!