kernel-hardening - Re: [PATCH v13 (resend) 2/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45db2e83-5b87-0083-924f-f1ce7c89fe73@linux.com>
Date: Fri, 6 Jul 2018 00:55:48 +0300
From: Alexander Popov <alex.popov@...ux.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: kernel-hardening@...ts.openwall.com, Kees Cook <keescook@...omium.org>,
 PaX Team <pageexec@...email.hu>, Brad Spengler <spender@...ecurity.net>,
 Andy Lutomirski <luto@...nel.org>, Tycho Andersen <tycho@...ho.ws>,
 Laura Abbott <labbott@...hat.com>, Mark Rutland <mark.rutland@....com>,
 Ard Biesheuvel <ard.biesheuvel@...aro.org>, Borislav Petkov <bp@...en8.de>,
 Richard Sandiford <richard.sandiford@....com>,
 Thomas Gleixner <tglx@...utronix.de>, "H . Peter Anvin" <hpa@...or.com>,
 Peter Zijlstra <a.p.zijlstra@...llo.nl>, "Dmitry V . Levin"
 <ldv@...linux.org>, Emese Revfy <re.emese@...il.com>,
 Jonathan Corbet <corbet@....net>, Andrey Ryabinin <aryabinin@...tuozzo.com>,
 "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
 Thomas Garnier <thgarnie@...gle.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Alexei Starovoitov <ast@...nel.org>, Josef Bacik <jbacik@...com>,
 Masami Hiramatsu <mhiramat@...nel.org>, Nicholas Piggin <npiggin@...il.com>,
 Al Viro <viro@...iv.linux.org.uk>, "David S . Miller" <davem@...emloft.net>,
 Ding Tianhong <dingtianhong@...wei.com>, David Woodhouse
 <dwmw@...zon.co.uk>, Josh Poimboeuf <jpoimboe@...hat.com>,
 Steven Rostedt <rostedt@...dmis.org>,
 Dominik Brodowski <linux@...inikbrodowski.net>,
 Juergen Gross <jgross@...e.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Dan Williams <dan.j.williams@...el.com>,
 Dave Hansen <dave.hansen@...ux.intel.com>,
 Mathias Krause <minipli@...glemail.com>,
 Vikas Shivappa <vikas.shivappa@...ux.intel.com>, Kyle Huey
 <me@...ehuey.com>, Dmitry Safonov <dsafonov@...tuozzo.com>,
 Will Deacon <will.deacon@....com>, Arnd Bergmann <arnd@...db.de>,
 Florian Weimer <fweimer@...hat.com>,
 Boris Lukashev <blukashev@...pervictus.com>,
 Andrey Konovalov <andreyknvl@...gle.com>, x86@...nel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v13 (resend) 2/6] x86/entry: Add STACKLEAK erasing the
 kernel stack at the end of syscalls

Hello Ingo,

Thanks for your review! I'll fix the style issues you pointed at.

Please also see my answers below.

On 05.07.2018 11:12, Ingo Molnar wrote:
>> +	  The tradeoff is the performance impact: on a single CPU system kernel
>> +	  compilation sees a 1% slowdown, other systems and workloads may vary
>> +	  and you are advised to test this feature on your expected workload
>> +	  before deploying it.
> 
> Is there a way to patch this out runtime? I.e. if a distro enabled it, is there an 
> easy way to disable much of the overhead without rebooting the kernel?

Hm. We can't completely disable STACKLEAK in runtime, since STACKLEAK gcc plugin
performs compile-time instrumentation of the kernel code. So we can only chop
off a part of functionality, for example, by introducing some variable and
checking it before every stack erasing (additional performance impact), but the
kernel will stay uselessly instrumented. It doesn't look reasonable to me.


>> +	BUG_ON(boundary - kstack_ptr >= THREAD_SIZE);
> 
> Should never happen, right? 

Yes. It can happen only if 'current->lowest_stack' was corrupted.

> If so then please make this:
> 
> 	if (WARN_ON(boundary - kstack_ptr >= THREAD_SIZE))
> 		return;
> 
> or so, to make it non-fatal and to allow users to report it, should it trigger 
> against all expectations.

I've made an experiment. The results:
 1. BUG_ON() here doesn't freeze the kernel output - I see a full 'PANIC: double
fault' report;
 2. WARN_ON() here gives absolutely same 'PANIC: double fault' here.

So there is no reason to introduce the surplus "WARN_ON() + return" logic here.


> Overall I like the interface cleanups in v13, so if these nits are addressed and 
> it becomes possible for (root users) to disable the checking then I suppose this 
> is fine.

Thanks a lot for your positive attitude.

Best regards,
Alexander
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.