[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 8 Feb 2016 21:51:23 -0800
From: Andy Lutomirski <luto@...capital.net>
To: Scotty Bauer <sbauer@....utah.edu>
Cc: Mika Penttilä <mika.penttila@...tfour.com>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, X86 ML <x86@...nel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Abhiram Balasubramanian <abhiram@...utah.edu>, Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCHv2 2/2] x86: SROP mitigation: implement signal cookies
On Feb 8, 2016 3:17 PM, "Scotty Bauer" <sbauer@....utah.edu> wrote:
>
>
>
> 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_?
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
