| Message ID | 20230218211433.26859-29-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Sat, Feb 18, 2023 at 01:14:20PM -0800, Rick Edgecombe wrote: > diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h > index 500b96e71f18..b2b3b7200b2d 100644 > --- a/arch/x86/include/uapi/asm/prctl.h > +++ b/arch/x86/include/uapi/asm/prctl.h > @@ -20,4 +20,10 @@ > #define ARCH_MAP_VDSO_32 0x2002 > #define ARCH_MAP_VDSO_64 0x2003 > > +/* Don't use 0x3001-0x3004 because of old glibcs */ So where is this all new interface to userspace programs documented? Do we have an agreement with all the involved parties that this is how we're going to support shadow stacks and that this is what userspace should do? I'd like to avoid one more fiasco with glibc etc here... Thx.
On Fri, 2023-02-24 at 13:20 +0100, Borislav Petkov wrote: > On Sat, Feb 18, 2023 at 01:14:20PM -0800, Rick Edgecombe wrote: > > diff --git a/arch/x86/include/uapi/asm/prctl.h > > b/arch/x86/include/uapi/asm/prctl.h > > index 500b96e71f18..b2b3b7200b2d 100644 > > --- a/arch/x86/include/uapi/asm/prctl.h > > +++ b/arch/x86/include/uapi/asm/prctl.h > > @@ -20,4 +20,10 @@ > > #define ARCH_MAP_VDSO_32 0x2002 > > #define ARCH_MAP_VDSO_64 0x2003 > > > > +/* Don't use 0x3001-0x3004 because of old glibcs */ > > So where is this all new interface to userspace programs documented? In the first patch: https://lore.kernel.org/lkml/20230218211433.26859-2-rick.p.edgecombe@intel.com/ Then some more documentation is added about ARCH_SHSTK_UNLOCK and ARCH_SHSTK_STATUS (which are for CRIU) in those patches. > Do > we have an agreement with all the involved parties that this is how > we're going to support shadow stacks and that this is what userspace > should do? > > I'd like to avoid one more fiasco with glibc etc here... There are glibc patches prepared by HJ to use the new interface and it's my understanding that he has discussed the changes with the other glibc folks. Those glibc patches are used for testing these kernel patches, but will not get upstream until the kernel patches to avoid repeating the past problems. So I think it's as prepared as it can be. One future thing that might come up... Glibc has this mode called "permissive mode". When glibc is configured this way (compile time or env var), it is supposed to disable shadow stack when dlopen()ing any DSO that doesn't have the shadow stack elf header bit. The problem is, it doesn't really work as intended. It only turns off shadow stack for the calling thread, leaving the other threads free to call into the DSO with shadow stack enabled. It's not clear yet if they are going to be able to fix it in userspace. So this prompted some thinking of if there could be an additional kernel mode to help glibc in this scenario. But for non-permissive mode, glibc is queued up to use the interface as is.
On Fri, Feb 24, 2023 at 06:37:57PM +0000, Edgecombe, Rick P wrote: > In the first patch: > > https://lore.kernel.org/lkml/20230218211433.26859-2-rick.p.edgecombe@intel.com/ > > Then some more documentation is added about ARCH_SHSTK_UNLOCK and > ARCH_SHSTK_STATUS (which are for CRIU) in those patches. Right, I was thinking more about ARCH_PRCTL(2), the man page. But you can send that to the manpages folks later. I.e., it should be nearly impossible to be missed. :) > There are glibc patches prepared by HJ to use the new interface and > it's my understanding that he has discussed the changes with the other > glibc folks. Those glibc patches are used for testing these kernel > patches, but will not get upstream until the kernel patches to avoid > repeating the past problems. So I think it's as prepared as it can be. Good. > One future thing that might come up... Glibc has this mode called > "permissive mode". When glibc is configured this way (compile time or > env var), it is supposed to disable shadow stack when dlopen()ing any > DSO that doesn't have the shadow stack elf header bit. Maybe I don't understand all the possible use cases but if I were interested in using shadow stack, then I'd enable it for all objects. And if I want permissive, I'd disable it for all. A mixed thing sounds like a mixed can of worms waiting to be opened.
On Tue, 2023-02-28 at 11:58 +0100, Borislav Petkov wrote: > On Fri, Feb 24, 2023 at 06:37:57PM +0000, Edgecombe, Rick P wrote: > > In the first patch: > > > > https://lore.kernel.org/lkml/20230218211433.26859-2-rick.p.edgecombe@intel.com/ > > > > Then some more documentation is added about ARCH_SHSTK_UNLOCK and > > ARCH_SHSTK_STATUS (which are for CRIU) in those patches. > > Right, I was thinking more about ARCH_PRCTL(2), the man page. > > But you can send that to the manpages folks later. I.e., it should be > nearly impossible to be missed. :) Sure I can add something. Somewhere I have a man page for map_shadow_stack written up as well. > > > There are glibc patches prepared by HJ to use the new interface and > > it's my understanding that he has discussed the changes with the > > other > > glibc folks. Those glibc patches are used for testing these kernel > > patches, but will not get upstream until the kernel patches to > > avoid > > repeating the past problems. So I think it's as prepared as it can > > be. > > Good. > > > One future thing that might come up... Glibc has this mode called > > "permissive mode". When glibc is configured this way (compile time > > or > > env var), it is supposed to disable shadow stack when dlopen()ing > > any > > DSO that doesn't have the shadow stack elf header bit. > > Maybe I don't understand all the possible use cases but if I were > interested in using shadow stack, then I'd enable it for all objects. Enabling for all objects is the ideal, but in practice distros don't have that. > And if I want permissive, I'd disable it for all. A mixed thing > sounds > like a mixed can of worms waiting to be opened. It is definitely a can of worms.
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 8d73004e4cac..bd16e012b3e9 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -28,6 +28,7 @@ struct vm86; #include <asm/unwind_hints.h> #include <asm/vmxfeatures.h> #include <asm/vdso/processor.h> +#include <asm/shstk.h> #include <linux/personality.h> #include <linux/cache.h> @@ -475,6 +476,11 @@ struct thread_struct { */ u32 pkru; +#ifdef CONFIG_X86_USER_SHADOW_STACK + unsigned long features; + unsigned long features_locked; +#endif + /* Floating point and extended processor state */ struct fpu fpu; /* diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h new file mode 100644 index 000000000000..ec753809f074 --- /dev/null +++ b/arch/x86/include/asm/shstk.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_X86_SHSTK_H +#define _ASM_X86_SHSTK_H + +#ifndef __ASSEMBLY__ +#include <linux/types.h> + +struct task_struct; + +#ifdef CONFIG_X86_USER_SHADOW_STACK +long shstk_prctl(struct task_struct *task, int option, unsigned long features); +void reset_thread_features(void); +#else +static inline long shstk_prctl(struct task_struct *task, int option, + unsigned long arg2) { return -EINVAL; } +static inline void reset_thread_features(void) {} +#endif /* CONFIG_X86_USER_SHADOW_STACK */ + +#endif /* __ASSEMBLY__ */ + +#endif /* _ASM_X86_SHSTK_H */ diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h index 500b96e71f18..b2b3b7200b2d 100644 --- a/arch/x86/include/uapi/asm/prctl.h +++ b/arch/x86/include/uapi/asm/prctl.h @@ -20,4 +20,10 @@ #define ARCH_MAP_VDSO_32 0x2002 #define ARCH_MAP_VDSO_64 0x2003 +/* Don't use 0x3001-0x3004 because of old glibcs */ + +#define ARCH_SHSTK_ENABLE 0x5001 +#define ARCH_SHSTK_DISABLE 0x5002 +#define ARCH_SHSTK_LOCK 0x5003 + #endif /* _ASM_X86_PRCTL_H */ diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 92446f1dedd7..b366641703e3 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -146,6 +146,8 @@ obj-$(CONFIG_CALL_THUNKS) += callthunks.o obj-$(CONFIG_X86_CET) += cet.o +obj-$(CONFIG_X86_USER_SHADOW_STACK) += shstk.o + ### # 64 bit specific files ifeq ($(CONFIG_X86_64),y) diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index 4e34b3b68ebd..71094c8a305f 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c @@ -514,6 +514,8 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip, load_gs_index(__USER_DS); } + reset_thread_features(); + loadsegment(fs, 0); loadsegment(es, _ds); loadsegment(ds, _ds); @@ -830,7 +832,10 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2) case ARCH_MAP_VDSO_64: return prctl_map_vdso(&vdso_image_64, arg2); #endif - + case ARCH_SHSTK_ENABLE: + case ARCH_SHSTK_DISABLE: + case ARCH_SHSTK_LOCK: + return shstk_prctl(task, option, arg2); default: ret = -EINVAL; break; diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c new file mode 100644 index 000000000000..41ed6552e0a5 --- /dev/null +++ b/arch/x86/kernel/shstk.c @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * shstk.c - Intel shadow stack support + * + * Copyright (c) 2021, Intel Corporation. + * Yu-cheng Yu <yu-cheng.yu@intel.com> + */ + +#include <linux/sched.h> +#include <linux/bitops.h> +#include <asm/prctl.h> + +void reset_thread_features(void) +{ + current->thread.features = 0; + current->thread.features_locked = 0; +} + +long shstk_prctl(struct task_struct *task, int option, unsigned long features) +{ + if (option == ARCH_SHSTK_LOCK) { + task->thread.features_locked |= features; + return 0; + } + + /* Don't allow via ptrace */ + if (task != current) + return -EINVAL; + + /* Do not allow to change locked features */ + if (features & task->thread.features_locked) + return -EPERM; + + /* Only support enabling/disabling one feature at a time. */ + if (hweight_long(features) > 1) + return -EINVAL; + + if (option == ARCH_SHSTK_DISABLE) { + return -EINVAL; + } + + /* Handle ARCH_SHSTK_ENABLE */ + return -EINVAL; +}