|
Message-ID: <CAK8P3a0bWWZbFW5AFag=vRKC=PbCbwaLNPNiyk1ziJk=fTe0nQ@mail.gmail.com> Date: Tue, 6 Mar 2018 21:42:54 +0100 From: Arnd Bergmann <arnd@...db.de> To: Linus Torvalds <torvalds@...ux-foundation.org> Cc: Ard Biesheuvel <ard.biesheuvel@...aro.org>, Daniel Micay <danielmicay@...il.com>, Ingo Molnar <mingo@...nel.org>, Kees Cook <keescook@...omium.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>, 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>, X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter() On Tue, Mar 6, 2018 at 8:16 PM, Linus Torvalds <torvalds@...ux-foundation.org> wrote: > On Tue, Mar 6, 2018 at 11:07 AM, Ard Biesheuvel > <ard.biesheuvel@...aro.org> wrote: >> >> The compiler usually does a pretty good job of detecting which scalar >> variables are never initialized by regular assignment. > > Sure, but "usually" is not really the same as always. Sometimes scalar > types are initialized by passing a reference to them too. > >> We could easily extend this to scalar and array types, but we'd first >> need to see what the performance impact is, because I don't think it >> will be negligible. > > For scalar types, I suspect it will be entirely unnoticeable, because > they are not only small, but it's rare that this kind of "initialize > by passing a reference" happens in the first place. A lot of the scalar variables with actual bugs are missed by the gcc warnings, because it never allocates a stack slot for examples like int f(int c) { int i; if (c) return i; /* uninitialized return */ asm volatile("" : "=r" (i)); /* gcc sees that 'i' escapes here */ return 0; } int g(int c) { int i; if (c) /* gcc optimizes out the condition as nothing else sets i */ i = 1; return i; } At -O2 optimization level, these fail to produce a warning, and they won't ever leak stack data, but they are still undefined behavior and don't do what the author intended. Forcing gcc to allocate a stack slot and zero-initialize it should find many bugs by adding valid warnings, but also add lots of false positives as well as prevent important optimizations in other places that are actually well-defined. > For arrays, I agree. We very well may have arrays that we really want > to do magic things about. But even then I'd rather have a "don't > initialize this" flag for critical stuff that really *does* get > initialized some other way. Then we can grep for those things and be > more careful. > > If somebody has big arrays on the stack, that's often a problem > anyway. It may be common in non-kernel code, but kernel code is very > special. I can think of a few cases that are important, this one is well-known: int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct timespec64 *end_time) { .... /* Allocate small arguments on the stack to save memory and be faster */ long stack_fds[SELECT_STACK_ALLOC/sizeof(long)]; Another case I came across very recently with a similar optimization is: int ib_process_cq_direct(struct ib_cq *cq, int budget) { struct ib_wc wcs[IB_POLL_BATCH]; In both cases, the stack variables are chosen to be just under the CONFIG_FRAME_WARN limit to avoid a memory allocation in the fast path. If we add an explicit zero initialization, that optimization may turn out counterproductive, but a "don't initialize" flag would be sufficient to deal with them one at a time. There is also the really scary code like: #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \ char __##name##_desc[sizeof(struct skcipher_request) + \ crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \ struct skcipher_request *name = (void *)__##name##_desc that implements an alloca() through a dynamic array for storing a variable-sized structure on the stack. These are usually small, but the size is driver specific and some can be surprisingly big, e.g. struct ccp_aes_req_ctx, struct hifn_request_context, or struct iproc_reqctx_s. If we can come up with a way to avoid those, we could actually enable -Wstack-usage=${CONFIG_FRAME_WARN} to warn for any functions with dynamic stack allocation. Arnd
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.