Re: [PATCH v7 32/41] x86/shstk: Handle signals for shadow stack

[Edgecombe, Rick P]

On Thu, 2023-03-09 at 18:02 +0100, Borislav Petkov wrote:
> On Mon, Feb 27, 2023 at 02:29:48PM -0800, Rick Edgecombe wrote:
> > From: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> > 
> > When a signal is handled normally the context is pushed to the
> > stack
> 
> s/normally //

It is trying to say "When a signal is handled without shadow stack, the
context is pushed to the stack"

> 
> > before handling it. For shadow stacks, since the shadow stack only
> > track's
> 
> "tracks"

Right.

> 
> > 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.
> 
> "visible to userspace"

Sure.

> 
> s/which //

Ok.

> 
> > 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.
> 
> 		^^^
> 
> Please use passive voice in your commit message: no "we" or "I", etc.

Argh, right. And it looks like I wrote this one.

> 
> > 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.
> 
> ...
> 
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > index 13c02747386f..40f0a55762a9 100644
> > --- a/arch/x86/kernel/shstk.c
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -232,6 +232,104 @@ static int get_shstk_data(unsigned long
> > *data, unsigned long __user *addr)
> >  	return 0;
> >  }
> >  
> > +static int shstk_push_sigframe(unsigned long *ssp)
> > +{
> > +	unsigned long target_ssp = *ssp;
> > +
> > +	/* Token must be aligned */
> > +	if (!IS_ALIGNED(*ssp, 8))
> > +		return -EINVAL;
> > +
> > +	if (!IS_ALIGNED(target_ssp, 8))
> > +		return -EINVAL;
> 
> Those two statements are identical AFAICT.

Uhh, yes they are. Not sure what happened here.

> 
> > +	*ssp -= SS_FRAME_SIZE;
> > +	if (put_shstk_data((void *__user)*ssp, target_ssp))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> 
>