Re: [PATCH v2 23/39] x86: Introduce userspace API for CET enabling

[Kees Cook <keescook@chromium.org>]

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