| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202210031141.0E0DE2CAEE@keescook>
Date: Mon, 3 Oct 2022 12:01:59 -0700
From: Kees Cook <keescook@...omium.org>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc: x86@...nel.org, "H . Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-mm@...ck.org,
linux-arch@...r.kernel.org, linux-api@...r.kernel.org,
Arnd Bergmann <arnd@...db.de>,
Andy Lutomirski <luto@...nel.org>,
Balbir Singh <bsingharora@...il.com>,
Borislav Petkov <bp@...en8.de>,
Cyrill Gorcunov <gorcunov@...il.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Eugene Syromiatnikov <esyr@...hat.com>,
Florian Weimer <fweimer@...hat.com>,
"H . J . Lu" <hjl.tools@...il.com>, Jann Horn <jannh@...gle.com>,
Jonathan Corbet <corbet@....net>,
Mike Kravetz <mike.kravetz@...cle.com>,
Nadav Amit <nadav.amit@...il.com>,
Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
Peter Zijlstra <peterz@...radead.org>,
Randy Dunlap <rdunlap@...radead.org>,
"Ravi V . Shankar" <ravi.v.shankar@...el.com>,
Weijiang Yang <weijiang.yang@...el.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
joao.moreira@...el.com, John Allen <john.allen@....com>,
kcc@...gle.com, eranian@...gle.com, rppt@...nel.org,
jamorris@...ux.microsoft.com, dethoma@...rosoft.com
Subject: Re: [PATCH v2 23/39] x86: Introduce userspace API for CET enabling
On Thu, Sep 29, 2022 at 03:29:20PM -0700, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
>
> 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@...ux.intel.com>
> [tweaked with feedback from tglx]
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.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.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@...el.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
Powered by blists - more mailing lists