Re: [PATCH 11/17] x86/cet: Sync with Linux kernel 6.6 shadow stack interface

["H.J. Lu" <hjl.tools@gmail.com>]

On Mon, Dec 11, 2023 at 3:34 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 12/06/2023 09:20, H.J. Lu wrote:
> > Sync with Linux kernel 6.6 shadow stack interface.  Since only x86-64 is
> > supported, i386 shadow stack codes are unchanged and CET shouldn't be
> > enabled for i386.
> >
> > 1. When the shadow stack base in TCB is unset, the default shadow stack
> > is in use.  Use the current shadow stack pointer as the marker for the
> > default shadow stack.
>
> what is the role of ssp_base in the tcb?

It is used to identify if the current stack is the same as the target
shadow stack when switching ucontexts.  If yes, INCSSP will
be used to unwind shadow stack.  Otherwise, shadow stack
restore token will be used.

> > 2. Allocate shadow stack with the map_shadow_stack syscall.
> > 3. Rename arch_prctl CET commands to ARCH_SHSTK_XXX.
> > 4. Rewrite the CET control functions with the current kernel shadow stack
> > interface.
> >
> > Since CET is no longer enabled by kernel, a separate patch will enable
> > shadow stack during startup.
> ...
> > +/* NB: This can be treated as a syscall by caller.  */
> > +
> > +#ifndef __x86_64__
> > +__attribute__ ((regparm (2)))
> > +#endif
> > +long int
> > +__allocate_shadow_stack (size_t stack_size,
> > +                      shadow_stack_size_t *child_stack)
> > +{
> > +#ifdef __NR_map_shadow_stack
> > +  size_t shadow_stack_size
> > +    = stack_size >> STACK_SIZE_TO_SHADOW_STACK_SIZE_SHIFT;
> > +  /* Align shadow stack to 8 bytes.  */
> > +  shadow_stack_size = ALIGN_UP (shadow_stack_size, 8);
>
> since sigaltstack shares shadow stack with the current context
> in the thread, this should include the shadow stack requirement
> for signal handlers too. otherwise a user can use makecontext
> to fill up a shadow stack such that a stack overflow signal
> handler cannot run (presumably a crash handler should be able
> to handle crashes in all situations).
>
> i think a fixed fixed size is enough to cover for reasonable
> signal handler (e.g. ~20 stack frames).

It sounds reasonable.

>
> > +  void *shadow_stack = (void *)INLINE_SYSCALL_CALL
> > +    (map_shadow_stack, NULL, shadow_stack_size, SHADOW_STACK_SET_TOKEN);
> > +  /* Report the map_shadow_stack error.  */
> > +  if (shadow_stack == MAP_FAILED)
> > +    return -errno;
> > +
> > +  /* Save the shadow stack base and size on child stack.  */
> > +  child_stack[0] = (uintptr_t) shadow_stack;
> > +  child_stack[1] = shadow_stack_size;
> > +
> > +  return 0;
> > +#else
> > +  return -ENOSYS;
> > +#endif
> > +}
> ...
> > --- a/sysdeps/unix/sysv/linux/x86_64/makecontext.c
> > +++ b/sysdeps/unix/sysv/linux/x86_64/makecontext.c
> > @@ -24,6 +24,8 @@
> >  # include <pthread.h>
> >  # include <libc-pointer-arith.h>
> >  # include <sys/prctl.h>
> > +# include <sys/mman.h>
> > +# include <allocate-shadow-stack.h>
> >  #endif
> >
> >  #include "ucontext_i.h"
> > @@ -88,23 +90,24 @@ __makecontext (ucontext_t *ucp, void (*func) (void), int argc, ...)
> >    if ((feature_1 & X86_FEATURE_1_SHSTK) != 0)
> >      {
> >        /* Shadow stack is enabled.  We need to allocate a new shadow
> > -         stack.  */
> > -      unsigned long ssp_size = (((uintptr_t) sp
> > -                              - (uintptr_t) ucp->uc_stack.ss_sp)
> > -                             >> STACK_SIZE_TO_SHADOW_STACK_SIZE_SHIFT);
> > -      /* Align shadow stack to 8 bytes.  */
> > -      ssp_size = ALIGN_UP (ssp_size, 8);
> > -
> > -      ucp->__ssp[1] = ssp_size;
> > -      ucp->__ssp[2] = ssp_size;
> > -
> > -      /* Call __push___start_context to allocate a new shadow stack,
> > -      push __start_context onto the new stack as well as the new
> > -      shadow stack.  NB: After __push___start_context returns,
> > +         stack.  NB:
> >          ucp->__ssp[0]: The new shadow stack pointer.
> >          ucp->__ssp[1]: The base address of the new shadow stack.
> >          ucp->__ssp[2]: The size of the new shadow stack.
> >         */
> > +      long int ret
> > +     = __allocate_shadow_stack (((uintptr_t) sp
> > +                                 - (uintptr_t) ucp->uc_stack.ss_sp),
> > +                                &ucp->__ssp[1]);
>
> this allocation in glibc does not seem to be freed in glibc.
>
> so normal makecontext use will leak the shadow stack.
>
> (e.g. this is true even if the makecontext function returns
> and thus the shadow stack lifetime ends).

Correct.  Since there is no function to explicitly release ucontext,
there is no place to release shadow stack and shadow stack will
be leaked.

> > +      if (ret != 0)
> > +     {
> > +       /* FIXME: What should we do?  */
> > +       abort ();
>
> makecontext cannot report errors, so we can make this an
> error in setcontext/swapcontext (not sure if that's
> better in practice, but it is more compatible with the
> posix api).
>
> > +     }
> > +
> > +      ucp->__ssp[0] = ucp->__ssp[1] + ucp->__ssp[2] - 8;
> > +      /* Call __push___start_context to push __start_context onto the new
> > +      stack as well as the new shadow stack.  */
> >        __push___start_context (ucp);
> ...
> > --- a/sysdeps/x86_64/nptl/tls.h
> > +++ b/sysdeps/x86_64/nptl/tls.h
> > @@ -60,7 +60,7 @@ typedef struct
> >    void *__private_tm[4];
> >    /* GCC split stack support.  */
> >    void *__private_ss;
> > -  /* The lowest address of shadow stack,  */
> > +  /* The marker for the current shadow stack.  */
> >    unsigned long long int ssp_base;
>
> is this abi between libc/unwinder or compiler or something else?

This is used by ucontext functions internally in glibc.

Thanks.

-- 
H.J.