[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jL2TTUs_LbjzVoVfP3YXsgCYcSXveo2dS9H-FiSdPVqMw@mail.gmail.com>
Date: Tue, 31 Oct 2017 08:20:42 -0700
From: Kees Cook <keescook@...omium.org>
To: Alexander Popov <alex.popov@...ux.com>
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
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().
> 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?)
Or is there some reason for erasing the stack after seccomp processing
but before running the syscall?
-Kees
--
Kees Cook
Pixel Security
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.
