Re: [PATCHv2 2/2] x86: SROP mitigation: implement signal cookies

[Andy Lutomirski]

On Feb 8, 2016 3:17 PM, "Scotty Bauer" <sbauer@xxxxxxxxxxxx> wrote:
>
>
>
> On 02/08/2016 02:50 PM, Andy Lutomirski wrote:
> > On Sun, Feb 7, 2016 at 12:10 AM, Scotty Bauer <sbauer@xxxxxxxxxxxx> wrote:
> >>
> >>
> >> On 02/06/2016 11:35 PM, Mika Penttilà wrote:
> >>> Hi,
> >>>
> >>>
> >>> On 07.02.2016 01:39, Scott Bauer wrote:
> >>>> This patch adds SROP mitigation logic to the x86 signal delivery
> >>>> and sigreturn code. The cookie is placed in the unused alignment
> >>>> space above the saved FP state, if it exists. If there is no FP
> >>>> state to save then the cookie is placed in the alignment space above
> >>>> the sigframe.
> >>>>
> >>>> Cc: Abhiram Balasubramanian <abhiram@xxxxxxxxxxx>
> >>>> Signed-off-by: Scott Bauer <sbauer@xxxxxxxxxxxx>
> >>>> ---
> >>>>  arch/x86/ia32/ia32_signal.c        | 63 +++++++++++++++++++++++++---
> >>>>  arch/x86/include/asm/fpu/signal.h  |  1 +
> >>>>  arch/x86/include/asm/sighandling.h |  5 ++-
> >>>>  arch/x86/kernel/fpu/signal.c       | 10 +++++
> >>>>  arch/x86/kernel/signal.c           | 86 +++++++++++++++++++++++++++++++++-----
> >>>>  5 files changed, 146 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> >>>> index 0552884..2751f47 100644
> >>>> --- a/arch/x86/ia32/ia32_signal.c
> >>>> +++ b/arch/x86/ia32/ia32_signal.c
> >>>> @@ -68,7 +68,8 @@
> >>>>  }
> >>>>
> >>>>  static int ia32_restore_sigcontext(struct pt_regs *regs,
> >>>> -                               struct sigcontext_32 __user *sc)
> >>>> +                               struct sigcontext_32 __user *sc,
> >>>> +                               void __user **user_cookie)
> >>>>  {
> >>>>      unsigned int tmpflags, err = 0;
> >>>>      void __user *buf;
> >>>> @@ -105,6 +106,16 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
> >>>>              buf = compat_ptr(tmp);
> >>>>      } get_user_catch(err);
> >>>>
> >>>> +    /*
> >>>> +     * If there is fp state get cookie from the top of the fp state,
> >>>> +     * else get it from the top of the sig frame.
> >>>> +     */
> >>>> +
> >>>> +    if (tmp != 0)
> >>>> +            *user_cookie = compat_ptr(tmp + fpu__getsize(1));
> >>>> +    else
> >>>> +            *user_cookie = NULL;
> >>>> +
> >>>>      err |= fpu__restore_sig(buf, 1);
> >>>>
> >>>>      force_iret();
> >>>> @@ -117,6 +128,7 @@ asmlinkage long sys32_sigreturn(void)
> >>>>      struct pt_regs *regs = current_pt_regs();
> >>>>      struct sigframe_ia32 __user *frame = (struct sigframe_ia32 __user *)(regs->sp-8);
> >>>>      sigset_t set;
> >>>> +    void __user *user_cookie;
> >>>>
> >>>>      if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> >>>>              goto badframe;
> >>>> @@ -129,8 +141,15 @@ asmlinkage long sys32_sigreturn(void)
> >>>>
> >>>>      set_current_blocked(&set);
> >>>>
> >>>> -    if (ia32_restore_sigcontext(regs, &frame->sc))
> >>>> +    if (ia32_restore_sigcontext(regs, &frame->sc, &user_cookie))
> >>>> +            goto badframe;
> >>>> +
> >>>> +    if (user_cookie == NULL)
> >>>> +            user_cookie = compat_ptr((regs->sp - 8) + sizeof(*frame));
> >>>> +
> >>>> +    if (verify_clear_sigcookie(user_cookie))
> >>>>              goto badframe;
> >>>> +
> >>>>      return regs->ax;
> >>>>
> >>>>  badframe:
> >>>> @@ -142,6 +161,7 @@ asmlinkage long sys32_rt_sigreturn(void)
> >>>>  {
> >>>>      struct pt_regs *regs = current_pt_regs();
> >>>>      struct rt_sigframe_ia32 __user *frame;
> >>>> +    void __user *user_cookie;
> >>>>      sigset_t set;
> >>>>
> >>>>      frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
> >>>> @@ -153,7 +173,13 @@ asmlinkage long sys32_rt_sigreturn(void)
> >>>>
> >>>>      set_current_blocked(&set);
> >>>>
> >>>> -    if (ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext))
> >>>> +    if (ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext, &user_cookie))
> >>>> +            goto badframe;
> >>>> +
> >>>> +    if (user_cookie == NULL)
> >>>> +            user_cookie = (void __user *)((regs->sp - 4) + sizeof(*frame));
> >>> regs->sp is already restored so you should use frame instead.
> >>>
> >>> --Mika
> >>>
> >>
> >> Nice catch, thank you. I'm surprised I haven't hit this issue yet.
> >> I've been running these patches for 3 weeks on 2 different machines and
> >> haven't had an issue. I'll submit v3 with this fix a bit later, I want
> >> to see if anyone else has stuff to fix.
> >
> > Is this compatible with existing userspace?  CRIU and DOSEMU seem like
> > things that are likely to blow up to me.
> >
>
> Ugh just looking at CRIU I can already see how this wouldn't work. I'll
> install and run tonight and see what happens. If there are other "swap"
> type userspace apps that save state etc this will probably break them
> without adding patches to save the kernel's cookie/task struct to disk as
> well.
>
> > We may need to make this type of mitigation be opt-in.  I'm already
> > vaguely planning an opt-in mitigation framework for vsyscall runtime
> > disablement, and this could use the same opt-in mechanism.
>
> I'm not against having them hidden behind CONFIG's if thats
> what you were thinking. My only concern is it will make the current
> patches super ugly as some of the function declarations have been modified.
>
> Whats the general approach for having CONFIG'd code, just surrounding the code
> with #ifdef CONFIG_?

I don't mean config.  I'm thinking of having an ELF note to flip it on
per process.  Newer programs (or maybe newer ELF interpreters) would
have a note set with bits indicating which new incompatible features
they're okay with.  A prctl could override it.

--Andy