Re: [PATCH v2 23/39] x86: Introduce userspace API for CET enabling
From: Kees Cook <hidden>
Date: 2022-10-03 19:02:13
Also in:
linux-arch, linux-doc, linux-mm, lkml
On Thu, Sep 29, 2022 at 03:29:20PM -0700, Rick Edgecombe wrote:
From: "Kirill A. Shutemov" <redacted> Add three new arch_prctl() handles: - ARCH_CET_ENABLE/DISABLE enables or disables the specified feature. Returns 0 on success or an error. - ARCH_CET_LOCK prevents future disabling or enabling of the specified feature. Returns 0 on success or an error The features are handled per-thread and inherited over fork(2)/clone(2), but reset on exec(). This is preparation patch. It does not impelement any features.
typo: "implement"
quoted hunk ↗ jump to hunk
Signed-off-by: Kirill A. Shutemov <redacted> [tweaked with feedback from tglx] Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v2: - Only allow one enable/disable per call (tglx) - Return error code like a normal arch_prctl() (Alexander Potapenko) - Make CET only (tglx) arch/x86/include/asm/cet.h | 20 ++++++++++++++++ arch/x86/include/asm/processor.h | 3 +++ arch/x86/include/uapi/asm/prctl.h | 6 +++++ arch/x86/kernel/process.c | 4 ++++ arch/x86/kernel/process_64.c | 5 +++- arch/x86/kernel/shstk.c | 38 +++++++++++++++++++++++++++++++ 6 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 arch/x86/include/asm/cet.h create mode 100644 arch/x86/kernel/shstk.cdiff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h new file mode 100644 index 000000000000..0fa4dbc98c49 --- /dev/null +++ b/arch/x86/include/asm/cet.h@@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_X86_CET_H +#define _ASM_X86_CET_H + +#ifndef __ASSEMBLY__ +#include <linux/types.h> + +struct task_struct; + +#ifdef CONFIG_X86_SHADOW_STACK +long cet_prctl(struct task_struct *task, int option, + unsigned long features); +#else +static inline long cet_prctl(struct task_struct *task, int option, + unsigned long features) { return -EINVAL; } +#endif /* CONFIG_X86_SHADOW_STACK */ + +#endif /* __ASSEMBLY__ */ + +#endif /* _ASM_X86_CET_H */diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 356308c73951..a92bf76edafe 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h@@ -530,6 +530,9 @@ struct thread_struct { */ u32 pkru; + unsigned long features; + unsigned long features_locked;
Should these be wrapped in #ifdef CONFIG_X86_SHADOW_STACK (or CONFIG_X86_CET) ? Also, just named "features"? Is this expected to be more than CET?
quoted hunk ↗ jump to hunk
+ /* Floating point and extended processor state */ struct fpu fpu; /*diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h index 500b96e71f18..028158e35269 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_CET_ENABLE 0x4001 +#define ARCH_CET_DISABLE 0x4002 +#define ARCH_CET_LOCK 0x4003 + #endif /* _ASM_X86_PRCTL_H */diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index 58a6ea472db9..034880311e6b 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c@@ -367,6 +367,10 @@ void arch_setup_new_exec(void) task_clear_spec_ssb_noexec(current); speculation_ctrl_update(read_thread_flags()); } + + /* Reset thread features on exec */ + current->thread.features = 0; + current->thread.features_locked = 0;
Same ifdef question here.
quoted hunk ↗ jump to hunk
} #ifdef CONFIG_X86_IOPL_IOPERMdiff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index 1962008fe743..8fa2c2b7de65 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c@@ -829,7 +829,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_CET_ENABLE: + case ARCH_CET_DISABLE: + case ARCH_CET_LOCK: + return cet_prctl(task, option, arg2); default: ret = -EINVAL; break;
I remain annoyed that prctl interfaces didn't use -ENOTSUP for "unknown option". :P
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c new file mode 100644 index 000000000000..e3276ac9e9b9 --- /dev/null +++ b/arch/x86/kernel/shstk.c
I think the Makefile addition should be moved from "x86/cet/shstk: Add user-mode shadow stack support" to here, yes? Otherwise, there is a bisectability randconfig-with-CONFIG_X86_SHADOW_STACK risk here (nothing will implement "cet_prctl").
quoted hunk ↗ jump to hunk
@@ -0,0 +1,38 @@ +// 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> + +long cet_prctl(struct task_struct *task, int option, unsigned long features) +{ + if (option == ARCH_CET_LOCK) { + task->thread.features_locked |= features; + return 0; + } + + /* Don't allow via ptrace */ + if (task != current) + return -EINVAL;
... but locking _is_ allowed via ptrace? If that intended, it should be explicitly mentioned in the commit log and in a comment here. Also, perhaps -ESRCH ?
+ + /* 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;
Perhaps -E2BIG ?
+ if (option == ARCH_CET_DISABLE) {
+ return -EINVAL;
+ }
+
+ /* Handle ARCH_CET_ENABLE */
+ return -EINVAL;
+}
--
2.17.1-- Kees Cook