| Message ID | 20210127212524.10188-25-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Shadow Stack | expand |
On 1/27/21 1:25 PM, Yu-cheng Yu wrote: > arch_prctl(ARCH_X86_CET_STATUS, u64 *args) > Get CET feature status. > > The parameter 'args' is a pointer to a user buffer. The kernel returns > the following information: > > *args = shadow stack/IBT status > *(args + 1) = shadow stack base address > *(args + 2) = shadow stack size What's the deal for 32-bit binaries? The in-kernel code looks 64-bit only, but I don't see anything restricting the interface to 64-bit. > +static int copy_status_to_user(struct cet_status *cet, u64 arg2) This has static scope, but it's still awfully generically named. A cet_ prefix would be nice. > +{ > + u64 buf[3] = {0, 0, 0}; > + > + if (cet->shstk_size) { > + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_SHSTK; > + buf[1] = (u64)cet->shstk_base; > + buf[2] = (u64)cet->shstk_size; What's the casting for? > + } > + > + return copy_to_user((u64 __user *)arg2, buf, sizeof(buf)); > +} > + > +int prctl_cet(int option, u64 arg2) > +{ > + struct cet_status *cet; > + unsigned int features; > + > + /* > + * GLIBC's ENOTSUPP == EOPNOTSUPP == 95, and it does not recognize > + * the kernel's ENOTSUPP (524). So return EOPNOTSUPP here. > + */ > + if (!IS_ENABLED(CONFIG_X86_CET)) > + return -EOPNOTSUPP; Let's ignore glibc for a moment. What error code *should* the kernel be returning here? errno(3) says: EOPNOTSUPP Operation not supported on socket (POSIX.1) ... ENOTSUP Operation not supported (POSIX.1) > + cet = ¤t->thread.cet; > + > + if (option == ARCH_X86_CET_STATUS) > + return copy_status_to_user(cet, arg2); What's the point of doing copy_status_to_user() if the processor doesn't support CET? In other words, shouldn't this be below the CPU feature check? Also, please cast arg2 *here*. It becomes a user pointer here, not at the copy_to_user(). > + if (!static_cpu_has(X86_FEATURE_CET)) > + return -EOPNOTSUPP; So, you went to the trouble of adding a disabled-features.h entry for this. Why not just do: if (cpu_feature_enabled(X86_FEATURE_CET)) ... instead of the IS_ENABLED() check above? That should get rid of one of these if's. > + switch (option) { > + case ARCH_X86_CET_DISABLE: > + if (cet->locked) > + return -EPERM; > + > + features = (unsigned int)arg2; What's the purpose of this cast? > + if (features & ~GNU_PROPERTY_X86_FEATURE_1_VALID) > + return -EINVAL; > + if (features & GNU_PROPERTY_X86_FEATURE_1_SHSTK) > + cet_disable_shstk(); > + return 0; This doesn't enforce that the high bits of arg2 be 0. Shouldn't we call them reserved and enforce that they be 0? > + case ARCH_X86_CET_LOCK: > + cet->locked = 1; > + return 0; This needs to check for and enforce that arg2==0. > + default: > + return -ENOSYS; > + } > +}
On 1/29/2021 9:07 AM, Dave Hansen wrote: > On 1/27/21 1:25 PM, Yu-cheng Yu wrote: >> arch_prctl(ARCH_X86_CET_STATUS, u64 *args) >> Get CET feature status. >> >> The parameter 'args' is a pointer to a user buffer. The kernel returns >> the following information: >> >> *args = shadow stack/IBT status >> *(args + 1) = shadow stack base address >> *(args + 2) = shadow stack size > > What's the deal for 32-bit binaries? The in-kernel code looks 64-bit > only, but I don't see anything restricting the interface to 64-bit. Items in args are 64-bit. A 32-bit binary uses the same interface, but uses only lower bits. I will add that in the comments. >> +static int copy_status_to_user(struct cet_status *cet, u64 arg2) > > This has static scope, but it's still awfully generically named. A cet_ > prefix would be nice. I will add that. >> +{ >> + u64 buf[3] = {0, 0, 0}; >> + >> + if (cet->shstk_size) { >> + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_SHSTK; >> + buf[1] = (u64)cet->shstk_base; >> + buf[2] = (u64)cet->shstk_size; > > What's the casting for? cet->shstk_base and cet->shstk_size are both 'unsigned long', not u64, so the cast. >> + } >> + >> + return copy_to_user((u64 __user *)arg2, buf, sizeof(buf)); >> +} >> + >> +int prctl_cet(int option, u64 arg2) >> +{ >> + struct cet_status *cet; >> + unsigned int features; >> + >> + /* >> + * GLIBC's ENOTSUPP == EOPNOTSUPP == 95, and it does not recognize >> + * the kernel's ENOTSUPP (524). So return EOPNOTSUPP here. >> + */ >> + if (!IS_ENABLED(CONFIG_X86_CET)) >> + return -EOPNOTSUPP; > > Let's ignore glibc for a moment. What error code *should* the kernel be > returning here? errno(3) says: > > EOPNOTSUPP Operation not supported on socket (POSIX.1) > ... > ENOTSUP Operation not supported (POSIX.1) > Yeah, other places in kernel use ENOTSUPP. This seems to be out of line. And since the issue is long-existing, applications already know how to deal with it. I should have made that argument. Change it to ENOTSUPP. >> + cet = ¤t->thread.cet; >> + >> + if (option == ARCH_X86_CET_STATUS) >> + return copy_status_to_user(cet, arg2); > > What's the point of doing copy_status_to_user() if the processor doesn't > support CET? In other words, shouldn't this be below the CPU feature check? The thought was to tell the difference between the kernel itself does not support CET and the system does not have CET. And, if the kernel supports it, show CET status of the thread. > Also, please cast arg2 *here*. It becomes a user pointer here, not at > the copy_to_user(). I will fix it. >> + if (!static_cpu_has(X86_FEATURE_CET)) >> + return -EOPNOTSUPP; > > So, you went to the trouble of adding a disabled-features.h entry for > this. Why not just do: > > if (cpu_feature_enabled(X86_FEATURE_CET)) > ... > > instead of the IS_ENABLED() check above? That should get rid of one of > these if's. > Explained above. >> + switch (option) { >> + case ARCH_X86_CET_DISABLE: >> + if (cet->locked) >> + return -EPERM; >> + >> + features = (unsigned int)arg2; > > What's the purpose of this cast? > >> + if (features & ~GNU_PROPERTY_X86_FEATURE_1_VALID) >> + return -EINVAL; >> + if (features & GNU_PROPERTY_X86_FEATURE_1_SHSTK) >> + cet_disable_shstk(); >> + return 0; > > This doesn't enforce that the high bits of arg2 be 0. Shouldn't we call > them reserved and enforce that they be 0? Yes, the code already checks invalid bits. We don't need the cast. >> + case ARCH_X86_CET_LOCK: >> + cet->locked = 1; >> + return 0; > > This needs to check for and enforce that arg2==0. Yes. > >> + default: >> + return -ENOSYS; >> + } >> +}
On 1/29/21 10:56 AM, Yu, Yu-cheng wrote: > On 1/29/2021 9:07 AM, Dave Hansen wrote: >> On 1/27/21 1:25 PM, Yu-cheng Yu wrote: >>> + u64 buf[3] = {0, 0, 0}; Doesn't the compiler zero these if you initialize it to anything? In other words, doesn't this work? u64 buf[3] = {}; >>> + if (cet->shstk_size) { >>> + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_SHSTK; >>> + buf[1] = (u64)cet->shstk_base; >>> + buf[2] = (u64)cet->shstk_size; >> >> What's the casting for? > > cet->shstk_base and cet->shstk_size are both 'unsigned long', not u64, > so the cast. Sure, but we don't put explicit casts at every implicit type conversion in the kernel. What function does this casting serve? >>> + cet = ¤t->thread.cet; >>> + >>> + if (option == ARCH_X86_CET_STATUS) >>> + return copy_status_to_user(cet, arg2); >> >> What's the point of doing copy_status_to_user() if the processor doesn't >> support CET? In other words, shouldn't this be below the CPU feature >> check? > > The thought was to tell the difference between the kernel itself does > not support CET and the system does not have CET. And, if the kernel > supports it, show CET status of the thread. Why would that matter to userspace? If they want to know if the processor has CET support there are existing ways to do it. I don't think this should be part of the ABI.
On 1/29/2021 11:15 AM, Dave Hansen wrote: > On 1/29/21 10:56 AM, Yu, Yu-cheng wrote: >> On 1/29/2021 9:07 AM, Dave Hansen wrote: >>> On 1/27/21 1:25 PM, Yu-cheng Yu wrote: [...] >>> What's the point of doing copy_status_to_user() if the processor doesn't >>> support CET? In other words, shouldn't this be below the CPU feature >>> check? >> >> The thought was to tell the difference between the kernel itself does >> not support CET and the system does not have CET. And, if the kernel >> supports it, show CET status of the thread. > > Why would that matter to userspace? > > If they want to know if the processor has CET support there are existing > ways to do it. I don't think this should be part of the ABI. > Ok, I will make it: if (!cpu_feature_enabled(X86_FEATURE_CET)) ...
On 1/29/2021 10:56 AM, Yu, Yu-cheng wrote: > On 1/29/2021 9:07 AM, Dave Hansen wrote: >> On 1/27/21 1:25 PM, Yu-cheng Yu wrote: >>> arch_prctl(ARCH_X86_CET_STATUS, u64 *args) >>> Get CET feature status. >>> >>> The parameter 'args' is a pointer to a user buffer. The kernel >>> returns >>> the following information: >>> >>> *args = shadow stack/IBT status >>> *(args + 1) = shadow stack base address >>> *(args + 2) = shadow stack size [...] >>> +int prctl_cet(int option, u64 arg2) >>> +{ >>> + struct cet_status *cet; >>> + unsigned int features; >>> + >>> + /* >>> + * GLIBC's ENOTSUPP == EOPNOTSUPP == 95, and it does not recognize >>> + * the kernel's ENOTSUPP (524). So return EOPNOTSUPP here. >>> + */ >>> + if (!IS_ENABLED(CONFIG_X86_CET)) >>> + return -EOPNOTSUPP; >> >> Let's ignore glibc for a moment. What error code *should* the kernel be >> returning here? errno(3) says: >> >> EOPNOTSUPP Operation not supported on socket (POSIX.1) >> ... >> ENOTSUP Operation not supported (POSIX.1) >> > > Yeah, other places in kernel use ENOTSUPP. This seems to be out of > line. And since the issue is long-existing, applications already know > how to deal with it. I should have made that argument. Change it to > ENOTSUPP. When I make the change, checkpatch says... WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #128: FILE: arch/x86/kernel/cet_prctl.c:33: + return -ENOTSUPP; Do we want to reconsider? [...]
On 2/3/21 1:54 PM, Yu, Yu-cheng wrote: > On 1/29/2021 10:56 AM, Yu, Yu-cheng wrote: >> On 1/29/2021 9:07 AM, Dave Hansen wrote: >>> On 1/27/21 1:25 PM, Yu-cheng Yu wrote: >>>> + if (!IS_ENABLED(CONFIG_X86_CET)) >>>> + return -EOPNOTSUPP; >>> >>> Let's ignore glibc for a moment. What error code *should* the kernel be >>> returning here? errno(3) says: >>> >>> EOPNOTSUPP Operation not supported on socket (POSIX.1) >>> ... >>> ENOTSUP Operation not supported (POSIX.1) >>> >> >> Yeah, other places in kernel use ENOTSUPP. This seems to be out of >> line. And since the issue is long-existing, applications already know >> how to deal with it. I should have made that argument. Change it to >> ENOTSUPP. > > When I make the change, checkpatch says... > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > #128: FILE: arch/x86/kernel/cet_prctl.c:33: > + return -ENOTSUPP; > > Do we want to reconsider? I'm not sure I trust checkpatch over manpages. I had to google "SUSV4". I'm not sure it matters at *all* for a 100% Linux-specific interface. ENOTSUPP does seem less popular lately: > $ git diff v5.0.. kernel/ arch/ drivers/ | grep ^+.*return.*E.*NO.*SUP.*\; | grep -o -- -E.*\; | sort | uniq -c | sort -n > ... noise > 61 -EOPNOTSUPP); > 260 -ENOTSUPP; > 1577 -EOPNOTSUPP; but far from unused. That might be due to checkpatch spew more than anything.
On 2/3/2021 2:11 PM, Dave Hansen wrote: > On 2/3/21 1:54 PM, Yu, Yu-cheng wrote: >> On 1/29/2021 10:56 AM, Yu, Yu-cheng wrote: >>> On 1/29/2021 9:07 AM, Dave Hansen wrote: >>>> On 1/27/21 1:25 PM, Yu-cheng Yu wrote: >>>>> + if (!IS_ENABLED(CONFIG_X86_CET)) >>>>> + return -EOPNOTSUPP; >>>> >>>> Let's ignore glibc for a moment. What error code *should* the kernel be >>>> returning here? errno(3) says: >>>> >>>> EOPNOTSUPP Operation not supported on socket (POSIX.1) >>>> ... >>>> ENOTSUP Operation not supported (POSIX.1) >>>> >>> >>> Yeah, other places in kernel use ENOTSUPP. This seems to be out of >>> line. And since the issue is long-existing, applications already know >>> how to deal with it. I should have made that argument. Change it to >>> ENOTSUPP. >> >> When I make the change, checkpatch says... >> >> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP >> #128: FILE: arch/x86/kernel/cet_prctl.c:33: >> + return -ENOTSUPP; >> >> Do we want to reconsider? > > I'm not sure I trust checkpatch over manpages. I had to google "SUSV4". > I'm not sure it matters at *all* for a 100% Linux-specific interface. > > ENOTSUPP does seem less popular lately: > >> $ git diff v5.0.. kernel/ arch/ drivers/ | grep ^+.*return.*E.*NO.*SUP.*\; | grep -o -- -E.*\; | sort | uniq -c | sort -n >> ... noise >> 61 -EOPNOTSUPP); >> 260 -ENOTSUPP; >> 1577 -EOPNOTSUPP; > > but far from unused. That might be due to checkpatch spew more than > anything. > Maybe I will keep it ENOTSUPP for now. If any logical reason should come up, I will be happy to change it again. Thanks! -- Yu-cheng
diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h index ec4b5e62d0ce..16870e5bc8eb 100644 --- a/arch/x86/include/asm/cet.h +++ b/arch/x86/include/asm/cet.h @@ -14,9 +14,11 @@ struct sc_ext; struct cet_status { unsigned long shstk_base; unsigned long shstk_size; + unsigned int locked:1; }; #ifdef CONFIG_X86_CET +int prctl_cet(int option, u64 arg2); int cet_setup_shstk(void); int cet_setup_thread_shstk(struct task_struct *p, unsigned long clone_flags); void cet_disable_shstk(void); @@ -25,6 +27,7 @@ int cet_verify_rstor_token(bool ia32, unsigned long ssp, unsigned long *new_ssp) void cet_restore_signal(struct sc_ext *sc); int cet_setup_signal(bool ia32, unsigned long rstor, struct sc_ext *sc); #else +static inline int prctl_cet(int option, u64 arg2) { return -EINVAL; } static inline int cet_setup_thread_shstk(struct task_struct *p, unsigned long clone_flags) { return 0; } static inline void cet_disable_shstk(void) {} diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h index 5a6aac9fa41f..9245bf629120 100644 --- a/arch/x86/include/uapi/asm/prctl.h +++ b/arch/x86/include/uapi/asm/prctl.h @@ -14,4 +14,8 @@ #define ARCH_MAP_VDSO_32 0x2002 #define ARCH_MAP_VDSO_64 0x2003 +#define ARCH_X86_CET_STATUS 0x3001 +#define ARCH_X86_CET_DISABLE 0x3002 +#define ARCH_X86_CET_LOCK 0x3003 + #endif /* _ASM_X86_PRCTL_H */ diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 4a9a7e7d00dc..2f60a28769f9 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -151,7 +151,7 @@ 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_CET) += cet.o +obj-$(CONFIG_X86_CET) += cet.o cet_prctl.o ### # 64 bit specific files diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c new file mode 100644 index 000000000000..b26531608ae5 --- /dev/null +++ b/arch/x86/kernel/cet_prctl.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/errno.h> +#include <linux/uaccess.h> +#include <linux/prctl.h> +#include <linux/compat.h> +#include <linux/mman.h> +#include <linux/elfcore.h> +#include <linux/processor.h> +#include <asm/prctl.h> +#include <asm/cet.h> + +/* See Documentation/x86/intel_cet.rst. */ + +static int copy_status_to_user(struct cet_status *cet, u64 arg2) +{ + u64 buf[3] = {0, 0, 0}; + + if (cet->shstk_size) { + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_SHSTK; + buf[1] = (u64)cet->shstk_base; + buf[2] = (u64)cet->shstk_size; + } + + return copy_to_user((u64 __user *)arg2, buf, sizeof(buf)); +} + +int prctl_cet(int option, u64 arg2) +{ + struct cet_status *cet; + unsigned int features; + + /* + * GLIBC's ENOTSUPP == EOPNOTSUPP == 95, and it does not recognize + * the kernel's ENOTSUPP (524). So return EOPNOTSUPP here. + */ + if (!IS_ENABLED(CONFIG_X86_CET)) + return -EOPNOTSUPP; + + cet = ¤t->thread.cet; + + if (option == ARCH_X86_CET_STATUS) + return copy_status_to_user(cet, arg2); + + if (!static_cpu_has(X86_FEATURE_CET)) + return -EOPNOTSUPP; + + switch (option) { + case ARCH_X86_CET_DISABLE: + if (cet->locked) + return -EPERM; + + features = (unsigned int)arg2; + + if (features & ~GNU_PROPERTY_X86_FEATURE_1_VALID) + return -EINVAL; + if (features & GNU_PROPERTY_X86_FEATURE_1_SHSTK) + cet_disable_shstk(); + return 0; + + case ARCH_X86_CET_LOCK: + cet->locked = 1; + return 0; + + default: + return -ENOSYS; + } +} diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index 3af6b36e1a5c..9e11e5f589f3 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -979,14 +979,14 @@ unsigned long get_wchan(struct task_struct *p) } long do_arch_prctl_common(struct task_struct *task, int option, - unsigned long cpuid_enabled) + unsigned long arg2) { switch (option) { case ARCH_GET_CPUID: return get_cpuid_mode(); case ARCH_SET_CPUID: - return set_cpuid_mode(task, cpuid_enabled); + return set_cpuid_mode(task, arg2); } - return -EINVAL; + return prctl_cet(option, arg2); }
arch_prctl(ARCH_X86_CET_STATUS, u64 *args) Get CET feature status. The parameter 'args' is a pointer to a user buffer. The kernel returns the following information: *args = shadow stack/IBT status *(args + 1) = shadow stack base address *(args + 2) = shadow stack size arch_prctl(ARCH_X86_CET_DISABLE, unsigned int features) Disable CET features specified in 'features'. Return -EPERM if CET is locked. arch_prctl(ARCH_X86_CET_LOCK) Lock in CET features. Also change do_arch_prctl_common()'s parameter 'cpuid_enabled' to 'arg2', as it is now also passed to prctl_cet(). Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/include/asm/cet.h | 3 ++ arch/x86/include/uapi/asm/prctl.h | 4 ++ arch/x86/kernel/Makefile | 2 +- arch/x86/kernel/cet_prctl.c | 68 +++++++++++++++++++++++++++++++ arch/x86/kernel/process.c | 6 +-- 5 files changed, 79 insertions(+), 4 deletions(-) create mode 100644 arch/x86/kernel/cet_prctl.c