CC Mike about ptrace/CRIU question.
On Mon, 2022-10-03 at 12:01 -0700, Kees Cook wrote:
> 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"
Oops, thanks.
>
> >
> > 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?
Sigh, there have been many ideas about how this API and features
tracking could be shared with LAM. At some point there was some
discussion about LAM using the `features` as well, even if it had a
separate arch_prctl() interface. Just checking the last LAM posting,
I'm not sure it needs it. So yes, this could go back to being CET only
for the time being.
>
> > +
> > /* 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").
Oh, yep, good point.
>
> > @@ -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.
I believe CRIU needs to lock via ptrace as well. Maybe Mike can
confirm.
I can mention it.
>
> 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 ?
Ehh, I don't know. E2MUCH maybe. It's not necessarily too big. Like if
a third bit was added for IBT some day, you could set SHSTK and WRSS,
it would be invalid, but still be "less" than the valid passing of just
the (hypothetical IBT bit).
>
> > + if (option == ARCH_CET_DISABLE) {
> > + return -EINVAL;
> > + }
> > +
> > + /* Handle ARCH_CET_ENABLE */
> > + return -EINVAL;
> > +}
> > --
> > 2.17.1
> >
>
>
