Messages in this thread Patch in this message | | | Date | Tue, 28 Nov 2023 13:23:57 -0800 | From | Deepak Gupta <> | Subject | Re: [PATCH RFT v4 2/5] fork: Add shadow stack support to clone3() |
| |
On Tue, Nov 28, 2023 at 06:22:40PM +0000, Mark Brown wrote: >Unlike with the normal stack there is no API for configuring the the shadow >stack for a new thread, instead the kernel will dynamically allocate a new >shadow stack with the same size as the normal stack. This appears to be due >to the shadow stack series having been in development since before the more >extensible clone3() was added rather than anything more deliberate. > >Add a parameter to clone3() specifying the size of a shadow stack for >the newly created process. If no shadow stack is specified then the >existing implicit allocation behaviour is maintained. > >If the architecture does not support shadow stacks the shadow stack size >parameter must be zero, architectures that do support the feature are >expected to enforce the same requirement on individual systems that lack >shadow stack support. > >Update the existing x86 implementation to pay attention to the newly added >arguments, in order to maintain compatibility we use the existing behaviour >if no shadow stack is specified. Minimal validation is done of the supplied >parameters, detailed enforcement is left to when the thread is executed. >Since we are now using more fields from the kernel_clone_args we pass that >into the shadow stack code rather than individual fields. > >Signed-off-by: Mark Brown <broonie@kernel.org> >--- > arch/x86/include/asm/shstk.h | 11 +++++---- > arch/x86/kernel/process.c | 2 +- > arch/x86/kernel/shstk.c | 56 ++++++++++++++++++++++++++++++-------------- > include/linux/sched/task.h | 1 + > include/uapi/linux/sched.h | 4 ++++ > kernel/fork.c | 53 +++++++++++++++++++++++++++++++---------- > 6 files changed, 92 insertions(+), 35 deletions(-) > >diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h >index 42fee8959df7..8be7b0a909c3 100644 >--- a/arch/x86/include/asm/shstk.h >+++ b/arch/x86/include/asm/shstk.h >@@ -6,6 +6,7 @@ > #include <linux/types.h> > > struct task_struct; >+struct kernel_clone_args; > struct ksignal; > > #ifdef CONFIG_X86_USER_SHADOW_STACK >@@ -16,8 +17,8 @@ struct thread_shstk { > > long shstk_prctl(struct task_struct *task, int option, unsigned long arg2); > void reset_thread_features(void); >-unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags, >- unsigned long stack_size); >+unsigned long shstk_alloc_thread_stack(struct task_struct *p, >+ const struct kernel_clone_args *args); > void shstk_free(struct task_struct *p); > int setup_signal_shadow_stack(struct ksignal *ksig); > int restore_signal_shadow_stack(void); >@@ -26,8 +27,10 @@ static inline long shstk_prctl(struct task_struct *task, int option, > unsigned long arg2) { return -EINVAL; } > static inline void reset_thread_features(void) {} > static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p, >- unsigned long clone_flags, >- unsigned long stack_size) { return 0; } >+ const struct kernel_clone_args *args) >+{ >+ return 0; >+} > static inline void shstk_free(struct task_struct *p) {} > static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; } > static inline int restore_signal_shadow_stack(void) { return 0; } >diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >index b6f4e8399fca..a9ca80ea5056 100644 >--- a/arch/x86/kernel/process.c >+++ b/arch/x86/kernel/process.c >@@ -207,7 +207,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) > * is disabled, new_ssp will remain 0, and fpu_clone() will know not to > * update it. > */ >- new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->stack_size); >+ new_ssp = shstk_alloc_thread_stack(p, args); > if (IS_ERR_VALUE(new_ssp)) > return PTR_ERR((void *)new_ssp); > >diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c >index 59e15dd8d0f8..0d1325d2d94a 100644 >--- a/arch/x86/kernel/shstk.c >+++ b/arch/x86/kernel/shstk.c >@@ -191,38 +191,58 @@ void reset_thread_features(void) > current->thread.features_locked = 0; > } > >-unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, >- unsigned long stack_size) >+unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, >+ const struct kernel_clone_args *args) > { > struct thread_shstk *shstk = &tsk->thread.shstk; >+ unsigned long clone_flags = args->flags; > unsigned long addr, size; > > /* > * If shadow stack is not enabled on the new thread, skip any >- * switch to a new shadow stack. >+ * implicit switch to a new shadow stack and reject attempts to >+ * explciitly specify one. > */ >- if (!features_enabled(ARCH_SHSTK_SHSTK)) >- return 0; >+ if (!features_enabled(ARCH_SHSTK_SHSTK)) { >+ if (args->shadow_stack_size) >+ return (unsigned long)ERR_PTR(-EINVAL); > >- /* >- * For CLONE_VFORK the child will share the parents shadow stack. >- * Make sure to clear the internal tracking of the thread shadow >- * stack so the freeing logic run for child knows to leave it alone. >- */ >- if (clone_flags & CLONE_VFORK) { >- shstk->base = 0; >- shstk->size = 0; > return 0; > } > > /* >- * For !CLONE_VM the child will use a copy of the parents shadow >- * stack. >+ * If the user specified a shadow stack then do some basic >+ * validation and use it, otherwise fall back to a default >+ * shadow stack size if the clone_flags don't indicate an >+ * allocation is unneeded. > */ >- if (!(clone_flags & CLONE_VM)) >- return 0; >+ if (args->shadow_stack_size) { >+ size = args->shadow_stack_size; >+ } else { >+ /* >+ * For CLONE_VFORK the child will share the parents >+ * shadow stack. Make sure to clear the internal >+ * tracking of the thread shadow stack so the freeing >+ * logic run for child knows to leave it alone. >+ */ >+ if (clone_flags & CLONE_VFORK) { >+ shstk->base = 0; >+ shstk->size = 0; >+ return 0; >+ } >+ >+ /* >+ * For !CLONE_VM the child will use a copy of the >+ * parents shadow stack. >+ */ >+ if (!(clone_flags & CLONE_VM)) >+ return 0; >+ >+ size = args->stack_size; >+ >+ } > >- size = adjust_shstk_size(stack_size); >+ size = adjust_shstk_size(size); > addr = alloc_shstk(0, size, 0, false); > if (IS_ERR_VALUE(addr)) > return addr; >diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h >index a23af225c898..e86a09cfccd8 100644 >--- a/include/linux/sched/task.h >+++ b/include/linux/sched/task.h >@@ -41,6 +41,7 @@ struct kernel_clone_args { > void *fn_arg; > struct cgroup *cgrp; > struct css_set *cset; >+ unsigned long shadow_stack_size; > }; > > /* >diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h >index 3bac0a8ceab2..a998b6d0c897 100644 >--- a/include/uapi/linux/sched.h >+++ b/include/uapi/linux/sched.h >@@ -84,6 +84,8 @@ > * kernel's limit of nested PID namespaces. > * @cgroup: If CLONE_INTO_CGROUP is specified set this to > * a file descriptor for the cgroup. >+ * @shadow_stack_size: Specify the size of the shadow stack to allocate >+ * for the child process. > * > * The structure is versioned by size and thus extensible. > * New struct members must go at the end of the struct and >@@ -101,12 +103,14 @@ struct clone_args { > __aligned_u64 set_tid; > __aligned_u64 set_tid_size; > __aligned_u64 cgroup; >+ __aligned_u64 shadow_stack_size; > }; > #endif > > #define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ > #define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */ > #define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */ >+#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */ > > /* > * Scheduling policies >diff --git a/kernel/fork.c b/kernel/fork.c >index 10917c3e1f03..35131acd43d2 100644 >--- a/kernel/fork.c >+++ b/kernel/fork.c >@@ -121,6 +121,11 @@ > */ > #define MAX_THREADS FUTEX_TID_MASK > >+/* >+ * Require that shadow stacks can store at least one element >+ */ >+#define SHADOW_STACK_SIZE_MIN 8
nit: Sorry, should've mentioned it earlier. Can this be "#define SHADOW_STACK_SIZE_MIN sizeof(unsigned long)"
>+ > /* > * Protected counters by write_lock_irq(&tasklist_lock) > */ >@@ -3067,7 +3072,9 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs, > CLONE_ARGS_SIZE_VER1); > BUILD_BUG_ON(offsetofend(struct clone_args, cgroup) != > CLONE_ARGS_SIZE_VER2); >- BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
| |