|
Message-ID: <32f20d8a-3318-0be1-44c4-1153001e9ad8@linux.com> Date: Fri, 10 Nov 2017 19:59:34 +0300 From: Alexander Popov <alex.popov@...ux.com> To: Kees Cook <keescook@...omium.org> Cc: Tycho Andersen <tycho@...ker.com>, Andy Lutomirski <luto@...nel.org>, kernel-hardening@...ts.openwall.com, PaX Team <pageexec@...email.hu>, Brad Spengler <spender@...ecurity.net>, Ingo Molnar <mingo@...nel.org>, Laura Abbott <labbott@...hat.com>, Mark Rutland <mark.rutland@....com>, Ard Biesheuvel <ard.biesheuvel@...aro.org>, Borislav Petkov <bp@...en8.de>, Thomas Gleixner <tglx@...utronix.de>, "H . Peter Anvin" <hpa@...or.com>, Peter Zijlstra <a.p.zijlstra@...llo.nl>, x86@...nel.org Subject: Re: [PATCH RFC v5 1/5] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls Hello Kees, On 31.10.2017 18:20, Kees Cook wrote: > On Tue, Oct 24, 2017 at 2:30 PM, Alexander Popov <alex.popov@...ux.com> wrote: >> On 23.10.2017 16:17, Tycho Andersen wrote: >>> On Sun, Oct 22, 2017 at 03:22:49AM +0300, Alexander Popov wrote: >>>> The STACKLEAK feature erases the kernel stack before returning from >>>> syscalls. That reduces the information which kernel stack leak bugs can >>>> reveal and blocks some uninitialized stack variable attacks. Moreover, >>>> STACKLEAK provides runtime checks for kernel stack overflow detection. >>>> >>>> This commit introduces the architecture-specific code filling the used >>>> part of the kernel stack with a poison value before returning to the >>>> userspace. Full STACKLEAK feature also contains the gcc plugin which >>>> comes in a separate commit. >>>> >>>> The STACKLEAK feature is ported from grsecurity/PaX. More information at: >>>> https://grsecurity.net/ >>>> https://pax.grsecurity.net/ >>>> >>>> This code is modified from Brad Spengler/PaX Team's code in the last >>>> public patch of grsecurity/PaX based on our understanding of the code. >>>> Changes or omissions from the original code are ours and don't reflect >>>> the original grsecurity/PaX code. >>>> >>>> Signed-off-by: Alexander Popov <alex.popov@...ux.com> >>>> --- >> >> [...] >> >>>> @@ -81,8 +87,10 @@ static long syscall_trace_enter(struct pt_regs *regs) >>>> emulated = true; >>>> >>>> if ((emulated || (work & _TIF_SYSCALL_TRACE)) && >>>> - tracehook_report_syscall_entry(regs)) >>>> + tracehook_report_syscall_entry(regs)) { >>>> + erase_kstack(); >>>> return -1L; >>>> + } >>>> >>>> if (emulated) >>>> return -1L; >>>> @@ -116,9 +124,11 @@ static long syscall_trace_enter(struct pt_regs *regs) >>>> sd.args[5] = regs->bp; >>>> } >>>> >>>> - ret = __secure_computing(&sd); >>>> - if (ret == -1) >>>> + ret = secure_computing(&sd); >>> >>> Is there any reason to switch this from the __ version? This basically >>> adds an additional check on the TIF_SECCOMP flag, but I'm not sure >>> that's intentional with this patch. >> >> Hello Tycho, thanks for your remark! >> >> Initially I took this change from the grsecurity patch, because it looked >> reasonable for me at that time. But now I doubt, thank you. > > Yeah, I'd prefer this stay __secure_computing(). Ok. >> Kees and Andy (Lutomirski), you are the authors of syscall_trace_enter(). Could >> you please have a look at this change? >> >> By the way, it seems that one erase_kstack() call is missing in that function. >> Could you please have a glance at the places where erase_kstack() is called? > > Errr, wouldn't erase_kstack() get called outside of seccomp? (i.e. via > syscall_return_slowpath() or something later in the return path?) Yes, erase_kstack() is called later in entry_SYSCALL_64 just after do_syscall_64. > Or is there some reason for erasing the stack after seccomp processing > but before running the syscall? Ah, it seems that PaX Team is doing that intentionally. I guess I've found the proof in his slides for H2HC conference (slide 38): https://pax.grsecurity.net/docs/PaXTeam-H2HC13-PaX-gcc-plugins.pdf "- Special paths for ptrace/auditing -- Low-level kernel entry/exit paths can diverge for ptrace/auditing and leave interesting information on the stack for the actual syscall code" Do you know which kind of interesting information is mentioned? And again, erase_kstack() is not called before 1 of 4 return statements in syscall_trace_enter(). IMHO it is a bug. Could you please verify that? Thanks! Best regards, Alexander
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.