Linux-api Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-mm@kvack.org, linux-arch@vger.kernel.org,
	linux-api@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Andy Lutomirski <luto@kernel.org>,
	Balbir Singh <bsingharora@gmail.com>,
	Borislav Petkov <bp@alien8.de>,
	Cyrill Gorcunov <gorcunov@gmail.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Eugene Syromiatnikov <esyr@redhat.com>,
	Florian Weimer <fweimer@redhat.com>,
	"H.J. Lu" <hjl.tools@gmail.com>, Jann Horn <jannh@google.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Nadav Amit <nadav.amit@gmail.com>,
	Oleg Nesterov <oleg@redhat.com>, Pavel Machek <pavel@ucw.cz>,
	Peter Zijlstra <peterz@infradead.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Vedvyas Shanbhogue <vedvyas.shanbhogue@intel.com>,
	Dave Martin <Dave.Martin@arm.com>,
	Weijiang Yang <weijiang.yang@intel.com>
Subject: Re: [PATCH v10 26/26] x86/cet/shstk: Add arch_prctl functions for shadow stack
Date: Thu, 21 May 2020 15:42:07 -0700
Message-ID: <202005211528.A12B4AD@keescook> (raw)
In-Reply-To: <20200429220732.31602-27-yu-cheng.yu@intel.com>

On Wed, Apr 29, 2020 at 03:07:32PM -0700, Yu-cheng Yu wrote:
> arch_prctl(ARCH_X86_CET_STATUS, u64 *args)
>     Get CET feature status.
> 
>     The parameter 'args' is a pointer to a user buffer.  The kernel returns
>     the following information:
> 
>     *args = shadow stack/IBT status
>     *(args + 1) = shadow stack base address
>     *(args + 2) = shadow stack size
> 
> arch_prctl(ARCH_X86_CET_DISABLE, u64 features)
>     Disable CET features specified in 'features'.  Return -EPERM if CET is
>     locked.
> 
> arch_prctl(ARCH_X86_CET_LOCK)
>     Lock in CET features.
> 
> arch_prctl(ARCH_X86_CET_ALLOC_SHSTK, u64 *args)
>     Allocate a new shadow stack.
> 
>     The parameter 'args' is a pointer to a user buffer containing the
>     desired size to allocate.  The kernel returns the allocated shadow
>     stack address in *args.

Hi! Just a quick note about getting these designs right -- prctl() (and
similar APIs) needs to make sure they're examining all "unknown" flags
as zero, or we run the risk of breaking sloppy userspace callers who
accidentally set flags and then later the kernel gives meaning to those
flags. Notes below...

> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> v10:
> - Verify CET is enabled before handling arch_prctl.
> - Change input parameters from unsigned long to u64, to make it clear they
>   are 64-bit.
> 
>  arch/x86/include/asm/cet.h        |  4 ++
>  arch/x86/include/uapi/asm/prctl.h |  5 ++
>  arch/x86/kernel/Makefile          |  2 +-
>  arch/x86/kernel/cet.c             | 29 +++++++++++
>  arch/x86/kernel/cet_prctl.c       | 87 +++++++++++++++++++++++++++++++
>  arch/x86/kernel/process.c         |  4 +-
>  6 files changed, 128 insertions(+), 3 deletions(-)
>  create mode 100644 arch/x86/kernel/cet_prctl.c
> 
> diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
> index 71dc92acd2f2..99e6e741d28c 100644
> --- a/arch/x86/include/asm/cet.h
> +++ b/arch/x86/include/asm/cet.h
> @@ -14,16 +14,20 @@ struct sc_ext;
>  struct cet_status {
>  	unsigned long	shstk_base;
>  	unsigned long	shstk_size;
> +	unsigned int	locked:1;
>  };
>  
>  #ifdef CONFIG_X86_INTEL_CET
> +int prctl_cet(int option, u64 arg2);
>  int cet_setup_shstk(void);
>  int cet_setup_thread_shstk(struct task_struct *p);
> +int cet_alloc_shstk(unsigned long *arg);
>  void cet_disable_free_shstk(struct task_struct *p);
>  int cet_verify_rstor_token(bool ia32, unsigned long ssp, unsigned long *new_ssp);
>  void cet_restore_signal(struct sc_ext *sc);
>  int cet_setup_signal(bool ia32, unsigned long rstor, struct sc_ext *sc);
>  #else
> +static inline int prctl_cet(int option, u64 arg2) { return -EINVAL; }
>  static inline int cet_setup_thread_shstk(struct task_struct *p) { return 0; }
>  static inline void cet_disable_free_shstk(struct task_struct *p) {}
>  static inline void cet_restore_signal(struct sc_ext *sc) { return; }
> diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
> index 5a6aac9fa41f..d962f0ec9ccf 100644
> --- a/arch/x86/include/uapi/asm/prctl.h
> +++ b/arch/x86/include/uapi/asm/prctl.h
> @@ -14,4 +14,9 @@
>  #define ARCH_MAP_VDSO_32	0x2002
>  #define ARCH_MAP_VDSO_64	0x2003
>  
> +#define ARCH_X86_CET_STATUS		0x3001
> +#define ARCH_X86_CET_DISABLE		0x3002
> +#define ARCH_X86_CET_LOCK		0x3003
> +#define ARCH_X86_CET_ALLOC_SHSTK	0x3004
> +
>  #endif /* _ASM_X86_PRCTL_H */
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index e9cc2551573b..0b621e2afbdc 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -144,7 +144,7 @@ obj-$(CONFIG_UNWINDER_ORC)		+= unwind_orc.o
>  obj-$(CONFIG_UNWINDER_FRAME_POINTER)	+= unwind_frame.o
>  obj-$(CONFIG_UNWINDER_GUESS)		+= unwind_guess.o
>  
> -obj-$(CONFIG_X86_INTEL_CET)		+= cet.o
> +obj-$(CONFIG_X86_INTEL_CET)		+= cet.o cet_prctl.o
>  
>  ###
>  # 64 bit specific files
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index 121552047b86..c1b9b540c03e 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -145,6 +145,35 @@ static int create_rstor_token(bool ia32, unsigned long ssp,
>  	return 0;
>  }
>  
> +int cet_alloc_shstk(unsigned long *arg)
> +{
> +	unsigned long len = *arg;
> +	unsigned long addr;
> +	unsigned long token;
> +	unsigned long ssp;
> +
> +	addr = alloc_shstk(round_up(len, PAGE_SIZE));
> +
> +	if (IS_ERR((void *)addr))
> +		return PTR_ERR((void *)addr);
> +
> +	/* Restore token is 8 bytes and aligned to 8 bytes */
> +	ssp = addr + len;
> +	token = ssp;
> +
> +	if (!in_ia32_syscall())
> +		token |= TOKEN_MODE_64;
> +	ssp -= 8;
> +
> +	if (write_user_shstk_64(ssp, token)) {
> +		vm_munmap(addr, len);
> +		return -EINVAL;
> +	}
> +
> +	*arg = addr;
> +	return 0;
> +}
> +
>  int cet_setup_shstk(void)
>  {
>  	unsigned long addr, size;
> diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c
> new file mode 100644
> index 000000000000..0139c48f2215
> --- /dev/null
> +++ b/arch/x86/kernel/cet_prctl.c
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/errno.h>
> +#include <linux/uaccess.h>
> +#include <linux/prctl.h>
> +#include <linux/compat.h>
> +#include <linux/mman.h>
> +#include <linux/elfcore.h>
> +#include <asm/processor.h>
> +#include <asm/prctl.h>
> +#include <asm/cet.h>
> +
> +/* See Documentation/x86/intel_cet.rst. */
> +
> +static int handle_get_status(u64 arg2)
> +{
> +	struct cet_status *cet = &current->thread.cet;
> +	u64 buf[3] = {0, 0, 0};
> +
> +	if (cet->shstk_size) {
> +		buf[0] |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
> +		buf[1] = (u64)cet->shstk_base;
> +		buf[2] = (u64)cet->shstk_size;
> +	}
> +
> +	return copy_to_user((u64 __user *)arg2, buf, sizeof(buf));
> +}
> +
> +static int handle_alloc_shstk(u64 arg2)
> +{
> +	int err = 0;
> +	unsigned long arg;
> +	unsigned long addr = 0;
> +	unsigned long size = 0;
> +
> +	if (get_user(arg, (unsigned long __user *)arg2))
> +		return -EFAULT;
> +
> +	size = arg;
> +	err = cet_alloc_shstk(&arg);
> +	if (err)
> +		return err;
> +
> +	addr = arg;
> +	if (put_user((u64)addr, (u64 __user *)arg2)) {
> +		vm_munmap(addr, size);
> +		return -EFAULT;
> +	}
> +
> +	return 0;
> +}
> +
> +int prctl_cet(int option, u64 arg2)
> +{
> +	struct cet_status *cet;
> +
> +	if (!IS_ENABLED(CONFIG_X86_INTEL_CET))
> +		return -EINVAL;

Using -EINVAL here means userspace can't tell the difference between an
old kernel and a kernel not built with CONFIG_X86_INTEL_CET. Perhaps
-ENOTSUPP?

> +
> +	if (option == ARCH_X86_CET_STATUS)
> +		return handle_get_status(arg2);
> +
> +	if (!static_cpu_has(X86_FEATURE_SHSTK))
> +		return -EINVAL;

Similar case: though this is now a kernel that knows how, but a CPU that
doesn't.

> +
> +	cet = &current->thread.cet;

You get this both here and in handle_get_status(). Why not just get it
once and pass it into handle_get_status()? (And perhaps rename it to
"copy_status_to_user" or so?

> +
> +	switch (option) {
> +	case ARCH_X86_CET_DISABLE:

This must check for unknown flags before doing anything else:

		if (arg & ~(GNU_PROPERTY_X86_FEATURE_1_SHSTK))
			return -EINVAL;

> +		if (cet->locked)
> +			return -EPERM;
> +		if (arg2 & GNU_PROPERTY_X86_FEATURE_1_SHSTK)
> +			cet_disable_free_shstk(current);
> +
> +		return 0;
> +
> +	case ARCH_X86_CET_LOCK:

Same here.

> +		cet->locked = 1;
> +		return 0;
> +
> +	case ARCH_X86_CET_ALLOC_SHSTK:
> +		return handle_alloc_shstk(arg2);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index ef1c2b8086a2..de6773dd6a16 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -996,7 +996,7 @@ long do_arch_prctl_common(struct task_struct *task, int option,
>  		return get_cpuid_mode();
>  	case ARCH_SET_CPUID:
>  		return set_cpuid_mode(task, cpuid_enabled);
> +	default:
> +		return prctl_cet(option, cpuid_enabled);
>  	}

This is weird, but yeah, there's only the cpuid and cet handlers... I
think do_arch_prctrl_common() should call the second arg "arg2" and
there should be a series of calls:

	ret = prctl_cpuid(task, option, args);
	if (ret != -EINVAL)
		return ret;
	return prctl_cet(option, args);

But that's just a style nit, I guess.

And really, why is x86's arch prtcl return EINVAL for unknown options?
It should use -ENOSYS for unknown options and -EINVAL for bad arguments,
but I guess it's too late for that. :)

-- 
Kees Cook

  reply index

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-29 22:07 [PATCH v10 00/26] Control-flow Enforcement: Shadow Stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 01/26] Documentation/x86: Add CET description Yu-cheng Yu
2020-04-29 22:53   ` Dave Hansen
2020-04-29 23:02     ` Yu-cheng Yu
2020-05-12 23:20       ` Yu-cheng Yu
2020-05-15 18:39         ` Dave Hansen
2020-05-15 21:33           ` Yu-cheng Yu
2020-05-15 22:43             ` Dave Hansen
2020-05-15 23:29               ` Yu-cheng Yu
2020-05-15 23:56                 ` Dave Hansen
2020-05-16  2:51                   ` H.J. Lu
2020-05-17 23:09                     ` Dave Hansen
2020-05-16  2:53                   ` Yu-cheng Yu
2020-05-18 13:41                     ` Dave Hansen
2020-05-18 14:01                       ` H.J. Lu
2020-05-18 14:26                         ` Dave Hansen
2020-05-18 14:21                       ` Yu-cheng Yu
2020-05-18 23:47                     ` Yu-cheng Yu
2020-05-19  0:38                       ` Dave Hansen
2020-05-19  1:35                         ` Andy Lutomirski
2020-05-20  1:04                           ` Andy Lutomirski
2020-05-29  2:08                             ` Yu-cheng Yu
2020-05-16  0:13               ` Andrew Cooper
2020-05-16  2:37                 ` H.J. Lu
2020-05-16 14:09                   ` Andrew Cooper
2020-05-22 16:49                     ` Peter Zijlstra
2020-05-22 17:48                       ` Andrew Cooper
2020-04-29 22:07 ` [PATCH v10 02/26] x86/cpufeatures: Add CET CPU feature flags for Control-flow Enforcement Technology (CET) Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 03/26] x86/fpu/xstate: Introduce CET MSR XSAVES supervisor states Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 04/26] x86/cet: Add control-protection fault handler Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 05/26] x86/cet/shstk: Add Kconfig option for user-mode Shadow Stack Yu-cheng Yu
2020-05-07 15:55   ` Dave Hansen
2020-05-07 16:59     ` Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 06/26] x86/mm: Change _PAGE_DIRTY to _PAGE_DIRTY_HW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 07/26] x86/mm: Remove _PAGE_DIRTY_HW from kernel RO pages Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 08/26] x86/mm: Introduce _PAGE_COW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 09/26] drm/i915/gvt: Change _PAGE_DIRTY to _PAGE_DIRTY_BITS Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 10/26] x86/mm: Update pte_modify for _PAGE_COW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 11/26] x86/mm: Update ptep_set_wrprotect() and pmdp_set_wrprotect() for transition from _PAGE_DIRTY_HW to _PAGE_COW Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 12/26] mm: Introduce VM_SHSTK for shadow stack memory Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 13/26] x86/mm: Shadow Stack page fault error checking Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 14/26] x86/mm: Update maybe_mkwrite() for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 15/26] mm: Fixup places that call pte_mkwrite() directly Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 16/26] mm: Add guard pages around a shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 17/26] mm/mmap: Add shadow stack pages to memory accounting Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 18/26] mm: Update can_follow_write_pte() for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 19/26] x86/cet/shstk: User-mode shadow stack support Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 20/26] x86/cet/shstk: Handle signals for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 21/26] ELF: UAPI and Kconfig additions for ELF program properties Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 22/26] ELF: Add ELF program property parsing support Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 23/26] ELF: Introduce arch_setup_elf_property() Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 24/26] x86/cet/shstk: ELF header parsing for shadow stack Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 25/26] x86/cet/shstk: Handle thread " Yu-cheng Yu
2020-04-29 22:07 ` [PATCH v10 26/26] x86/cet/shstk: Add arch_prctl functions for " Yu-cheng Yu
2020-05-21 22:42   ` Kees Cook [this message]
2020-05-22 17:17     ` Yu-cheng Yu
2020-05-22 17:29       ` Eugene Syromiatnikov
2020-05-22 18:13         ` Yu-cheng Yu
2020-05-21 15:15 ` [PATCH v10 00/26] Control-flow Enforcement: Shadow Stack Josh Poimboeuf
2020-05-21 15:57   ` Yu-cheng Yu
2020-05-21 18:50     ` Josh Poimboeuf
2020-05-21 19:08       ` Yu-cheng Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=202005211528.A12B4AD@keescook \
    --to=keescook@chromium.org \
    --cc=Dave.Martin@arm.com \
    --cc=arnd@arndb.de \
    --cc=bp@alien8.de \
    --cc=bsingharora@gmail.com \
    --cc=corbet@lwn.net \
    --cc=dave.hansen@linux.intel.com \
    --cc=esyr@redhat.com \
    --cc=fweimer@redhat.com \
    --cc=gorcunov@gmail.com \
    --cc=hjl.tools@gmail.com \
    --cc=hpa@zytor.com \
    --cc=jannh@google.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@kernel.org \
    --cc=mike.kravetz@oracle.com \
    --cc=mingo@redhat.com \
    --cc=nadav.amit@gmail.com \
    --cc=oleg@redhat.com \
    --cc=pavel@ucw.cz \
    --cc=peterz@infradead.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=rdunlap@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=vedvyas.shanbhogue@intel.com \
    --cc=weijiang.yang@intel.com \
    --cc=x86@kernel.org \
    --cc=yu-cheng.yu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-api Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-api/0 linux-api/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-api linux-api/ https://lore.kernel.org/linux-api \
		linux-api@vger.kernel.org
	public-inbox-index linux-api

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-api


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git