lkml.org 
[lkml]   [2022]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@linux.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@linux.intel.com>
    > [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.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@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

    \
     
     \ /
      Last update: 2022-10-03 21:02    [W:4.340 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site