Re: [PATCH v24 22/30] x86/cet/shstk: Add user-mode shadow stack support

2021-04-09 Thread Yu, Yu-cheng

On 4/9/2021 8:57 AM, Kirill A. Shutemov wrote:

On Thu, Apr 01, 2021 at 03:10:56PM -0700, Yu-cheng Yu wrote:

Introduce basic shadow stack enabling/disabling/allocation routines.
A task's shadow stack is allocated from memory with VM_SHADOW_STACK flag
and has a fixed size of min(RLIMIT_STACK, 4GB).

Signed-off-by: Yu-cheng Yu 
Cc: Kees Cook 


[...]


diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
new file mode 100644
index ..5406fdf6df3c
--- /dev/null
+++ b/arch/x86/kernel/shstk.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * shstk.c - Intel shadow stack support
+ *
+ * Copyright (c) 2021, Intel Corporation.
+ * Yu-cheng Yu 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static void start_update_msrs(void)
+{
+   fpregs_lock();
+   if (test_thread_flag(TIF_NEED_FPU_LOAD))
+   __fpregs_load_activate();
+}
+
+static void end_update_msrs(void)
+{
+   fpregs_unlock();
+}
+
+static unsigned long alloc_shstk(unsigned long size, int flags)
+{
+   struct mm_struct *mm = current->mm;
+   unsigned long addr, populate;
+
+   /* VM_SHADOW_STACK requires MAP_ANONYMOUS, MAP_PRIVATE */
+   flags |= MAP_ANONYMOUS | MAP_PRIVATE;


Looks like all callers has flags == 0. Do I miss something.


My earlier versions use this flag.  I should have removed it.


+
+   mmap_write_lock(mm);
+   addr = do_mmap(NULL, 0, size, PROT_READ, flags, VM_SHADOW_STACK, 0,
+  , NULL);
+   mmap_write_unlock(mm);
+
+   if (populate)
+   mm_populate(addr, populate);


If all callers pass down flags==0, populate will never happen.


I will fix it.


+
+   return addr;
+}
+
+int shstk_setup(void)
+{
+   unsigned long addr, size;
+   struct cet_status *cet = >thread.cet;
+
+   if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
+   return -EOPNOTSUPP;
+
+   size = round_up(min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G), 
PAGE_SIZE);
+   addr = alloc_shstk(size, 0);
+   if (IS_ERR_VALUE(addr))
+   return PTR_ERR((void *)addr);
+
+   cet->shstk_base = addr;
+   cet->shstk_size = size;
+
+   start_update_msrs();
+   wrmsrl(MSR_IA32_PL3_SSP, addr + size);
+   wrmsrl(MSR_IA32_U_CET, CET_SHSTK_EN);
+   end_update_msrs();
+   return 0;
+}
+
+void shstk_free(struct task_struct *tsk)
+{
+   struct cet_status *cet = >thread.cet;
+
+   if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
+   !cet->shstk_size ||
+   !cet->shstk_base)
+   return;
+
+   if (!tsk->mm)
+   return;
+
+   while (1) {
+   int r;
+
+   r = vm_munmap(cet->shstk_base, cet->shstk_size);
+
+   /*
+* vm_munmap() returns -EINTR when mmap_lock is held by
+* something else, and that lock should not be held for a
+* long time.  Retry it for the case.
+*/


Hm, no. -EINTR is not about the lock being held by somebody else. The task
got a signal and need to return to userspace.


From tracing the code itself, it looks like it cannot acquire the lock. 
 Let me dig into it.



I have not looked at the rest of the patches yet, but why do you need a
special free path for shadow stack? Why the normal unmap route doesn't
work for you?


The thread's shadow stack is allocated by the kernel, so it needs to be 
freed when the thread exits.



+   if (r == -EINTR) {
+   cond_resched();
+   continue;
+   }
+   break;
+   }
+
+   cet->shstk_base = 0;
+   cet->shstk_size = 0;
+}
+


[...]

Thanks,
Yu-cheng


Re: [PATCH v24 22/30] x86/cet/shstk: Add user-mode shadow stack support

2021-04-09 Thread Kirill A. Shutemov
On Thu, Apr 01, 2021 at 03:10:56PM -0700, Yu-cheng Yu wrote:
> Introduce basic shadow stack enabling/disabling/allocation routines.
> A task's shadow stack is allocated from memory with VM_SHADOW_STACK flag
> and has a fixed size of min(RLIMIT_STACK, 4GB).
> 
> Signed-off-by: Yu-cheng Yu 
> Cc: Kees Cook 
> ---
> v24:
> - Rename cet.c to shstk.c, update related areas accordingly.
> 
>  arch/x86/include/asm/cet.h   |  29 +++
>  arch/x86/include/asm/processor.h |   5 ++
>  arch/x86/kernel/Makefile |   2 +
>  arch/x86/kernel/shstk.c  | 128 +++
>  4 files changed, 164 insertions(+)
>  create mode 100644 arch/x86/include/asm/cet.h
>  create mode 100644 arch/x86/kernel/shstk.c
> 
> diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
> new file mode 100644
> index ..aa85d599b184
> --- /dev/null
> +++ b/arch/x86/include/asm/cet.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_CET_H
> +#define _ASM_X86_CET_H
> +
> +#ifndef __ASSEMBLY__
> +#include 
> +
> +struct task_struct;
> +/*
> + * Per-thread CET status
> + */
> +struct cet_status {
> + unsigned long   shstk_base;
> + unsigned long   shstk_size;
> +};
> +
> +#ifdef CONFIG_X86_SHADOW_STACK
> +int shstk_setup(void);
> +void shstk_free(struct task_struct *p);
> +void shstk_disable(void);
> +#else
> +static inline int shstk_setup(void) { return 0; }
> +static inline void shstk_free(struct task_struct *p) {}
> +static inline void shstk_disable(void) {}
> +#endif
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* _ASM_X86_CET_H */
> diff --git a/arch/x86/include/asm/processor.h 
> b/arch/x86/include/asm/processor.h
> index f1b9ed5efaa9..a5d703fda74e 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -27,6 +27,7 @@ struct vm86;
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -535,6 +536,10 @@ struct thread_struct {
>  
>   unsigned intsig_on_uaccess_err:1;
>  
> +#ifdef CONFIG_X86_CET
> + struct cet_status   cet;
> +#endif
> +
>   /* Floating point and extended processor state */
>   struct fpu  fpu;
>   /*
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 2ddf08351f0b..0f99b093f350 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -150,6 +150,8 @@ obj-$(CONFIG_UNWINDER_FRAME_POINTER)  += 
> unwind_frame.o
>  obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>  
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)+= sev-es.o
> +obj-$(CONFIG_X86_SHADOW_STACK)   += shstk.o
> +
>  ###
>  # 64 bit specific files
>  ifeq ($(CONFIG_X86_64),y)
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> new file mode 100644
> index ..5406fdf6df3c
> --- /dev/null
> +++ b/arch/x86/kernel/shstk.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * shstk.c - Intel shadow stack support
> + *
> + * Copyright (c) 2021, Intel Corporation.
> + * Yu-cheng Yu 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static void start_update_msrs(void)
> +{
> + fpregs_lock();
> + if (test_thread_flag(TIF_NEED_FPU_LOAD))
> + __fpregs_load_activate();
> +}
> +
> +static void end_update_msrs(void)
> +{
> + fpregs_unlock();
> +}
> +
> +static unsigned long alloc_shstk(unsigned long size, int flags)
> +{
> + struct mm_struct *mm = current->mm;
> + unsigned long addr, populate;
> +
> + /* VM_SHADOW_STACK requires MAP_ANONYMOUS, MAP_PRIVATE */
> + flags |= MAP_ANONYMOUS | MAP_PRIVATE;

Looks like all callers has flags == 0. Do I miss something.

> +
> + mmap_write_lock(mm);
> + addr = do_mmap(NULL, 0, size, PROT_READ, flags, VM_SHADOW_STACK, 0,
> +, NULL);
> + mmap_write_unlock(mm);
> +
> + if (populate)
> + mm_populate(addr, populate);

If all callers pass down flags==0, populate will never happen.

> +
> + return addr;
> +}
> +
> +int shstk_setup(void)
> +{
> + unsigned long addr, size;
> + struct cet_status *cet = >thread.cet;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return -EOPNOTSUPP;
> +
> + size = round_up(min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G), 
> PAGE_SIZE);
> + addr = alloc_shstk(size, 0);
> + if (IS_ERR_VALUE(addr))
> + return PTR_ERR((void *)addr);
> +
> + cet->shstk_base = addr;
> + cet->shstk_size = size;
> +
> + start_update_msrs();
> + wrmsrl(MSR_IA32_PL3_SSP, addr + size);
> + wrmsrl(MSR_IA32_U_CET, CET_SHSTK_EN);
> + end_update_msrs();
> + return 0;
> +}
> +
> +void shstk_free(struct task_struct *tsk)
> +{
> + struct cet_status *cet = >thread.cet;
> +
> + if