On Thu, Sep 29, 2022 at 03:29:20PM -0700, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
>
> 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"
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> [tweaked with feedback from tglx]
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
>
> ---
>
> 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.c
>
> diff --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?
> +
> /* 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.
> }
>
> #ifdef CONFIG_X86_IOPL_IOPERM
> diff --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
> 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").
> @@ -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@xxxxxxxxx>
> + */
> +
> +#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
