[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <59b5e578-cf68-4cc2-ebb7-c2fdf873b79d@linux.com>
Date: Wed, 25 Oct 2017 00:30:38 +0300
From: Alexander Popov <alex.popov@...ux.com>
To: Tycho Andersen <tycho@...ker.com>, keescook@...omium.org,
Andy Lutomirski <luto@...nel.org>
Cc: kernel-hardening@...ts.openwall.com, pageexec@...email.hu,
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 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.
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?
Thanks in advance.
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.
