Re: [PATCH v2 27/39] x86/cet/shstk: Handle signals for shadow stack

[Edgecombe, Rick P]

Kees, sorry for the delayed response. There was so much feedback, I
missed responding to some.

On Mon, 2022-10-03 at 13:52 -0700, Kees Cook wrote:
> On Thu, Sep 29, 2022 at 03:29:24PM -0700, Rick Edgecombe wrote:
> > From: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> > 
> > When a signal is handled normally the context is pushed to the
> > stack
> > before handling it. For shadow stacks, since the shadow stack only
> > track's
> > return addresses, there isn't any state that needs to be pushed.
> > However,
> > there are still a few things that need to be done. These things are
> > userspace visible and which will be kernel ABI for shadow stacks.
> > 
> > One is to make sure the restorer address is written to shadow
> > stack, since
> > the signal handler (if not changing ucontext) returns to the
> > restorer, and
> > the restorer calls sigreturn. So add the restorer on the shadow
> > stack
> > before handling the signal, so there is not a conflict when the
> > signal
> > handler returns to the restorer.
> > 
> > The other thing to do is to place some type of checkable token on
> > the
> > thread's shadow stack before handling the signal and check it
> > during
> > sigreturn. This is an extra layer of protection to hamper attackers
> > calling sigreturn manually as in SROP-like attacks.
> > 
> > For this token we can use the shadow stack data format defined
> > earlier.
> > Have the data pushed be the previous SSP. In the future the
> > sigreturn
> > might want to return back to a different stack. Storing the SSP
> > (instead
> > of a restore offset or something) allows for future functionality
> > that
> > may want to restore to a different stack.
> > 
> > So, when handling a signal push
> >  - the SSP pointing in the shadow stack data format
> >  - the restorer address below the restore token.
> > 
> > In sigreturn, verify SSP is stored in the data format and pop the
> > shadow
> > stack.
> > 
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> > Cc: Cyrill Gorcunov <gorcunov@xxxxxxxxx>
> > Cc: Florian Weimer <fweimer@xxxxxxxxxx>
> > Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > 
> > ---
> > 
> > v2:
> >  - Switch to new shstk signal format
> > 
> > v1:
> >  - Use xsave helpers.
> >  - Expand commit log.
> > 
> > Yu-cheng v27:
> >  - Eliminate saving shadow stack pointer to signal context.
> > 
> > Yu-cheng v25:
> >  - Update commit log/comments for the sc_ext struct.
> >  - Use restorer address already calculated.
> >  - Change CONFIG_X86_CET to CONFIG_X86_SHADOW_STACK.
> >  - Change X86_FEATURE_CET to X86_FEATURE_SHSTK.
> >  - Eliminate writing to MSR_IA32_U_CET for shadow stack.
> >  - Change wrmsrl() to wrmsrl_safe() and handle error.
> > 
> >  arch/x86/ia32/ia32_signal.c |   1 +
> >  arch/x86/include/asm/cet.h  |   5 ++
> >  arch/x86/kernel/shstk.c     | 126 ++++++++++++++++++++++++++++++
> > ------
> >  arch/x86/kernel/signal.c    |  10 +++
> >  4 files changed, 123 insertions(+), 19 deletions(-)
> > 
> > diff --git a/arch/x86/ia32/ia32_signal.c
> > b/arch/x86/ia32/ia32_signal.c
> > index c9c3859322fa..88d71b9de616 100644
> > --- a/arch/x86/ia32/ia32_signal.c
> > +++ b/arch/x86/ia32/ia32_signal.c
> > @@ -34,6 +34,7 @@
> >  #include <asm/sigframe.h>
> >  #include <asm/sighandling.h>
> >  #include <asm/smap.h>
> > +#include <asm/cet.h>
> >  
> >  static inline void reload_segments(struct sigcontext_32 *sc)
> >  {
> > diff --git a/arch/x86/include/asm/cet.h
> > b/arch/x86/include/asm/cet.h
> > index 924de99e0c61..8c6fab9f402a 100644
> > --- a/arch/x86/include/asm/cet.h
> > +++ b/arch/x86/include/asm/cet.h
> > @@ -6,6 +6,7 @@
> >  #include <linux/types.h>
> >  
> >  struct task_struct;
> > +struct ksignal;
> >  
> >  struct thread_shstk {
> >  	u64	base;
> > @@ -22,6 +23,8 @@ int shstk_alloc_thread_stack(struct task_struct
> > *p, unsigned long clone_flags,
> >  void shstk_free(struct task_struct *p);
> >  int shstk_disable(void);
> >  void reset_thread_shstk(void);
> > +int setup_signal_shadow_stack(struct ksignal *ksig);
> > +int restore_signal_shadow_stack(void);
> >  #else
> >  static inline long cet_prctl(struct task_struct *task, int option,
> >  		      unsigned long features) { return -EINVAL; }
> > @@ -33,6 +36,8 @@ static inline int shstk_alloc_thread_stack(struct
> > task_struct *p,
> >  static inline void shstk_free(struct task_struct *p) {}
> >  static inline int shstk_disable(void) { return -EOPNOTSUPP; }
> >  static inline void reset_thread_shstk(void) {}
> > +static inline int setup_signal_shadow_stack(struct ksignal *ksig)
> > { return 0; }
> > +static inline int restore_signal_shadow_stack(void) { return 0; }
> >  #endif /* CONFIG_X86_SHADOW_STACK */
> >  
> >  #endif /* __ASSEMBLY__ */
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > index 8904aef487bf..04442134aadd 100644
> > --- a/arch/x86/kernel/shstk.c
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -227,41 +227,129 @@ static int get_shstk_data(unsigned long
> > *data, unsigned long __user *addr)
> >  }
> >  
> >  /*
> > - * Verify the user shadow stack has a valid token on it, and then
> > set
> > - * *new_ssp according to the token.
> > + * Create a restore token on shadow stack, and then push the user-
> > mode
> > + * function return address.
> >   */
> > -static int shstk_check_rstor_token(unsigned long *new_ssp)
> > +static int shstk_setup_rstor_token(unsigned long ret_addr,
> > unsigned long *new_ssp)
> 
> Oh, hrm. Prior patch defines shstk_check_rstor_token() and
> doesn't call it. This patch removes it. :P Can you please remove
> shstk_check_rstor_token() from the prior patch?

Yes, this function is not needed until the alt shadow stack stuff. It
got all mangled across earlier patches. I removed it all together now.
Thanks.

> 
> >  {
> > -	unsigned long token_addr;
> > -	unsigned long token;
> > +	unsigned long ssp, token_addr;
> > +	int err;
> > +
> > +	if (!ret_addr)
> > +		return -EINVAL;
> > +
> > +	ssp = get_user_shstk_addr();
> > +	if (!ssp)
> > +		return -EINVAL;
> > +
> > +	err = create_rstor_token(ssp, &token_addr);
> > +	if (err)
> > +		return err;
> > +
> > +	ssp = token_addr - sizeof(u64);
> > +	err = write_user_shstk_64((u64 __user *)ssp, (u64)ret_addr);
> > +
> > +	if (!err)
> > +		*new_ssp = ssp;
> > +
> > +	return err;
> > +}
> > +
> > +static int shstk_push_sigframe(unsigned long *ssp)
> > +{
> > +	unsigned long target_ssp = *ssp;
> > +
> > +	/* Token must be aligned */
> > +	if (!IS_ALIGNED(*ssp, 8))
> > +		return -EINVAL;
> >  
> > -	token_addr = get_user_shstk_addr();
> > -	if (!token_addr)
> > +	if (!IS_ALIGNED(target_ssp, 8))
> >  		return -EINVAL;
> >  
> > -	if (get_user(token, (unsigned long __user *)token_addr))
> > +	*ssp -= SS_FRAME_SIZE;
> > +	if (put_shstk_data((void *__user)*ssp, target_ssp))
> >  		return -EFAULT;
> >  
> > -	/* Is mode flag correct? */
> > -	if (!(token & BIT(0)))
> > +	return 0;
> > +}
> > +
> > +
> > +static int shstk_pop_sigframe(unsigned long *ssp)
> > +{
> > +	unsigned long token_addr;
> > +	int err;
> > +
> > +	err = get_shstk_data(&token_addr, (unsigned long __user
> > *)*ssp);
> > +	if (unlikely(err))
> > +		return err;
> > +
> > +	/* Restore SSP aligned? */
> > +	if (unlikely(!IS_ALIGNED(token_addr, 8)))
> >  		return -EINVAL;
> 
> Why doesn't this always fail, given BIT(0) being set? I don't see it
> getting cleared until the end of this function.

Because it isn't a normal token, it was an address in the "data format"
that had bit 63 set. Then bit 63 was cleared, making it a normal
address.

> 
> >  
> > -	/* Is busy flag set? */
> > -	if (token & BIT(1))
> > +	/* SSP in userspace? */
> > +	if (unlikely(token_addr >= TASK_SIZE_MAX))
> >  		return -EINVAL;
> 
> BIT(63) already got cleared by here (in get_shstk_data(), but yes,
> this is still a reasonable check.

Good point. I guess I can leave it. Thanks.

> 
> >  
> > -	/* Mask out flags */
> > -	token &= ~3UL;
> > +	*ssp = token_addr;
> > +
> > +	return 0;
> > +}
> 
>