| Message ID | 20221104223604.29615-38-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Fri, Nov 4, 2022 at 3:40 PM Rick Edgecombe <rick.p.edgecombe@intel.com> wrote: > > The x86 Control-flow Enforcement Technology (CET) feature includes a new > feature called shadow stacks that provides security enforcement of > behavior that is rarely used by non-attackers. > > There exists a lurking compatibility problem for userspace shadow stack. > Old binaries exist that are marked as supporting shadow stack in their > elf header, but actually will crash if shadow stack is enabled. This would > only happens if the loader chooses to call the kernel APIs that enable > shadow stack. However, glibc plans to update to do just this. At which > point the old apps will crash. > > In a lot of ways this is userspace's business, however the kernel could > save the user from these crashes. It could do this by detecting the elf > bit and blocking the shadow stack APIs, so that loader (glibc) will fail > to enable shadow stack and the binary would then run without shadow stack. > So implement this logic in the elf processing that happens during exec. > > This is a bit dirty, and implemented here just for discussion on whether > the kernel should actually do something like this. > > The elf loading logic in the kernel has to do a little extra scanning > through the elf header in order to find the shadow stack bit. > > Since some people may not mind if some apps crash, also create > a Kconfig X86_USER_SHADOW_STACK_ALLOW_BROKEN to allow the old binaries > to still have access to the shadow stack kernel APIs. > > This is based on an earlier patch by Yu-cheng Yu that was looking at elf > bits on the interpreter instead of the execing binary. > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > arch/arm64/include/asm/elf.h | 5 +++++ > arch/x86/Kconfig | 13 +++++++++++++ > arch/x86/include/asm/cet.h | 2 ++ > arch/x86/include/asm/elf.h | 11 +++++++++++ > arch/x86/include/asm/processor.h | 1 + > arch/x86/kernel/process_64.c | 33 ++++++++++++++++++++++++++++++++ > arch/x86/kernel/shstk.c | 15 +++++++++++++++ > fs/binfmt_elf.c | 24 ++++++++++++++++++++++- > include/linux/elf.h | 6 ++++++ > include/uapi/linux/elf.h | 15 +++++++++++++++ > 10 files changed, 124 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h > index 97932fbf973d..1aa76ed02dda 100644 > --- a/arch/arm64/include/asm/elf.h > +++ b/arch/arm64/include/asm/elf.h > @@ -279,6 +279,11 @@ static inline int arch_parse_elf_property(u32 type, const void *data, > return 0; > } > > +static inline int arch_process_elf_property(struct arch_elf_state *arch) > +{ > + return 0; > +} > + > static inline int arch_elf_pt_proc(void *ehdr, void *phdr, > struct file *f, bool is_interp, > struct arch_elf_state *state) > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index f3d14f5accce..da9e43aa91a3 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -28,6 +28,7 @@ config X86_64 > select ARCH_HAS_GIGANTIC_PAGE > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 > select ARCH_USE_CMPXCHG_LOCKREF > + select ARCH_USE_GNU_PROPERTY > select HAVE_ARCH_SOFT_DIRTY > select MODULES_USE_ELF_RELA > select NEED_DMA_MAP_STATE > @@ -60,6 +61,7 @@ config X86 > select ACPI_LEGACY_TABLES_LOOKUP if ACPI > select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI > select ARCH_32BIT_OFF_T if X86_32 > + select ARCH_BINFMT_ELF_STATE > select ARCH_CLOCKSOURCE_INIT > select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE > select ARCH_ENABLE_HUGEPAGE_MIGRATION if X86_64 && HUGETLB_PAGE && MIGRATION > @@ -1977,6 +1979,17 @@ config X86_USER_SHADOW_STACK > > If unsure, say N. > > +config X86_USER_SHADOW_STACK_ALLOW_BROKEN > + bool "Allow enabling shadow stack for broken binaries" > + depends on EXPERT > + depends on X86_USER_SHADOW_STACK > + help > + There exist old binaries that are marked as compatible with shadow > + stack, but actually aren't. The kernel blocks these binaries from > + getting shadow stack enabled by default. But some working binaries > + are also blocked. Select this option if you would like to allow these > + binaries to run with shadow stack, and possibly crash. > + > config EFI > bool "EFI runtime service support" > depends on ACPI > diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h > index 098e4ecfdf9b..7f0cabb3db21 100644 > --- a/arch/x86/include/asm/cet.h > +++ b/arch/x86/include/asm/cet.h > @@ -22,6 +22,7 @@ int shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags, > void shstk_free(struct task_struct *p); > int setup_signal_shadow_stack(struct ksignal *ksig); > int restore_signal_shadow_stack(void); > +void bad_cet_binary_disable(bool disable); > #else > static inline long cet_prctl(struct task_struct *task, int option, > unsigned long features) { return -EINVAL; } > @@ -33,6 +34,7 @@ static inline int shstk_alloc_thread_stack(struct task_struct *p, > 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; } > +static inline void bad_cet_binary_disable(bool disable) {}; > #endif /* CONFIG_X86_USER_SHADOW_STACK */ > > #endif /* __ASSEMBLY__ */ > diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h > index cb0ff1055ab1..95ee133acffb 100644 > --- a/arch/x86/include/asm/elf.h > +++ b/arch/x86/include/asm/elf.h > @@ -383,6 +383,17 @@ extern int compat_arch_setup_additional_pages(struct linux_binprm *bprm, > > extern bool arch_syscall_is_vdso_sigreturn(struct pt_regs *regs); > > +struct arch_elf_state { > + unsigned int gnu_property; > +}; > + > +#define INIT_ARCH_ELF_STATE { \ > + .gnu_property = 0, \ > +} > + > +#define arch_elf_pt_proc(ehdr, phdr, elf, interp, state) (0) > +#define arch_check_elf(ehdr, interp, interp_ehdr, state) (0) > + > /* Do not change the values. See get_align_mask() */ > enum align_flags { > ALIGN_VA_32 = BIT(0), > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index a6c414dfd10f..4b333c801010 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -534,6 +534,7 @@ struct thread_struct { > #ifdef CONFIG_X86_USER_SHADOW_STACK > unsigned long features; > unsigned long features_locked; > + bool bad_cet_binary_disable; > > struct thread_shstk shstk; > #endif > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c > index 03bc16c9cc19..461b8e9468df 100644 > --- a/arch/x86/kernel/process_64.c > +++ b/arch/x86/kernel/process_64.c > @@ -867,3 +867,36 @@ unsigned long KSTK_ESP(struct task_struct *task) > { > return task_pt_regs(task)->sp; > } > + > +#ifdef CONFIG_X86_USER_SHADOW_STACK > +int arch_parse_elf_property(u32 type, const void *data, size_t datasz, > + bool compat, struct arch_elf_state *state) > +{ > + if (type != GNU_PROPERTY_X86_FEATURE_1_AND) > + return 0; > + > + if (datasz != sizeof(unsigned int)) > + return -ENOEXEC; > + > + state->gnu_property = *(unsigned int *)data; > + return 0; > +} > + > +int arch_process_elf_property(struct arch_elf_state *state) > +{ > + bad_cet_binary_disable(state->gnu_property & GNU_PROPERTY_X86_FEATURE_1_BAD); > + return 0; > +} > +#else /* CONFIG_X86_USER_SHADOW_STACK */ > +int arch_parse_elf_property(u32 type, const void *data, size_t datasz, > + bool compat, struct arch_elf_state *state) > +{ > + return 0; > +} > + > +int arch_process_elf_property(struct arch_elf_state *state) > +{ > + return 0; > +} > +#endif /* CONFIG_X86_USER_SHADOW_STACK */ > + > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c > index bed7032d35f2..cb105e69c840 100644 > --- a/arch/x86/kernel/shstk.c > +++ b/arch/x86/kernel/shstk.c > @@ -445,6 +445,9 @@ SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsi > > long cet_prctl(struct task_struct *task, int option, unsigned long features) > { > + if (task->thread.bad_cet_binary_disable) > + return -EINVAL; > + > if (option == ARCH_CET_LOCK) { > task->thread.features_locked |= features; > return 0; > @@ -482,3 +485,15 @@ long cet_prctl(struct task_struct *task, int option, unsigned long features) > return wrss_control(true); > return -EINVAL; > } > + > +#ifdef CONFIG_X86_USER_SHADOW_STACK_ALLOW_BROKEN > +void bad_cet_binary_disable(bool disable) > +{ > + current->thread.bad_cet_binary_disable = false; > +} > +#else /* CONFIG_X86_USER_SHADOW_STACK_ALLOW_BROKEN */ > +void bad_cet_binary_disable(bool disable) > +{ > + current->thread.bad_cet_binary_disable = disable; > +} > +#endif /* CONFIG_X86_USER_SHADOW_STACK_ALLOW_BROKEN */ > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 6a11025e5850..8b6ae5e423fb 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -764,6 +764,8 @@ static int parse_elf_property(const char *data, size_t *off, size_t datasz, > #define GNU_PROPERTY_TYPE_0_NAME "GNU" > #define NOTE_NAME_SZ (sizeof(GNU_PROPERTY_TYPE_0_NAME)) > > + > + > static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr, > struct arch_elf_state *arch) > { > @@ -821,6 +823,18 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr, > return ret == -ENOENT ? 0 : ret; > } > > +static int check_elf_properties(struct file *f, const struct elf_phdr *phdr) > +{ > + struct arch_elf_state arch_state = INIT_ARCH_ELF_STATE; > + int retval; > + > + retval = parse_elf_properties(f, phdr, &arch_state); > + if (retval) > + return retval; > + > + return arch_process_elf_property(&arch_state); > +} > + > static int load_elf_binary(struct linux_binprm *bprm) > { > struct file *interpreter = NULL; /* to shut gcc up */ > @@ -920,13 +934,21 @@ static int load_elf_binary(struct linux_binprm *bprm) > if (retval < 0) > goto out_free_dentry; > > - break; > + /* Quit if already found PT_GNU_PROPERTY */ > + if (elf_property_phdata) > + break; > + > + continue; > > out_free_interp: > kfree(elf_interpreter); > goto out_free_ph; > } > > + retval = check_elf_properties(bprm->file, elf_property_phdata); > + if (retval) > + return retval; > + > elf_ppnt = elf_phdata; > for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) > switch (elf_ppnt->p_type) { > diff --git a/include/linux/elf.h b/include/linux/elf.h > index c9a46c4e183b..faf961b92a95 100644 > --- a/include/linux/elf.h > +++ b/include/linux/elf.h > @@ -92,9 +92,15 @@ static inline int arch_parse_elf_property(u32 type, const void *data, > { > return 0; > } > + > +static inline int arch_process_elf_property(struct arch_elf_state *arch) > +{ > + return 0; > +} > #else > extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz, > bool compat, struct arch_elf_state *arch); > +extern int arch_process_elf_property(struct arch_elf_state *arch); > #endif > > #ifdef CONFIG_ARCH_HAVE_ELF_PROT > diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h > index 11089731e2e9..d9b58adce321 100644 > --- a/include/uapi/linux/elf.h > +++ b/include/uapi/linux/elf.h > @@ -469,4 +469,19 @@ typedef struct elf64_note { > /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */ > #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) > > +/* > + * See the x86 64 psABI at: > + * https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/x86-64-psABI > + * .note.gnu.property types for x86: > + */ > +/* 0xc0000000 and 0xc0000001 are reserved */ > +#define GNU_PROPERTY_X86_FEATURE_1_AND 0xc0000002 > + > +/* Bits for GNU_PROPERTY_X86_FEATURE_1_AND */ > +#define GNU_PROPERTY_X86_FEATURE_1_IBT 0x00000001 > +#define GNU_PROPERTY_X86_FEATURE_1_SHSTK 0x00000002 > + > +#define GNU_PROPERTY_X86_FEATURE_1_BAD (GNU_PROPERTY_X86_FEATURE_1_IBT | \ > + GNU_PROPERTY_X86_FEATURE_1_SHSTK) > + > #endif /* _UAPI_LINUX_ELF_H */ > -- > 2.17.1 > This change doesn't make a binary CET compatible. It just requires that the toolchain must be updated and all binaries have to be recompiled with the new toolchain to enable CET. It doesn't solve any issue which can't be solved by not updating glibc.
* H. J. Lu: > This change doesn't make a binary CET compatible. It just requires > that the toolchain must be updated and all binaries have to be > recompiled with the new toolchain to enable CET. It doesn't solve any > issue which can't be solved by not updating glibc. Right, and it doesn't even address the library case (the kernel would have to hook into mmap for that). The kernel shouldn't do this. Thanks, Florian
On Fri, 2022-11-04 at 15:56 -0700, H.J. Lu wrote: > This change doesn't make a binary CET compatible. It just requires > that the toolchain > must be updated and all binaries have to be recompiled with the new > toolchain to > enable CET. I guess you mean distros could again blindly mark all binaries as supporting shadow stack? I think they would see the failures pretty quickly in this case, unlike the first time where there was little HW and no kernel support. > It doesn't solve any issue which can't be solved by not > updating glibc. If users never updates glibc, there won't be a problem, as I elaborated on in the coverletter. But how are they supposed to know the consequences of turning on CET?
On Sun, 2022-11-06 at 10:33 +0100, Florian Weimer wrote: > * H. J. Lu: > > > This change doesn't make a binary CET compatible. It just requires > > that the toolchain must be updated and all binaries have to be > > recompiled with the new toolchain to enable CET. It doesn't solve > > any > > issue which can't be solved by not updating glibc. > > Right, and it doesn't even address the library case (the kernel would > have to hook into mmap for that). The kernel shouldn't do this. Shadow stack shouldn't enable as a result of loading a library, if that's what you mean.
* Rick P. Edgecombe: > On Sun, 2022-11-06 at 10:33 +0100, Florian Weimer wrote: >> * H. J. Lu: >> >> > This change doesn't make a binary CET compatible. It just requires >> > that the toolchain must be updated and all binaries have to be >> > recompiled with the new toolchain to enable CET. It doesn't solve >> > any >> > issue which can't be solved by not updating glibc. >> >> Right, and it doesn't even address the library case (the kernel would >> have to hook into mmap for that). The kernel shouldn't do this. > > Shadow stack shouldn't enable as a result of loading a library, if > that's what you mean. It's the opposite—loading incompatible libraries needs to disable shadow stack (or ideally, not enable it in the first place). Technically, I think most incompatible code resides in libraries, so this kernel change achieves nothing besides punishing early implementations of the published-as-finalized x86-64 ABI. Thanks, Florian
On Mon, 2022-11-07 at 17:55 +0100, Florian Weimer wrote: > * Rick P. Edgecombe: > > > On Sun, 2022-11-06 at 10:33 +0100, Florian Weimer wrote: > > > * H. J. Lu: > > > > > > > This change doesn't make a binary CET compatible. It just > > > > requires > > > > that the toolchain must be updated and all binaries have to be > > > > recompiled with the new toolchain to enable CET. It doesn't > > > > solve > > > > any > > > > issue which can't be solved by not updating glibc. > > > > > > Right, and it doesn't even address the library case (the kernel > > > would > > > have to hook into mmap for that). The kernel shouldn't do this. > > > > Shadow stack shouldn't enable as a result of loading a library, if > > that's what you mean. > > It's the opposite—loading incompatible libraries needs to disable > shadow > stack (or ideally, not enable it in the first place). The glibc changes I have been using would not have enabled shadow stack in the first place unless the execing binary has the elf bit. So the binary would run as if shadow stack was not enabled in the kernel and there should be nothing to disable when an incompatible binary is loaded. Glibc will have to detect this and act accordingly because not all kernels will have shadow stack configured. > Technically, I > think most incompatible code resides in libraries, so this kernel > change > achieves nothing besides punishing early implementations of the > published-as-finalized x86-64 ABI. It's under the assumption that not breaking things is more important than having shadow stack enabled. So it is not intended as a punishment for users at all, rather the opposite. I'm not sure how much the spec mandates things by the letter of it, but in any case things have gone wrong in the real world. I am very open to discussion here. I only went this way as a last resort because I didn't hear back on the last thread.
On Mon, Nov 7, 2022 at 9:47 AM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > On Mon, 2022-11-07 at 17:55 +0100, Florian Weimer wrote: > > * Rick P. Edgecombe: > > > > > On Sun, 2022-11-06 at 10:33 +0100, Florian Weimer wrote: > > > > * H. J. Lu: > > > > > > > > > This change doesn't make a binary CET compatible. It just > > > > > requires > > > > > that the toolchain must be updated and all binaries have to be > > > > > recompiled with the new toolchain to enable CET. It doesn't > > > > > solve > > > > > any > > > > > issue which can't be solved by not updating glibc. > > > > > > > > Right, and it doesn't even address the library case (the kernel > > > > would > > > > have to hook into mmap for that). The kernel shouldn't do this. > > > > > > Shadow stack shouldn't enable as a result of loading a library, if > > > that's what you mean. > > > > It's the opposite—loading incompatible libraries needs to disable > > shadow > > stack (or ideally, not enable it in the first place). > > The glibc changes I have been using would not have enabled shadow stack > in the first place unless the execing binary has the elf bit. So the > binary would run as if shadow stack was not enabled in the kernel and > there should be nothing to disable when an incompatible binary is > loaded. Glibc will have to detect this and act accordingly because not > all kernels will have shadow stack configured. > > > Technically, I > > think most incompatible code resides in libraries, so this kernel > > change > > achieves nothing besides punishing early implementations of the > > published-as-finalized x86-64 ABI. > > It's under the assumption that not breaking things is more important > than having shadow stack enabled. So it is not intended as a punishment > for users at all, rather the opposite. > > I'm not sure how much the spec mandates things by the letter of it, but > in any case things have gone wrong in the real world. I am very open to > discussion here. I only went this way as a last resort because I didn't > hear back on the last thread. Some applications and libraries are compiled with -fcf-protection, but they manipulate the stack in such a way that they aren't compatible with the shadow stack. However, if the build/test setup doesn't support shadow stack, it is impossible to validate.
On Mon, 2022-11-07 at 11:10 -0800, H.J. Lu wrote: > On Mon, Nov 7, 2022 at 9:47 AM Edgecombe, Rick P > <rick.p.edgecombe@intel.com> wrote: > > > > On Mon, 2022-11-07 at 17:55 +0100, Florian Weimer wrote: > > > * Rick P. Edgecombe: > > > > > > > On Sun, 2022-11-06 at 10:33 +0100, Florian Weimer wrote: > > > > > * H. J. Lu: > > > > > > > > > > > This change doesn't make a binary CET compatible. It just > > > > > > requires > > > > > > that the toolchain must be updated and all binaries have to > > > > > > be > > > > > > recompiled with the new toolchain to enable CET. It > > > > > > doesn't > > > > > > solve > > > > > > any > > > > > > issue which can't be solved by not updating glibc. > > > > > > > > > > Right, and it doesn't even address the library case (the > > > > > kernel > > > > > would > > > > > have to hook into mmap for that). The kernel shouldn't do > > > > > this. > > > > > > > > Shadow stack shouldn't enable as a result of loading a library, > > > > if > > > > that's what you mean. > > > > > > It's the opposite—loading incompatible libraries needs to disable > > > shadow > > > stack (or ideally, not enable it in the first place). > > > > The glibc changes I have been using would not have enabled shadow > > stack > > in the first place unless the execing binary has the elf bit. So > > the > > binary would run as if shadow stack was not enabled in the kernel > > and > > there should be nothing to disable when an incompatible binary is > > loaded. Glibc will have to detect this and act accordingly because > > not > > all kernels will have shadow stack configured. > > > > > Technically, I > > > think most incompatible code resides in libraries, so this kernel > > > change > > > achieves nothing besides punishing early implementations of the > > > published-as-finalized x86-64 ABI. > > > > It's under the assumption that not breaking things is more > > important > > than having shadow stack enabled. So it is not intended as a > > punishment > > for users at all, rather the opposite. > > > > I'm not sure how much the spec mandates things by the letter of it, > > but > > in any case things have gone wrong in the real world. I am very > > open to > > discussion here. I only went this way as a last resort because I > > didn't > > hear back on the last thread. > > Some applications and libraries are compiled with -fcf-protection, > but > they manipulate the stack in such a way that they aren't compatible > with the shadow stack. However, if the build/test setup doesn't > support > shadow stack, it is impossible to validate. > When we have everything in place, the problems would be much more obvious when distros started turning it on. But we can't turn it on as planned without breaking things for existing binaries. We can have both by: 1. Choosing a new bit, adding it to the tools, and never supporting the old bit in glibc. 2. Providing the option to have the kernel block the old bit, so upgraded users can decide what experience they would like. Then distros can find the problems and adjust their packages. I'm starting to think a default off sysctl toggle might be better than a Kconfig. 3. Any other ideas?
On Mon, Nov 7, 2022 at 1:11 PM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > On Mon, 2022-11-07 at 11:10 -0800, H.J. Lu wrote: > > On Mon, Nov 7, 2022 at 9:47 AM Edgecombe, Rick P > > <rick.p.edgecombe@intel.com> wrote: > > > > > > On Mon, 2022-11-07 at 17:55 +0100, Florian Weimer wrote: > > > > * Rick P. Edgecombe: > > > > > > > > > On Sun, 2022-11-06 at 10:33 +0100, Florian Weimer wrote: > > > > > > * H. J. Lu: > > > > > > > > > > > > > This change doesn't make a binary CET compatible. It just > > > > > > > requires > > > > > > > that the toolchain must be updated and all binaries have to > > > > > > > be > > > > > > > recompiled with the new toolchain to enable CET. It > > > > > > > doesn't > > > > > > > solve > > > > > > > any > > > > > > > issue which can't be solved by not updating glibc. > > > > > > > > > > > > Right, and it doesn't even address the library case (the > > > > > > kernel > > > > > > would > > > > > > have to hook into mmap for that). The kernel shouldn't do > > > > > > this. > > > > > > > > > > Shadow stack shouldn't enable as a result of loading a library, > > > > > if > > > > > that's what you mean. > > > > > > > > It's the opposite—loading incompatible libraries needs to disable > > > > shadow > > > > stack (or ideally, not enable it in the first place). > > > > > > The glibc changes I have been using would not have enabled shadow > > > stack > > > in the first place unless the execing binary has the elf bit. So > > > the > > > binary would run as if shadow stack was not enabled in the kernel > > > and > > > there should be nothing to disable when an incompatible binary is > > > loaded. Glibc will have to detect this and act accordingly because > > > not > > > all kernels will have shadow stack configured. > > > > > > > Technically, I > > > > think most incompatible code resides in libraries, so this kernel > > > > change > > > > achieves nothing besides punishing early implementations of the > > > > published-as-finalized x86-64 ABI. > > > > > > It's under the assumption that not breaking things is more > > > important > > > than having shadow stack enabled. So it is not intended as a > > > punishment > > > for users at all, rather the opposite. > > > > > > I'm not sure how much the spec mandates things by the letter of it, > > > but > > > in any case things have gone wrong in the real world. I am very > > > open to > > > discussion here. I only went this way as a last resort because I > > > didn't > > > hear back on the last thread. > > > > Some applications and libraries are compiled with -fcf-protection, > > but > > they manipulate the stack in such a way that they aren't compatible > > with the shadow stack. However, if the build/test setup doesn't > > support > > shadow stack, it is impossible to validate. > > > > When we have everything in place, the problems would be much more > obvious when distros started turning it on. But we can't turn it on as Not necessarily. The problem will show up only in a CET enabled environment since build/test setup may not be on a CET capable hardware. > planned without breaking things for existing binaries. We can have both > by: > 1. Choosing a new bit, adding it to the tools, and never supporting the > old bit in glibc. > 2. Providing the option to have the kernel block the old bit, so > upgraded users can decide what experience they would like. Then distros > can find the problems and adjust their packages. I'm starting to think > a default off sysctl toggle might be better than a Kconfig. > 3. Any other ideas? Don't enable CET in glibc until we can validate CET functionality.
On Mon, 2022-11-07 at 13:21 -0800, H.J. Lu wrote: > > > Some applications and libraries are compiled with -fcf- > > > protection, > > > but > > > they manipulate the stack in such a way that they aren't > > > compatible > > > with the shadow stack. However, if the build/test setup doesn't > > > support > > > shadow stack, it is impossible to validate. > > > > > > > When we have everything in place, the problems would be much more > > obvious when distros started turning it on. But we can't turn it on > > as > > Not necessarily. The problem will show up only in a CET enabled > environment since build/test setup may not be on a CET capable > hardware. Well, I'm not sure of the details of distro testing, but there are plenty of TGL and later systems out there today. With kernel support, I'm thinking these types of problems couldn't lurk for years like they have. > > > planned without breaking things for existing binaries. We can have > > both > > by: > > 1. Choosing a new bit, adding it to the tools, and never supporting > > the > > old bit in glibc. > > 2. Providing the option to have the kernel block the old bit, so > > upgraded users can decide what experience they would like. Then > > distros > > can find the problems and adjust their packages. I'm starting to > > think > > a default off sysctl toggle might be better than a Kconfig. > > 3. Any other ideas? > > Don't enable CET in glibc until we can validate CET functionality. Can you elaborate on what you mean by this? Not upstream glibc CET support? Or have users not enable it? If the latter, how would they know about all these problems. And what is wrong with the cleanest option, number 1? The ABI document can be updated.
On Mon, Nov 7, 2022 at 1:34 PM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > On Mon, 2022-11-07 at 13:21 -0800, H.J. Lu wrote: > > > > Some applications and libraries are compiled with -fcf- > > > > protection, > > > > but > > > > they manipulate the stack in such a way that they aren't > > > > compatible > > > > with the shadow stack. However, if the build/test setup doesn't > > > > support > > > > shadow stack, it is impossible to validate. > > > > > > > > > > When we have everything in place, the problems would be much more > > > obvious when distros started turning it on. But we can't turn it on > > > as > > > > Not necessarily. The problem will show up only in a CET enabled > > environment since build/test setup may not be on a CET capable > > hardware. > > Well, I'm not sure of the details of distro testing, but there are > plenty of TGL and later systems out there today. With kernel support, > I'm thinking these types of problems couldn't lurk for years like they > have. If this is the case, we would have nothing to worry about since the CET enabled applications won't pass validation if they aren't CET compatible. > > > > > planned without breaking things for existing binaries. We can have > > > both > > > by: > > > 1. Choosing a new bit, adding it to the tools, and never supporting > > > the > > > old bit in glibc. > > > 2. Providing the option to have the kernel block the old bit, so > > > upgraded users can decide what experience they would like. Then > > > distros > > > can find the problems and adjust their packages. I'm starting to > > > think > > > a default off sysctl toggle might be better than a Kconfig. > > > 3. Any other ideas? > > > > Don't enable CET in glibc until we can validate CET functionality. > > Can you elaborate on what you mean by this? Not upstream glibc CET > support? Or have users not enable it? If the latter, how would they > know about all these problems. The current glibc doesn't support CET. To enable CET in an application, one should validate it together with the CET enabled glibc under the CET enabled kernel on a CET capable machine. > > And what is wrong with the cleanest option, number 1? The ABI document > can be updated. It doesn't help resolve any issues.
On Mon, 2022-11-07 at 13:47 -0800, H.J. Lu wrote: > On Mon, Nov 7, 2022 at 1:34 PM Edgecombe, Rick P > <rick.p.edgecombe@intel.com> wrote: > > > > On Mon, 2022-11-07 at 13:21 -0800, H.J. Lu wrote: > > > > > Some applications and libraries are compiled with -fcf- > > > > > protection, > > > > > but > > > > > they manipulate the stack in such a way that they aren't > > > > > compatible > > > > > with the shadow stack. However, if the build/test setup > > > > > doesn't > > > > > support > > > > > shadow stack, it is impossible to validate. > > > > > > > > > > > > > When we have everything in place, the problems would be much > > > > more > > > > obvious when distros started turning it on. But we can't turn > > > > it on > > > > as > > > > > > Not necessarily. The problem will show up only in a CET enabled > > > environment since build/test setup may not be on a CET capable > > > hardware. > > > > Well, I'm not sure of the details of distro testing, but there are > > plenty of TGL and later systems out there today. With kernel > > support, > > I'm thinking these types of problems couldn't lurk for years like > > they > > have. > > If this is the case, we would have nothing to worry about since the > CET > enabled applications won't pass validation if they aren't CET > compatible. Hmm, I think you couldn't have already forgotten the problem binaries are already shipped... > > > > > > > > planned without breaking things for existing binaries. We can > > > > have > > > > both > > > > by: > > > > 1. Choosing a new bit, adding it to the tools, and never > > > > supporting > > > > the > > > > old bit in glibc. > > > > 2. Providing the option to have the kernel block the old bit, > > > > so > > > > upgraded users can decide what experience they would like. Then > > > > distros > > > > can find the problems and adjust their packages. I'm starting > > > > to > > > > think > > > > a default off sysctl toggle might be better than a Kconfig. > > > > 3. Any other ideas? > > > > > > Don't enable CET in glibc until we can validate CET > > > functionality. > > > > Can you elaborate on what you mean by this? Not upstream glibc CET > > support? Or have users not enable it? If the latter, how would they > > know about all these problems. > > The current glibc doesn't support CET. To enable CET in an > application, > one should validate it together with the CET enabled glibc under the > CET > enabled kernel on a CET capable machine. Agreed that this is how it should have gone. > > > > > And what is wrong with the cleanest option, number 1? The ABI > > document > > can be updated. > > It doesn't help resolve any issues. Please read the coverletter if you are unsure of what issues this is trying to address. I should have put more in the commit log.
On Mon, Nov 7, 2022 at 2:47 PM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > On Mon, 2022-11-07 at 13:47 -0800, H.J. Lu wrote: > > On Mon, Nov 7, 2022 at 1:34 PM Edgecombe, Rick P > > <rick.p.edgecombe@intel.com> wrote: > > > > > > On Mon, 2022-11-07 at 13:21 -0800, H.J. Lu wrote: > > > > > > Some applications and libraries are compiled with -fcf- > > > > > > protection, > > > > > > but > > > > > > they manipulate the stack in such a way that they aren't > > > > > > compatible > > > > > > with the shadow stack. However, if the build/test setup > > > > > > doesn't > > > > > > support > > > > > > shadow stack, it is impossible to validate. > > > > > > > > > > > > > > > > When we have everything in place, the problems would be much > > > > > more > > > > > obvious when distros started turning it on. But we can't turn > > > > > it on > > > > > as > > > > > > > > Not necessarily. The problem will show up only in a CET enabled > > > > environment since build/test setup may not be on a CET capable > > > > hardware. > > > > > > Well, I'm not sure of the details of distro testing, but there are > > > plenty of TGL and later systems out there today. With kernel > > > support, > > > I'm thinking these types of problems couldn't lurk for years like > > > they > > > have. > > > > If this is the case, we would have nothing to worry about since the > > CET > > enabled applications won't pass validation if they aren't CET > > compatible. > > Hmm, I think you couldn't have already forgotten the problem binaries > are already shipped... It should be OK since glibc doesn't support CET. > > > > > > > > > > > planned without breaking things for existing binaries. We can > > > > > have > > > > > both > > > > > by: > > > > > 1. Choosing a new bit, adding it to the tools, and never > > > > > supporting > > > > > the > > > > > old bit in glibc. > > > > > 2. Providing the option to have the kernel block the old bit, > > > > > so > > > > > upgraded users can decide what experience they would like. Then > > > > > distros > > > > > can find the problems and adjust their packages. I'm starting > > > > > to > > > > > think > > > > > a default off sysctl toggle might be better than a Kconfig. > > > > > 3. Any other ideas? > > > > > > > > Don't enable CET in glibc until we can validate CET > > > > functionality. > > > > > > Can you elaborate on what you mean by this? Not upstream glibc CET > > > support? Or have users not enable it? If the latter, how would they > > > know about all these problems. > > > > The current glibc doesn't support CET. To enable CET in an > > application, > > one should validate it together with the CET enabled glibc under the > > CET > > enabled kernel on a CET capable machine. > > Agreed that this is how it should have gone. > > > > > > > > > And what is wrong with the cleanest option, number 1? The ABI > > > document > > > can be updated. > > > > It doesn't help resolve any issues. > > Please read the coverletter if you are unsure of what issues this is > trying to address. I should have put more in the commit log. > > >
* Rick P. Edgecombe: > When we have everything in place, the problems would be much more > obvious when distros started turning it on. But we can't turn it on as > planned without breaking things for existing binaries. We can have both > by: > 1. Choosing a new bit, adding it to the tools, and never supporting the > old bit in glibc. > 2. Providing the option to have the kernel block the old bit, so > upgraded users can decide what experience they would like. Then distros > can find the problems and adjust their packages. I'm starting to think > a default off sysctl toggle might be better than a Kconfig. > 3. Any other ideas? This problem is fairly common nowadays for new system calls. Before glibc can use them internally, we need to port userspace first, otherwise key applications fail to work. Yet we do not require ELF markup to make the new system call available to glibc. The situation here seems similar: before deploying a new glibc, we need to upgrade parts of userspace. Thanks, Florian
diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h index 97932fbf973d..1aa76ed02dda 100644 --- a/arch/arm64/include/asm/elf.h +++ b/arch/arm64/include/asm/elf.h @@ -279,6 +279,11 @@ static inline int arch_parse_elf_property(u32 type, const void *data, return 0; } +static inline int arch_process_elf_property(struct arch_elf_state *arch) +{ + return 0; +} + static inline int arch_elf_pt_proc(void *ehdr, void *phdr, struct file *f, bool is_interp, struct arch_elf_state *state) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index f3d14f5accce..da9e43aa91a3 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -28,6 +28,7 @@ config X86_64 select ARCH_HAS_GIGANTIC_PAGE select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 select ARCH_USE_CMPXCHG_LOCKREF + select ARCH_USE_GNU_PROPERTY select HAVE_ARCH_SOFT_DIRTY select MODULES_USE_ELF_RELA select NEED_DMA_MAP_STATE @@ -60,6 +61,7 @@ config X86 select ACPI_LEGACY_TABLES_LOOKUP if ACPI select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI select ARCH_32BIT_OFF_T if X86_32 + select ARCH_BINFMT_ELF_STATE select ARCH_CLOCKSOURCE_INIT select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE select ARCH_ENABLE_HUGEPAGE_MIGRATION if X86_64 && HUGETLB_PAGE && MIGRATION @@ -1977,6 +1979,17 @@ config X86_USER_SHADOW_STACK If unsure, say N. +config X86_USER_SHADOW_STACK_ALLOW_BROKEN + bool "Allow enabling shadow stack for broken binaries" + depends on EXPERT + depends on X86_USER_SHADOW_STACK + help + There exist old binaries that are marked as compatible with shadow + stack, but actually aren't. The kernel blocks these binaries from + getting shadow stack enabled by default. But some working binaries + are also blocked. Select this option if you would like to allow these + binaries to run with shadow stack, and possibly crash. + config EFI bool "EFI runtime service support" depends on ACPI diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h index 098e4ecfdf9b..7f0cabb3db21 100644 --- a/arch/x86/include/asm/cet.h +++ b/arch/x86/include/asm/cet.h @@ -22,6 +22,7 @@ int shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags, void shstk_free(struct task_struct *p); int setup_signal_shadow_stack(struct ksignal *ksig); int restore_signal_shadow_stack(void); +void bad_cet_binary_disable(bool disable); #else static inline long cet_prctl(struct task_struct *task, int option, unsigned long features) { return -EINVAL; } @@ -33,6 +34,7 @@ static inline int shstk_alloc_thread_stack(struct task_struct *p, 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; } +static inline void bad_cet_binary_disable(bool disable) {}; #endif /* CONFIG_X86_USER_SHADOW_STACK */ #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h index cb0ff1055ab1..95ee133acffb 100644 --- a/arch/x86/include/asm/elf.h +++ b/arch/x86/include/asm/elf.h @@ -383,6 +383,17 @@ extern int compat_arch_setup_additional_pages(struct linux_binprm *bprm, extern bool arch_syscall_is_vdso_sigreturn(struct pt_regs *regs); +struct arch_elf_state { + unsigned int gnu_property; +}; + +#define INIT_ARCH_ELF_STATE { \ + .gnu_property = 0, \ +} + +#define arch_elf_pt_proc(ehdr, phdr, elf, interp, state) (0) +#define arch_check_elf(ehdr, interp, interp_ehdr, state) (0) + /* Do not change the values. See get_align_mask() */ enum align_flags { ALIGN_VA_32 = BIT(0), diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index a6c414dfd10f..4b333c801010 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -534,6 +534,7 @@ struct thread_struct { #ifdef CONFIG_X86_USER_SHADOW_STACK unsigned long features; unsigned long features_locked; + bool bad_cet_binary_disable; struct thread_shstk shstk; #endif diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index 03bc16c9cc19..461b8e9468df 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c @@ -867,3 +867,36 @@ unsigned long KSTK_ESP(struct task_struct *task) { return task_pt_regs(task)->sp; } + +#ifdef CONFIG_X86_USER_SHADOW_STACK +int arch_parse_elf_property(u32 type, const void *data, size_t datasz, + bool compat, struct arch_elf_state *state) +{ + if (type != GNU_PROPERTY_X86_FEATURE_1_AND) + return 0; + + if (datasz != sizeof(unsigned int)) + return -ENOEXEC; + + state->gnu_property = *(unsigned int *)data; + return 0; +} + +int arch_process_elf_property(struct arch_elf_state *state) +{ + bad_cet_binary_disable(state->gnu_property & GNU_PROPERTY_X86_FEATURE_1_BAD); + return 0; +} +#else /* CONFIG_X86_USER_SHADOW_STACK */ +int arch_parse_elf_property(u32 type, const void *data, size_t datasz, + bool compat, struct arch_elf_state *state) +{ + return 0; +} + +int arch_process_elf_property(struct arch_elf_state *state) +{ + return 0; +} +#endif /* CONFIG_X86_USER_SHADOW_STACK */ + diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c index bed7032d35f2..cb105e69c840 100644 --- a/arch/x86/kernel/shstk.c +++ b/arch/x86/kernel/shstk.c @@ -445,6 +445,9 @@ SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsi long cet_prctl(struct task_struct *task, int option, unsigned long features) { + if (task->thread.bad_cet_binary_disable) + return -EINVAL; + if (option == ARCH_CET_LOCK) { task->thread.features_locked |= features; return 0; @@ -482,3 +485,15 @@ long cet_prctl(struct task_struct *task, int option, unsigned long features) return wrss_control(true); return -EINVAL; } + +#ifdef CONFIG_X86_USER_SHADOW_STACK_ALLOW_BROKEN +void bad_cet_binary_disable(bool disable) +{ + current->thread.bad_cet_binary_disable = false; +} +#else /* CONFIG_X86_USER_SHADOW_STACK_ALLOW_BROKEN */ +void bad_cet_binary_disable(bool disable) +{ + current->thread.bad_cet_binary_disable = disable; +} +#endif /* CONFIG_X86_USER_SHADOW_STACK_ALLOW_BROKEN */ diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 6a11025e5850..8b6ae5e423fb 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -764,6 +764,8 @@ static int parse_elf_property(const char *data, size_t *off, size_t datasz, #define GNU_PROPERTY_TYPE_0_NAME "GNU" #define NOTE_NAME_SZ (sizeof(GNU_PROPERTY_TYPE_0_NAME)) + + static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr, struct arch_elf_state *arch) { @@ -821,6 +823,18 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr, return ret == -ENOENT ? 0 : ret; } +static int check_elf_properties(struct file *f, const struct elf_phdr *phdr) +{ + struct arch_elf_state arch_state = INIT_ARCH_ELF_STATE; + int retval; + + retval = parse_elf_properties(f, phdr, &arch_state); + if (retval) + return retval; + + return arch_process_elf_property(&arch_state); +} + static int load_elf_binary(struct linux_binprm *bprm) { struct file *interpreter = NULL; /* to shut gcc up */ @@ -920,13 +934,21 @@ static int load_elf_binary(struct linux_binprm *bprm) if (retval < 0) goto out_free_dentry; - break; + /* Quit if already found PT_GNU_PROPERTY */ + if (elf_property_phdata) + break; + + continue; out_free_interp: kfree(elf_interpreter); goto out_free_ph; } + retval = check_elf_properties(bprm->file, elf_property_phdata); + if (retval) + return retval; + elf_ppnt = elf_phdata; for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) switch (elf_ppnt->p_type) { diff --git a/include/linux/elf.h b/include/linux/elf.h index c9a46c4e183b..faf961b92a95 100644 --- a/include/linux/elf.h +++ b/include/linux/elf.h @@ -92,9 +92,15 @@ static inline int arch_parse_elf_property(u32 type, const void *data, { return 0; } + +static inline int arch_process_elf_property(struct arch_elf_state *arch) +{ + return 0; +} #else extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz, bool compat, struct arch_elf_state *arch); +extern int arch_process_elf_property(struct arch_elf_state *arch); #endif #ifdef CONFIG_ARCH_HAVE_ELF_PROT diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index 11089731e2e9..d9b58adce321 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -469,4 +469,19 @@ typedef struct elf64_note { /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */ #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) +/* + * See the x86 64 psABI at: + * https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/x86-64-psABI + * .note.gnu.property types for x86: + */ +/* 0xc0000000 and 0xc0000001 are reserved */ +#define GNU_PROPERTY_X86_FEATURE_1_AND 0xc0000002 + +/* Bits for GNU_PROPERTY_X86_FEATURE_1_AND */ +#define GNU_PROPERTY_X86_FEATURE_1_IBT 0x00000001 +#define GNU_PROPERTY_X86_FEATURE_1_SHSTK 0x00000002 + +#define GNU_PROPERTY_X86_FEATURE_1_BAD (GNU_PROPERTY_X86_FEATURE_1_IBT | \ + GNU_PROPERTY_X86_FEATURE_1_SHSTK) + #endif /* _UAPI_LINUX_ELF_H */