Re: [PATCH v10 26/26] x86/cet/shstk: Add arch_prctl functions for shadow stack

2020-05-22 Thread Yu-cheng Yu
On Fri, 2020-05-22 at 19:29 +0200, Eugene Syromiatnikov wrote:
> On Fri, May 22, 2020 at 10:17:43AM -0700, Yu-cheng Yu wrote:
> > On Thu, 2020-05-21 at 15:42 -0700, Kees Cook wrote:
> > > On Wed, Apr 29, 2020 at 03:07:32PM -0700, Yu-cheng Yu wrote:
> > [...]
> > > > +
> > > > +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?
> > 
> > Looked into this.  The kernel and GLIBC are not in sync.  So maybe we still 
> > use
> > EINVAL here?
> > 
> > Yu-cheng
> > 
> > 
> > 
> > In kernel:
> > --
> > 
> > #define EOPNOTSUPP  95
> > #define ENOTSUPP524
> > 
> > In GLIBC:
> > -
> > 
> > printf("ENOTSUP=%d\n", ENOTSUP);
> > printf("EOPNOTSUPP=%d\n", EOPNOTSUPP);
> > printf("%s=524\n", strerror(524));
> >  
> > ENOTSUP=95
> > EOPNOTSUPP=95
> > Unknown error 524=524
> 
> EOPNOTSUPP/ENOTSUP/ENOTSUPP is actually a mess, it's summarized recently
> by Michael Kerrisk[1].  From the kernel's point of view, I think it
> would be reasonable to return EOPNOTSUPP, and expect that the userspace
> would use ENOTSUP to match against it.

Ok, use EOPNOTSUPP and add a comment why.

Yu-cheng



Re: [PATCH v10 26/26] x86/cet/shstk: Add arch_prctl functions for shadow stack

2020-05-22 Thread Eugene Syromiatnikov
On Fri, May 22, 2020 at 10:17:43AM -0700, Yu-cheng Yu wrote:
> On Thu, 2020-05-21 at 15:42 -0700, Kees Cook wrote:
> > On Wed, Apr 29, 2020 at 03:07:32PM -0700, Yu-cheng Yu wrote:
> [...]
> > > +
> > > +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?
> 
> Looked into this.  The kernel and GLIBC are not in sync.  So maybe we still 
> use
> EINVAL here?
> 
> Yu-cheng
> 
> 
> 
> In kernel:
> --
> 
> #define EOPNOTSUPP95
> #define ENOTSUPP  524
> 
> In GLIBC:
> -
> 
> printf("ENOTSUP=%d\n", ENOTSUP);
> printf("EOPNOTSUPP=%d\n", EOPNOTSUPP);
> printf("%s=524\n", strerror(524));
>  
> ENOTSUP=95
> EOPNOTSUPP=95
> Unknown error 524=524

EOPNOTSUPP/ENOTSUP/ENOTSUPP is actually a mess, it's summarized recently
by Michael Kerrisk[1].  From the kernel's point of view, I think it
would be reasonable to return EOPNOTSUPP, and expect that the userspace
would use ENOTSUP to match against it.

[1] 
https://lore.kernel.org/linux-man/cb4c685b-6c5d-9c16-aade-0c95e57de...@gmail.com/



Re: [PATCH v10 26/26] x86/cet/shstk: Add arch_prctl functions for shadow stack

2020-05-22 Thread Yu-cheng Yu
On Thu, 2020-05-21 at 15:42 -0700, Kees Cook wrote:
> On Wed, Apr 29, 2020 at 03:07:32PM -0700, Yu-cheng Yu wrote:
[...]
> > +
> > +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?

Looked into this.  The kernel and GLIBC are not in sync.  So maybe we still use
EINVAL here?

Yu-cheng



In kernel:
--

#define EOPNOTSUPP  95
#define ENOTSUPP524

In GLIBC:
-

printf("ENOTSUP=%d\n", ENOTSUP);
printf("EOPNOTSUPP=%d\n", EOPNOTSUPP);
printf("%s=524\n", strerror(524));
 
ENOTSUP=95
EOPNOTSUPP=95
Unknown error 524=524



Re: [PATCH v10 26/26] x86/cet/shstk: Add arch_prctl functions for shadow stack

2020-05-21 Thread Kees Cook
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 
> ---
> 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 intlocked: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_LOCK0x3003
> +#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, siz