[RFC PATCH 0/5] Update selftests/x86 for CET

[RFC PATCH 0/5] Update selftests/x86 for CET

[Yu-cheng Yu]

When CET is enabled for selftests/x86, two tests need updates.

- The test 'sigreturn_64' does a sigreturn() from a 64-bit context into a
  32-bit context.  The task's 64-bit shadow stack pointer certainly
  triggers a fault.  Fix it by allocating and switching to a new shadow
  stack in the 32-bit address range.

  The arch_ptrcl(ARCH_X86_CET_ALLOC_SHSTK) is updated for taking a bit from
  the input parameter to indicate the desire of MAP_32BIT.  I am proposing
  this change to minimize API changes, but open to any alternatives.

- The test 'sysret_rip' fails because the assembly code needs ENDBR
  opcodes.  Fix it by adding just that.  My latest CET submission (v10)
  does not include the IBT patches.  My purpose of posting this now is to
  show the changes needed in x86 tests.  Since ENDBR is nop when IBT is not
  enabled, this patch can be applied now or split out and merged with the
  IBT patches.

- The makefile changes add "-fcf-protection -mshstk" to the gcc command,
  when those are available.

- Introduce cet_quick_test that checks the system's CET capabilities.

This series is based on my CET series:

https://lore.kernel.org/lkml/20200429220732.31602-2-yu-cheng.yu@intel.com/

Yu-cheng Yu (5):
  x86/cet/shstk: Modify ARCH_X86_CET_ALLOC_SHSTK for 32-bit address
    range
  selftest/x86: Enable CET for selftests/x86
  selftest/x86: Fix sigreturn_64 test.
  selftest/x86: Fix sysret_rip with ENDBR
  selftest/x86: Add CET quick test

 arch/x86/include/asm/cet.h                   |   2 +-
 arch/x86/include/uapi/asm/prctl.h            |   2 +
 arch/x86/kernel/cet.c                        |  19 ++-
 arch/x86/kernel/cet_prctl.c                  |   6 +-
 tools/testing/selftests/x86/Makefile         |   7 +-
 tools/testing/selftests/x86/cet_quick_test.c | 128 +++++++++++++++++++
 tools/testing/selftests/x86/sigreturn.c      |  28 ++++
 tools/testing/selftests/x86/sysret_rip.c     |   5 +-
 8 files changed, 185 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/x86/cet_quick_test.c

-- 
2.21.0

[RFC PATCH 1/5] x86/cet/shstk: Modify ARCH_X86_CET_ALLOC_SHSTK for 32-bit address range

[Yu-cheng Yu]

Sometimes a 64-bit task might need to have a shadow stack allocated from
within 32-bit address range.  One example is selftests/x86/sigreturn.

Currently arch_prctl(ARCH_X86_CET_ALLOC_SHSTK) takes a input parameter for
the desired shadow stack size.  Modify it and use bit[0] of the parameter
to indicate the desire to allocate from 32-bit address range.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/include/asm/cet.h        |  2 +-
 arch/x86/include/uapi/asm/prctl.h |  2 ++
 arch/x86/kernel/cet.c             | 19 ++++++++++++-------
 arch/x86/kernel/cet_prctl.c       |  6 +++++-
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
index f163c805a559..534b02785a39 100644
--- a/arch/x86/include/asm/cet.h
+++ b/arch/x86/include/asm/cet.h
@@ -22,7 +22,7 @@ struct cet_status {
 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);
+int cet_alloc_shstk(unsigned long *arg, int map_32bit);
 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);
diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
index d962f0ec9ccf..e254c6a21475 100644
--- a/arch/x86/include/uapi/asm/prctl.h
+++ b/arch/x86/include/uapi/asm/prctl.h
@@ -19,4 +19,6 @@
 #define ARCH_X86_CET_LOCK		0x3003
 #define ARCH_X86_CET_ALLOC_SHSTK	0x3004
 
+#define ARCH_X86_CET_ALLOC_SHSTK_32BIT	0x1UL
+
 #endif /* _ASM_X86_PRCTL_H */
diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
index 92b8730c0b08..d6f93e1864b2 100644
--- a/arch/x86/kernel/cet.c
+++ b/arch/x86/kernel/cet.c
@@ -57,14 +57,19 @@ static unsigned long cet_get_shstk_addr(void)
 	return ssp;
 }
 
-static unsigned long alloc_shstk(unsigned long size)
+static unsigned long alloc_shstk(unsigned long size, int map_32bit)
 {
 	struct mm_struct *mm = current->mm;
 	unsigned long addr, populate;
+	unsigned long map_flags;
+
+	map_flags = MAP_ANONYMOUS | MAP_PRIVATE;
+	if (map_32bit)
+		map_flags |= MAP_32BIT;
 
 	down_write(&mm->mmap_sem);
-	addr = do_mmap(NULL, 0, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE,
-		       VM_SHSTK, 0, &populate, NULL);
+	addr = do_mmap(NULL, 0, size, PROT_READ, map_flags, VM_SHSTK, 0,
+		       &populate, NULL);
 	up_write(&mm->mmap_sem);
 
 	if (populate)
@@ -147,14 +152,14 @@ static int create_rstor_token(bool ia32, unsigned long ssp,
 	return 0;
 }
 
-int cet_alloc_shstk(unsigned long *arg)
+int cet_alloc_shstk(unsigned long *arg, int map_32bit)
 {
 	unsigned long len = *arg;
 	unsigned long addr;
 	unsigned long token;
 	unsigned long ssp;
 
-	addr = alloc_shstk(round_up(len, PAGE_SIZE));
+	addr = alloc_shstk(round_up(len, PAGE_SIZE), map_32bit);
 
 	if (IS_ERR((void *)addr))
 		return PTR_ERR((void *)addr);
@@ -185,7 +190,7 @@ int cet_setup_shstk(void)
 		return -EOPNOTSUPP;
 
 	size = round_up(min(rlimit(RLIMIT_STACK), 1UL << 32), PAGE_SIZE);
-	addr = alloc_shstk(size);
+	addr = alloc_shstk(size, 0);
 
 	if (IS_ERR((void *)addr))
 		return PTR_ERR((void *)addr);
@@ -226,7 +231,7 @@ int cet_setup_thread_shstk(struct task_struct *tsk)
 	if (in_compat_syscall())
 		size /= 4;
 	size = round_up(size, PAGE_SIZE);
-	addr = alloc_shstk(size);
+	addr = alloc_shstk(size, 0);
 
 	if (IS_ERR((void *)addr)) {
 		cet->shstk_base = 0;
diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c
index a8e68fefd524..364ed2420202 100644
--- a/arch/x86/kernel/cet_prctl.c
+++ b/arch/x86/kernel/cet_prctl.c
@@ -35,12 +35,16 @@ static int handle_alloc_shstk(u64 arg2)
 	unsigned long arg;
 	unsigned long addr = 0;
 	unsigned long size = 0;
+	int map_32bit;
 
 	if (get_user(arg, (unsigned long __user *)arg2))
 		return -EFAULT;
 
+	map_32bit = (arg & ARCH_X86_CET_ALLOC_SHSTK_32BIT) ? 1 : 0;
+	arg &= ~(ARCH_X86_CET_ALLOC_SHSTK_32BIT);
+
 	size = arg;
-	err = cet_alloc_shstk(&arg);
+	err = cet_alloc_shstk(&arg, map_32bit);
 	if (err)
 		return err;
 
-- 
2.21.0

Re: [RFC PATCH 1/5] x86/cet/shstk: Modify ARCH_X86_CET_ALLOC_SHSTK for 32-bit address range

[Kees Cook]

On Thu, May 21, 2020 at 02:17:16PM -0700, Yu-cheng Yu wrote:
> Sometimes a 64-bit task might need to have a shadow stack allocated from
> within 32-bit address range.  One example is selftests/x86/sigreturn.
> 
> Currently arch_prctl(ARCH_X86_CET_ALLOC_SHSTK) takes a input parameter for
> the desired shadow stack size.  Modify it and use bit[0] of the parameter
> to indicate the desire to allocate from 32-bit address range.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  arch/x86/include/asm/cet.h        |  2 +-
>  arch/x86/include/uapi/asm/prctl.h |  2 ++
>  arch/x86/kernel/cet.c             | 19 ++++++++++++-------
>  arch/x86/kernel/cet_prctl.c       |  6 +++++-
>  4 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
> index f163c805a559..534b02785a39 100644
> --- a/arch/x86/include/asm/cet.h
> +++ b/arch/x86/include/asm/cet.h
> @@ -22,7 +22,7 @@ struct cet_status {
>  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);
> +int cet_alloc_shstk(unsigned long *arg, int map_32bit);
>  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);
> diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
> index d962f0ec9ccf..e254c6a21475 100644
> --- a/arch/x86/include/uapi/asm/prctl.h
> +++ b/arch/x86/include/uapi/asm/prctl.h
> @@ -19,4 +19,6 @@
>  #define ARCH_X86_CET_LOCK		0x3003
>  #define ARCH_X86_CET_ALLOC_SHSTK	0x3004
>  
> +#define ARCH_X86_CET_ALLOC_SHSTK_32BIT	0x1UL

Perhaps declare a set of bits here to verify that they are zero into the
future?

> +
>  #endif /* _ASM_X86_PRCTL_H */
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index 92b8730c0b08..d6f93e1864b2 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -57,14 +57,19 @@ static unsigned long cet_get_shstk_addr(void)
>  	return ssp;
>  }
>  
> -static unsigned long alloc_shstk(unsigned long size)
> +static unsigned long alloc_shstk(unsigned long size, int map_32bit)
>  {
>  	struct mm_struct *mm = current->mm;
>  	unsigned long addr, populate;
> +	unsigned long map_flags;
> +
> +	map_flags = MAP_ANONYMOUS | MAP_PRIVATE;
> +	if (map_32bit)
> +		map_flags |= MAP_32BIT;
>  
>  	down_write(&mm->mmap_sem);
> -	addr = do_mmap(NULL, 0, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE,
> -		       VM_SHSTK, 0, &populate, NULL);
> +	addr = do_mmap(NULL, 0, size, PROT_READ, map_flags, VM_SHSTK, 0,
> +		       &populate, NULL);
>  	up_write(&mm->mmap_sem);
>  
>  	if (populate)
> @@ -147,14 +152,14 @@ static int create_rstor_token(bool ia32, unsigned long ssp,
>  	return 0;
>  }
>  
> -int cet_alloc_shstk(unsigned long *arg)
> +int cet_alloc_shstk(unsigned long *arg, int map_32bit)
>  {
>  	unsigned long len = *arg;
>  	unsigned long addr;
>  	unsigned long token;
>  	unsigned long ssp;
>  
> -	addr = alloc_shstk(round_up(len, PAGE_SIZE));
> +	addr = alloc_shstk(round_up(len, PAGE_SIZE), map_32bit);
>  
>  	if (IS_ERR((void *)addr))
>  		return PTR_ERR((void *)addr);
> @@ -185,7 +190,7 @@ int cet_setup_shstk(void)
>  		return -EOPNOTSUPP;
>  
>  	size = round_up(min(rlimit(RLIMIT_STACK), 1UL << 32), PAGE_SIZE);
> -	addr = alloc_shstk(size);
> +	addr = alloc_shstk(size, 0);
>  
>  	if (IS_ERR((void *)addr))
>  		return PTR_ERR((void *)addr);
> @@ -226,7 +231,7 @@ int cet_setup_thread_shstk(struct task_struct *tsk)
>  	if (in_compat_syscall())
>  		size /= 4;
>  	size = round_up(size, PAGE_SIZE);
> -	addr = alloc_shstk(size);
> +	addr = alloc_shstk(size, 0);
>  
>  	if (IS_ERR((void *)addr)) {
>  		cet->shstk_base = 0;
> diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c
> index a8e68fefd524..364ed2420202 100644
> --- a/arch/x86/kernel/cet_prctl.c
> +++ b/arch/x86/kernel/cet_prctl.c
> @@ -35,12 +35,16 @@ static int handle_alloc_shstk(u64 arg2)
>  	unsigned long arg;
>  	unsigned long addr = 0;
>  	unsigned long size = 0;
> +	int map_32bit;
>  
>  	if (get_user(arg, (unsigned long __user *)arg2))
>  		return -EFAULT;
>  

i.e. reject arg if any bits besides ARCH_X86_CET_ALLOC_SHSTK_32BIT are
set in the mask you pick.

> +	map_32bit = (arg & ARCH_X86_CET_ALLOC_SHSTK_32BIT) ? 1 : 0;
> +	arg &= ~(ARCH_X86_CET_ALLOC_SHSTK_32BIT);

And then clear the whole mask here.

-Kees

> +
>  	size = arg;
> -	err = cet_alloc_shstk(&arg);
> +	err = cet_alloc_shstk(&arg, map_32bit);
>  	if (err)
>  		return err;
>  
> -- 
> 2.21.0
> 

-- 
Kees Cook

[RFC PATCH 2/5] selftest/x86: Enable CET for selftests/x86

[Yu-cheng Yu]

To build CET-enabled applications, GCC needs to support '-fcf-protection'.
Update x86 selftest makefile to detect and enable CET for x86 selftest
applications.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 tools/testing/selftests/x86/Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index 5d49bfec1e9a..f1bf5ab87160 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -9,6 +9,7 @@ UNAME_M := $(shell uname -m)
 CAN_BUILD_I386 := $(shell ./check_cc.sh $(CC) trivial_32bit_program.c -m32)
 CAN_BUILD_X86_64 := $(shell ./check_cc.sh $(CC) trivial_64bit_program.c)
 CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC) trivial_program.c -no-pie)
+CAN_BUILD_CET := $(shell ./check_cc.sh $(CC) trivial_program.c -fcf-protection)
 
 TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
 			check_initial_reg_state sigreturn iopl ioperm \
@@ -35,6 +36,10 @@ BINARIES_64 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_64))
 
 CFLAGS := -O2 -g -std=gnu99 -pthread -Wall
 
+ifeq ($(CAN_BUILD_CET),1)
+CFLAGS += -fcf-protection -mshstk
+endif
+
 # call32_from_64 in thunks.S uses absolute addresses.
 ifeq ($(CAN_BUILD_WITH_NOPIE),1)
 CFLAGS += -no-pie
-- 
2.21.0

Re: [RFC PATCH 2/5] selftest/x86: Enable CET for selftests/x86

[Kees Cook]

On Thu, May 21, 2020 at 02:17:17PM -0700, Yu-cheng Yu wrote:
> To build CET-enabled applications, GCC needs to support '-fcf-protection'.
> Update x86 selftest makefile to detect and enable CET for x86 selftest
> applications.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [RFC PATCH 2/5] selftest/x86: Enable CET for selftests/x86

[Yu-cheng Yu]

On Thu, 2020-05-21 at 15:44 -0700, Kees Cook wrote:
> On Thu, May 21, 2020 at 02:17:17PM -0700, Yu-cheng Yu wrote:
> > To build CET-enabled applications, GCC needs to support '-fcf-protection'.
> > Update x86 selftest makefile to detect and enable CET for x86 selftest
> > applications.
> > 
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks!  I will fix issues you pointed out in the series.

Yu-cheng

[RFC PATCH 3/5] selftest/x86: Fix sigreturn_64 test.

[Yu-cheng Yu]

When shadow stack is enabled, selftests/x86/sigreturn_64 triggers a fault
when doing sigreturn to 32-bit context but the task's shadow stack pointer
is above 32-bit address range.  Fix it by:

- Allocate a small shadow stack below 32-bit address,
- Switch to the new shadow stack,
- Run tests,
- Switch back to the original 64-bit shadow stack.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 tools/testing/selftests/x86/sigreturn.c | 28 +++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/x86/sigreturn.c b/tools/testing/selftests/x86/sigreturn.c
index 57c4f67f16ef..5bcd74d416ff 100644
--- a/tools/testing/selftests/x86/sigreturn.c
+++ b/tools/testing/selftests/x86/sigreturn.c
@@ -45,6 +45,14 @@
 #include <stdbool.h>
 #include <sys/ptrace.h>
 #include <sys/user.h>
+#include <x86intrin.h>
+#include <asm/prctl.h>
+#include <sys/prctl.h>
+
+#ifdef __x86_64__
+int arch_prctl(int code, unsigned long *addr);
+#define ARCH_CET_ALLOC_SHSTK 0x3004
+#endif
 
 /* Pull in AR_xyz defines. */
 typedef unsigned int u32;
@@ -766,6 +774,20 @@ int main()
 	int total_nerrs = 0;
 	unsigned short my_cs, my_ss;
 
+#ifdef __x86_64__
+	/* Alloc a shadow stack within 32-bit address range */
+	unsigned long arg, ssp_64, ssp_32;
+	ssp_64 = _get_ssp();
+
+	if (ssp_64 != 0) {
+		arg = 0x1001;
+		arch_prctl(ARCH_CET_ALLOC_SHSTK, &arg);
+		ssp_32 = arg + 0x1000 - 8;
+		asm volatile("RSTORSSP (%0)\n":: "r" (ssp_32));
+		asm volatile("SAVEPREVSSP");
+	}
+#endif
+
 	asm volatile ("mov %%cs,%0" : "=r" (my_cs));
 	asm volatile ("mov %%ss,%0" : "=r" (my_ss));
 	setup_ldt();
@@ -870,6 +892,12 @@ int main()
 
 #ifdef __x86_64__
 	total_nerrs += test_nonstrict_ss();
+
+	if (ssp_64 != 0) {
+		ssp_64 -= 8;
+		asm volatile("RSTORSSP (%0)\n":: "r" (ssp_64));
+		asm volatile("SAVEPREVSSP");
+	}
 #endif
 
 	return total_nerrs ? 1 : 0;
-- 
2.21.0

Re: [RFC PATCH 3/5] selftest/x86: Fix sigreturn_64 test.

[Kees Cook]

On Thu, May 21, 2020 at 02:17:18PM -0700, Yu-cheng Yu wrote:
> When shadow stack is enabled, selftests/x86/sigreturn_64 triggers a fault
> when doing sigreturn to 32-bit context but the task's shadow stack pointer
> is above 32-bit address range.  Fix it by:
> 
> - Allocate a small shadow stack below 32-bit address,
> - Switch to the new shadow stack,
> - Run tests,
> - Switch back to the original 64-bit shadow stack.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  tools/testing/selftests/x86/sigreturn.c | 28 +++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/x86/sigreturn.c b/tools/testing/selftests/x86/sigreturn.c
> index 57c4f67f16ef..5bcd74d416ff 100644
> --- a/tools/testing/selftests/x86/sigreturn.c
> +++ b/tools/testing/selftests/x86/sigreturn.c
> @@ -45,6 +45,14 @@
>  #include <stdbool.h>
>  #include <sys/ptrace.h>
>  #include <sys/user.h>
> +#include <x86intrin.h>
> +#include <asm/prctl.h>
> +#include <sys/prctl.h>
> +
> +#ifdef __x86_64__
> +int arch_prctl(int code, unsigned long *addr);
> +#define ARCH_CET_ALLOC_SHSTK 0x3004
> +#endif
>  
>  /* Pull in AR_xyz defines. */
>  typedef unsigned int u32;
> @@ -766,6 +774,20 @@ int main()
>  	int total_nerrs = 0;
>  	unsigned short my_cs, my_ss;
>  
> +#ifdef __x86_64__

I think this should also be gated by whether the compiler will know what
to do with the shadow stack instructions. (Perhaps the earlier Makefile
define can be exported and tested here.)

> +	/* Alloc a shadow stack within 32-bit address range */
> +	unsigned long arg, ssp_64, ssp_32;
> +	ssp_64 = _get_ssp();
> +
> +	if (ssp_64 != 0) {
> +		arg = 0x1001;
> +		arch_prctl(ARCH_CET_ALLOC_SHSTK, &arg);
> +		ssp_32 = arg + 0x1000 - 8;
> +		asm volatile("RSTORSSP (%0)\n":: "r" (ssp_32));
> +		asm volatile("SAVEPREVSSP");
> +	}
> +#endif

-- 
Kees Cook

Re: [RFC PATCH 3/5] selftest/x86: Fix sigreturn_64 test.

[Kees Cook]

On Thu, May 21, 2020 at 02:17:18PM -0700, Yu-cheng Yu wrote:
> When shadow stack is enabled, selftests/x86/sigreturn_64 triggers a fault
> when doing sigreturn to 32-bit context but the task's shadow stack pointer
> is above 32-bit address range.  Fix it by:
> 
> - Allocate a small shadow stack below 32-bit address,
> - Switch to the new shadow stack,
> - Run tests,
> - Switch back to the original 64-bit shadow stack.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  tools/testing/selftests/x86/sigreturn.c | 28 +++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/x86/sigreturn.c b/tools/testing/selftests/x86/sigreturn.c
> index 57c4f67f16ef..5bcd74d416ff 100644
> --- a/tools/testing/selftests/x86/sigreturn.c
> +++ b/tools/testing/selftests/x86/sigreturn.c
> @@ -45,6 +45,14 @@
>  #include <stdbool.h>
>  #include <sys/ptrace.h>
>  #include <sys/user.h>
> +#include <x86intrin.h>
> +#include <asm/prctl.h>
> +#include <sys/prctl.h>
> +
> +#ifdef __x86_64__
> +int arch_prctl(int code, unsigned long *addr);
> +#define ARCH_CET_ALLOC_SHSTK 0x3004
> +#endif
>  
>  /* Pull in AR_xyz defines. */
>  typedef unsigned int u32;
> @@ -766,6 +774,20 @@ int main()
>  	int total_nerrs = 0;
>  	unsigned short my_cs, my_ss;
>  
> +#ifdef __x86_64__
> +	/* Alloc a shadow stack within 32-bit address range */
> +	unsigned long arg, ssp_64, ssp_32;
> +	ssp_64 = _get_ssp();
> +
> +	if (ssp_64 != 0) {
> +		arg = 0x1001;
> +		arch_prctl(ARCH_CET_ALLOC_SHSTK, &arg);
> +		ssp_32 = arg + 0x1000 - 8;
> +		asm volatile("RSTORSSP (%0)\n":: "r" (ssp_32));
> +		asm volatile("SAVEPREVSSP");
> +	}
> +#endif

If the headers and code are going to be repeated, I would put that in a
shared header so they're not copy/pasted between these two tests.

-Kees

-- 
Kees Cook

[RFC PATCH 4/5] selftest/x86: Fix sysret_rip with ENDBR

[Yu-cheng Yu]

Insert endbr64 to assembly code of sysret_rip.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 tools/testing/selftests/x86/sysret_rip.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d902..027682a0f377 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -27,8 +27,9 @@ asm (
 	".pushsection \".text\", \"ax\"\n\t"
 	".balign 4096\n\t"
 	"test_page: .globl test_page\n\t"
-	".fill 4094,1,0xcc\n\t"
+	".fill 4090,1,0xcc\n\t"
 	"test_syscall_insn:\n\t"
+	"endbr64\n\t"
 	"syscall\n\t"
 	".ifne . - test_page - 4096\n\t"
 	".error \"test page is not one page long\"\n\t"
@@ -151,7 +152,7 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 
 	if (sigsetjmp(jmpbuf, 1) == 0) {
 		asm volatile ("call *%[syscall_insn]" :: "a" (SYS_getpid),
-			      [syscall_insn] "rm" (ip - 2));
+			      [syscall_insn] "rm" (ip - 6));
 		errx(1, "[FAIL]\tSyscall trampoline returned");
 	}
 
-- 
2.21.0

Re: [RFC PATCH 4/5] selftest/x86: Fix sysret_rip with ENDBR

[Thomas Gleixner]

Yu-cheng Yu <yu-cheng.yu@intel.com> writes:

> Insert endbr64 to assembly code of sysret_rip.

This changelog explains what the patch does, but not the WHY. It also
lacks any information why this is harmless for !CET enabled systems.

> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---

Please consult Dave Hansen about the void between your Signed-off-by
and the '---' line.

Thanks,

        tglx

Re: [RFC PATCH 4/5] selftest/x86: Fix sysret_rip with ENDBR

[Yu-cheng Yu]

On Thu, 2020-05-21 at 23:34 +0200, Thomas Gleixner wrote:
> Yu-cheng Yu <yu-cheng.yu@intel.com> writes:
> 
> > Insert endbr64 to assembly code of sysret_rip.
> 
> This changelog explains what the patch does, but not the WHY. It also
> lacks any information why this is harmless for !CET enabled systems.
> 
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > ---
> 
> Please consult Dave Hansen about the void between your Signed-off-by
> and the '---' line.
> 
> Thanks,
> 
>         tglx

Thanks!  I will work on it.

Yu-cheng

[RFC PATCH 5/5] selftest/x86: Add CET quick test

[Yu-cheng Yu]

Introduce a quick test to verify shadow stack and IBT are working.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 tools/testing/selftests/x86/Makefile         |   2 +-
 tools/testing/selftests/x86/cet_quick_test.c | 128 +++++++++++++++++++
 2 files changed, 129 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/x86/cet_quick_test.c

diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index f1bf5ab87160..26e68272117a 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -14,7 +14,7 @@ CAN_BUILD_CET := $(shell ./check_cc.sh $(CC) trivial_program.c -fcf-protection)
 TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
 			check_initial_reg_state sigreturn iopl ioperm \
 			protection_keys test_vdso test_vsyscall mov_ss_trap \
-			syscall_arg_fault
+			syscall_arg_fault cet_quick_test
 TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
diff --git a/tools/testing/selftests/x86/cet_quick_test.c b/tools/testing/selftests/x86/cet_quick_test.c
new file mode 100644
index 000000000000..e84bbbcfd26f
--- /dev/null
+++ b/tools/testing/selftests/x86/cet_quick_test.c
@@ -0,0 +1,128 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Quick tests to verify Shadow Stack and IBT are working */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <string.h>
+#include <ucontext.h>
+
+ucontext_t ucp;
+int result[4] = {-1, -1, -1, -1};
+int test_id;
+
+void stack_hacked(unsigned long x)
+{
+	result[test_id] = -1;
+	test_id++;
+	setcontext(&ucp);
+}
+
+#pragma GCC push_options
+#pragma GCC optimize ("O0")
+void ibt_violation(void)
+{
+#ifdef __i386__
+	asm volatile("lea 1f, %eax");
+	asm volatile("jmp *%eax");
+#else
+	asm volatile("lea 1f, %rax");
+	asm volatile("jmp *%rax");
+#endif
+	asm volatile("1:");
+	result[test_id] = -1;
+	test_id++;
+	setcontext(&ucp);
+}
+
+void shstk_violation(void)
+{
+#ifdef __i386__
+	unsigned long x = 0;
+
+	((unsigned long *)&x)[2] = (unsigned long)stack_hacked;
+#else
+	unsigned long long x = 0;
+
+	((unsigned long long *)&x)[2] = (unsigned long)stack_hacked;
+#endif
+}
+#pragma GCC pop_options
+
+void segv_handler(int signum, siginfo_t *si, void *uc)
+{
+	result[test_id] = 0;
+	test_id++;
+	setcontext(&ucp);
+}
+
+void user1_handler(int signum, siginfo_t *si, void *uc)
+{
+	shstk_violation();
+}
+
+void user2_handler(int signum, siginfo_t *si, void *uc)
+{
+	ibt_violation();
+}
+
+int main(int argc, char *argv[])
+{
+	struct sigaction sa;
+	int r;
+
+	r = sigemptyset(&sa.sa_mask);
+	if (r)
+		return -1;
+
+	sa.sa_flags = SA_SIGINFO;
+
+	/*
+	 * Control protection fault handler
+	 */
+	sa.sa_sigaction = segv_handler;
+	r = sigaction(SIGSEGV, &sa, NULL);
+	if (r)
+		return -1;
+
+	/*
+	 * Handler to test Shadow stack
+	 */
+	sa.sa_sigaction = user1_handler;
+	r = sigaction(SIGUSR1, &sa, NULL);
+	if (r)
+		return -1;
+
+	/*
+	 * Handler to test IBT
+	 */
+	sa.sa_sigaction = user2_handler;
+	r = sigaction(SIGUSR2, &sa, NULL);
+	if (r)
+		return -1;
+
+	test_id = 0;
+	r = getcontext(&ucp);
+	if (r)
+		return -1;
+
+	if (test_id == 0)
+		shstk_violation();
+	else if (test_id == 1)
+		ibt_violation();
+	else if (test_id == 2)
+		raise(SIGUSR1);
+	else if (test_id == 3)
+		raise(SIGUSR2);
+
+	r = 0;
+	printf("[%s]\tShadow stack\n", result[0] ? "FAIL":"OK");
+	r += result[0];
+	printf("[%s]\tIBT\n", result[1] ? "FAIL":"OK");
+	r += result[1];
+	printf("[%s]\tShadow stack in signal\n", result[2] ? "FAIL":"OK");
+	r += result[2];
+	printf("[%s]\tIBT in signal\n", result[3] ? "FAIL":"OK");
+	r += result[3];
+	return r;
+}
-- 
2.21.0

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Kees Cook]

On Thu, May 21, 2020 at 02:17:20PM -0700, Yu-cheng Yu wrote:
> Introduce a quick test to verify shadow stack and IBT are working.

Cool! :)

I'd love to see either more of a commit log or more comments in the test
code itself. I had to spend a bit of time trying to understand how the
test was working. (i.e. using ucontext to "reset", using segv handler to
catch some of them, etc.) I have not yet figured out why you need to
send USR1/USR2 for two of them instead of direct calls?

More notes below...

> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  tools/testing/selftests/x86/Makefile         |   2 +-
>  tools/testing/selftests/x86/cet_quick_test.c | 128 +++++++++++++++++++
>  2 files changed, 129 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/x86/cet_quick_test.c
> 
> diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> index f1bf5ab87160..26e68272117a 100644
> --- a/tools/testing/selftests/x86/Makefile
> +++ b/tools/testing/selftests/x86/Makefile
> @@ -14,7 +14,7 @@ CAN_BUILD_CET := $(shell ./check_cc.sh $(CC) trivial_program.c -fcf-protection)
>  TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
>  			check_initial_reg_state sigreturn iopl ioperm \
>  			protection_keys test_vdso test_vsyscall mov_ss_trap \
> -			syscall_arg_fault
> +			syscall_arg_fault cet_quick_test
>  TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
>  			test_FCMOV test_FCOMI test_FISTTP \
>  			vdso_restorer
> diff --git a/tools/testing/selftests/x86/cet_quick_test.c b/tools/testing/selftests/x86/cet_quick_test.c
> new file mode 100644
> index 000000000000..e84bbbcfd26f
> --- /dev/null
> +++ b/tools/testing/selftests/x86/cet_quick_test.c
> @@ -0,0 +1,128 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Quick tests to verify Shadow Stack and IBT are working */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <ucontext.h>
> +
> +ucontext_t ucp;
> +int result[4] = {-1, -1, -1, -1};

I think you likely want three states: no signal, failed, and okay.
Perhaps -1 for "no signal" like you have above, zero for failed, and 1
for okay.

> +int test_id;
> +
> +void stack_hacked(unsigned long x)
> +{
> +	result[test_id] = -1;

So this is set to 0: "I absolutely bypassed the protection".

> +	test_id++;
> +	setcontext(&ucp);
> +}
> +
> +#pragma GCC push_options
> +#pragma GCC optimize ("O0")

Can you avoid compiler-specific pragmas? (Or verify that Clang also
behaves correctly here?) Maybe it's better to just build the entire file
with -O0 in the Makefile?

> +void ibt_violation(void)
> +{
> +#ifdef __i386__
> +	asm volatile("lea 1f, %eax");
> +	asm volatile("jmp *%eax");
> +#else
> +	asm volatile("lea 1f, %rax");
> +	asm volatile("jmp *%rax");
> +#endif
> +	asm volatile("1:");
> +	result[test_id] = -1;

Set to 0, and if the segv doesn't see it, we know for sure it failed.

> +	test_id++;
> +	setcontext(&ucp);
> +}
> +
> +void shstk_violation(void)
> +{
> +#ifdef __i386__
> +	unsigned long x = 0;
> +
> +	((unsigned long *)&x)[2] = (unsigned long)stack_hacked;
> +#else
> +	unsigned long long x = 0;
> +
> +	((unsigned long long *)&x)[2] = (unsigned long)stack_hacked;
> +#endif
> +}
> +#pragma GCC pop_options
> +
> +void segv_handler(int signum, siginfo_t *si, void *uc)
> +{

Does anything in siginfo_t indicate which kind of failure you're
detecting? It'd be nice to verify test_id matches the failure mode being
tested.

> +	result[test_id] = 0;
> +	test_id++;
> +	setcontext(&ucp);
> +}
> +
> +void user1_handler(int signum, siginfo_t *si, void *uc)
> +{
> +	shstk_violation();
> +}
> +
> +void user2_handler(int signum, siginfo_t *si, void *uc)
> +{
> +	ibt_violation();
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct sigaction sa;
> +	int r;
> +
> +	r = sigemptyset(&sa.sa_mask);
> +	if (r)
> +		return -1;
> +
> +	sa.sa_flags = SA_SIGINFO;
> +
> +	/*
> +	 * Control protection fault handler
> +	 */
> +	sa.sa_sigaction = segv_handler;
> +	r = sigaction(SIGSEGV, &sa, NULL);
> +	if (r)
> +		return -1;
> +
> +	/*
> +	 * Handler to test Shadow stack
> +	 */
> +	sa.sa_sigaction = user1_handler;
> +	r = sigaction(SIGUSR1, &sa, NULL);
> +	if (r)
> +		return -1;
> +
> +	/*
> +	 * Handler to test IBT
> +	 */
> +	sa.sa_sigaction = user2_handler;
> +	r = sigaction(SIGUSR2, &sa, NULL);
> +	if (r)
> +		return -1;
> +
> +	test_id = 0;
> +	r = getcontext(&ucp);
> +	if (r)
> +		return -1;
> +
> +	if (test_id == 0)
> +		shstk_violation();
> +	else if (test_id == 1)
> +		ibt_violation();
> +	else if (test_id == 2)
> +		raise(SIGUSR1);
> +	else if (test_id == 3)
> +		raise(SIGUSR2);
> +
> +	r = 0;
> +	printf("[%s]\tShadow stack\n", result[0] ? "FAIL":"OK");

Then these are result[0] == -1 ? "untested" : (result[0] ? "OK" : "FAIL"))

> +	r += result[0];
> +	printf("[%s]\tIBT\n", result[1] ? "FAIL":"OK");
> +	r += result[1];
> +	printf("[%s]\tShadow stack in signal\n", result[2] ? "FAIL":"OK");
> +	r += result[2];
> +	printf("[%s]\tIBT in signal\n", result[3] ? "FAIL":"OK");
> +	r += result[3];
> +	return r;
> +}
> -- 
> 2.21.0
> 

-- 
Kees Cook

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Yu-cheng Yu]

On Thu, 2020-05-21 at 16:02 -0700, Kees Cook wrote:
> On Thu, May 21, 2020 at 02:17:20PM -0700, Yu-cheng Yu wrote:
> > Introduce a quick test to verify shadow stack and IBT are working.
> 
> Cool! :)
> 
> I'd love to see either more of a commit log or more comments in the test
> code itself. I had to spend a bit of time trying to understand how the
> test was working. (i.e. using ucontext to "reset", using segv handler to
> catch some of them, etc.) I have not yet figured out why you need to
> send USR1/USR2 for two of them instead of direct calls?

Yes, I will work on it.

[...]

> > +
> > +#pragma GCC push_options
> > +#pragma GCC optimize ("O0")
> 
> Can you avoid compiler-specific pragmas? (Or verify that Clang also
> behaves correctly here?) Maybe it's better to just build the entire file
> with -O0 in the Makefile?

This file is compiled using -O2 in the makefile.  I will see if other ways are
possible.

[...]

> > +
> > +void segv_handler(int signum, siginfo_t *si, void *uc)
> > +{
> 
> Does anything in siginfo_t indicate which kind of failure you're
> detecting? It'd be nice to verify test_id matches the failure mode being
> tested.

Yes, there is an si_code for control-protection fault.
I will fix this.

Agree with your other comments.

Thanks,
Yu-cheng

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Peter Zijlstra]

On Thu, May 21, 2020 at 02:17:20PM -0700, Yu-cheng Yu wrote:

> +#pragma GCC push_options
> +#pragma GCC optimize ("O0")
> +void ibt_violation(void)
> +{
> +#ifdef __i386__
> +	asm volatile("lea 1f, %eax");
> +	asm volatile("jmp *%eax");
> +#else
> +	asm volatile("lea 1f, %rax");
> +	asm volatile("jmp *%rax");
> +#endif
> +	asm volatile("1:");
> +	result[test_id] = -1;
> +	test_id++;
> +	setcontext(&ucp);
> +}
> +
> +void shstk_violation(void)
> +{
> +#ifdef __i386__
> +	unsigned long x = 0;
> +
> +	((unsigned long *)&x)[2] = (unsigned long)stack_hacked;
> +#else
> +	unsigned long long x = 0;
> +
> +	((unsigned long long *)&x)[2] = (unsigned long)stack_hacked;
> +#endif
> +}
> +#pragma GCC pop_options

This is absolutely atrocious.

The #pragma like Kees already said just need to go. Also, there's
absolutely no clue what so ever what it attempts to achieve.

The __i386__ ifdeffery is horrible crap. Splitting an asm with #ifdef
like that is also horrible crap.

This is not how you write code.

Get asm/asm.h into userspace and then write something like:


void ibt_violation(void)
{
	asm volatile("lea  1f, %" _ASM_AX "\n\t"
		     "jmp  *%" _ASM_AX "\n\t"
		     "1:\n\t" ::: "a");

	WRITE_ONCE(result[test_id], -1);
	WRITE_ONCE(test_id, test_id+1);

	setcontext(&ucp);
}

void shstk_violation(void)
{
	unsigned long x = 0;

	WRITE_ONCE(x[2], stack_hacked);
}

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Yu-cheng Yu]

On Fri, 2020-05-22 at 11:28 +0200, Peter Zijlstra wrote:
> On Thu, May 21, 2020 at 02:17:20PM -0700, Yu-cheng Yu wrote:
> 
> > +#pragma GCC push_options
> > +#pragma GCC optimize ("O0")
> > +void ibt_violation(void)
> > +{
> > +#ifdef __i386__
> > +	asm volatile("lea 1f, %eax");
> > +	asm volatile("jmp *%eax");
> > +#else
> > +	asm volatile("lea 1f, %rax");
> > +	asm volatile("jmp *%rax");
> > +#endif
> > +	asm volatile("1:");
> > +	result[test_id] = -1;
> > +	test_id++;
> > +	setcontext(&ucp);
> > +}
> > +
> > +void shstk_violation(void)
> > +{
> > +#ifdef __i386__
> > +	unsigned long x = 0;
> > +
> > +	((unsigned long *)&x)[2] = (unsigned long)stack_hacked;
> > +#else
> > +	unsigned long long x = 0;
> > +
> > +	((unsigned long long *)&x)[2] = (unsigned long)stack_hacked;
> > +#endif
> > +}
> > +#pragma GCC pop_options
> 
> This is absolutely atrocious.
> 
> The #pragma like Kees already said just need to go. Also, there's
> absolutely no clue what so ever what it attempts to achieve.
> 
> The __i386__ ifdeffery is horrible crap. Splitting an asm with #ifdef
> like that is also horrible crap.
> 
> This is not how you write code.
> 
> Get asm/asm.h into userspace and then write something like:
> 
> 
> void ibt_violation(void)
> {
> 	asm volatile("lea  1f, %" _ASM_AX "\n\t"
> 		     "jmp  *%" _ASM_AX "\n\t"
> 		     "1:\n\t" ::: "a");
> 
> 	WRITE_ONCE(result[test_id], -1);
> 	WRITE_ONCE(test_id, test_id+1);
> 
> 	setcontext(&ucp);
> }
> 
> void shstk_violation(void)
> {
> 	unsigned long x = 0;
> 
> 	WRITE_ONCE(x[2], stack_hacked);
> }

Thanks!  I will change it.

Yu-cheng

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Kees Cook]

On Fri, May 22, 2020 at 11:28:48AM +0200, Peter Zijlstra wrote:
> Get asm/asm.h into userspace and then write something like:

Yeah, selftests is going to start suffering from the same "tools/ header
duplication" problem. I've also had cases (see the logic in the Makefile
in selftests/x86) where selftests is duplicating existing Kconfig and
Makefile logic ("can I build this way?")

But yes, I think getting a copy of asm.h would be nice here. I don't
think the WRITE_ONCE() is needed in this particular case. Hmm.

-- 
Kees Cook

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Peter Zijlstra]

On Fri, May 22, 2020 at 10:22:51AM -0700, Kees Cook wrote:

> But yes, I think getting a copy of asm.h would be nice here. I don't
> think the WRITE_ONCE() is needed in this particular case. Hmm.

Paranoia on my end because I had no clue wth he wanted with his -O0
magic gunk.

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Kees Cook]

On Fri, May 22, 2020 at 07:27:11PM +0200, Peter Zijlstra wrote:
> On Fri, May 22, 2020 at 10:22:51AM -0700, Kees Cook wrote:
> 
> > But yes, I think getting a copy of asm.h would be nice here. I don't
> > think the WRITE_ONCE() is needed in this particular case. Hmm.
> 
> Paranoia on my end because I had no clue wth he wanted with his -O0
> magic gunk.

Heh, yes, which is why I asked for many more comments. ;) I *think* it
was entirely to control the stack (and ssp) behavior (i.e. don't inline,
don't elide unused stack variables, etc).

-- 
Kees Cook

Re: [RFC PATCH 5/5] selftest/x86: Add CET quick test

[Yu-cheng Yu]

On Fri, 2020-05-22 at 10:36 -0700, Kees Cook wrote:
> On Fri, May 22, 2020 at 07:27:11PM +0200, Peter Zijlstra wrote:
> > On Fri, May 22, 2020 at 10:22:51AM -0700, Kees Cook wrote:
> > 
> > > But yes, I think getting a copy of asm.h would be nice here. I don't
> > > think the WRITE_ONCE() is needed in this particular case. Hmm.
> > 
> > Paranoia on my end because I had no clue wth he wanted with his -O0
> > magic gunk.
> 
> Heh, yes, which is why I asked for many more comments. ;) I *think* it
> was entirely to control the stack (and ssp) behavior (i.e. don't inline,
> don't elide unused stack variables, etc).

Yes, that was the reason.

Yu-cheng