lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <56B921FE.9030603@eng.utah.edu>
Date:	Mon, 8 Feb 2016 16:17:18 -0700
From:	Scotty Bauer <sbauer@....utah.edu>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Mika Penttilä <mika.penttila@...tfour.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kernel-hardening@...ts.openwall.com" 
	<kernel-hardening@...ts.openwall.com>, X86 ML <x86@...nel.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Abhiram Balasubramanian <abhiram@...utah.edu>
Subject: Re: [PATCHv2 2/2] x86: SROP mitigation: implement signal cookies



On 02/08/2016 02:50 PM, Andy Lutomirski wrote:
> On Sun, Feb 7, 2016 at 12:10 AM, Scotty Bauer <sbauer@....utah.edu> 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@...utah.edu>
>>>> Signed-off-by: Scott Bauer <sbauer@....utah.edu>
>>>> ---
>>>>  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_?


Thanks,
Scott


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ