[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 6 Mar 2018 08:56:07 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Kees Cook <keescook@...omium.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Alexander Popov <alex.popov@...ux.com>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>,
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>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Dan Williams <dan.j.williams@...el.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>,
X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [OLD PATCH] net: recvmsg: Unconditionally zero struct
sockaddr_storage Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in
syscall_trace_enter()
* Kees Cook <keescook@...omium.org> wrote:
> In defense of the series, it's hardly "mindless". :) The primary
> feature is that it has run-time tracking of stack depth to clear only
> the minimum needed portion of the stack.
The stack-tracer has been able to do that for years, right?
> > And it doesn't necessarily generate any worse code.
>
> I agree, though some performance-sensitive subsystem (e.g. networking)
> get very defensive about an always-on stack initialization[2].
> [2] Both these cases, and so many more, are solved with the byref
> initialization plugin, but have been NAKed by -net:
> https://lkml.org/lkml/2013/4/9/641
> https://lkml.org/lkml/2017/10/31/699
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
> struct sockaddr __user *uaddr;
> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>
> + memset(&addr, 0, sizeof(addr));
> msg_sys->msg_name = &addr;
>
> if (MSG_CMSG_COMPAT & flags)
In defense of DaveM and Eric, that networking patch is just pure, utter garbage
and I'll NACK it just as much: it adds an unconditional 128-byte memset() to a hot
path!!
NACKed-by: Ingo Molnar <mingo@...nel.org>
The changelog is also infuriatingly misleading:
> Some protocols do not correctly wipe the contents of the on-stack
> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
> kernel stack contents to userspace. This wipes it unconditionally before
> per-protocol handlers run.
>
> Note that leaks like this are mitigated by building with
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
It's just a scary, passive-aggressive lie about "some protocols" and ignores the
desired case where all protocol handlers are correctly implemented.
Did you *really* expect this patch to be applied? The on-stack struct clearing GCC
plugins (CONFIG_GCC_PLUGIN_STRUCTLEAK*=y) are a nice feature which fix a bad
oversight in the C standard, but this particular unconditional memset() patch is
just garbage.
Also, this characterization of the patch review process of the networking
subsystem:
> though some performance-sensitive subsystem (e.g. networking)
> get very defensive about an always-on stack initialization[2].
... is thus very unfair as well: they didn't NAK or resist the
CONFIG_GCC_PLUGIN_STRUCTLEAK* compiler feature at all, they NAK-ed a poorly
thought out, unconditional memset() which adds unconditional overhead, which is
their job to NAK!
Please refrain from using such unfair pressure tactics to get harebrained security
patches upstream...
Thanks,
Ingo
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.
