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;
> > +}
>
>
