Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+55aFx45SM4VxD-V4qLo8=hxqYtGH+qBeB74KekwCkJK7D=xA@mail.gmail.com>
Date: Wed, 14 Mar 2018 09:29:07 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Florian Weimer <fweimer@...hat.com>
Cc: Kees Cook <keescook@...omium.org>, Ingo Molnar <mingo@...nel.org>, 
	P J P <pjp@...oraproject.org>, Ard Biesheuvel <ard.biesheuvel@...aro.org>, 
	Steven Rostedt <rostedt@...dmis.org>, Arnd Bergmann <arnd@...db.de>, 
	Daniel Micay <danielmicay@...il.com>, 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>, 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: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7]
 x86/entry: Erase kernel stack in syscall_trace_enter())

On Wed, Mar 14, 2018 at 7:21 AM, Florian Weimer <fweimer@...hat.com> wrote:
>
> What would be the model for the kernel?  Write zero to the padding
> initially, and on copying structs, make sure that you either copy the
> padding from the source, or clear the target?
>
> Clearing the padding while copying might be somewhat expensive.

Oh, no, I wouldn't expect that at all. A structure copy assignment
should act as a memcpy.

I think the semantics should be that any padding is basically "unnamed content".

So copying copies the whole struct, padding and all.

Just as an example:

  struct a {
        char a;
        int b;
  };

  int fn(struct a *);

  int test(struct a *dummy)
  {
        struct a a1 = { 1, 2 };
        struct a a2 = { };
        struct a a3 = *dummy;

        a2.b = 123;

        return fn(&a1) + fn(&a2) + fn(&a3);
  }

and you can see examples of the bug, but also examples of right
behavior. My current gcc (7.3.1-2 from F27) does

        movb    $1, 8(%rsp)
        movl    $2, 12(%rsp)

for that first one - which is problematic because it will pass
uninitialized state to 'fn()'.

The second one results in good code:

        movl    $0, 16(%rsp)
        movl    $123, 20(%rsp)

which is good and doesn't leave the padding uninitialized, and the
third one results in

        movq    (%rdi), %rax
        movq    %rax, 24(%rsp)

which is again fine.

So it mostly works as expected, but things that initialize the
structure one member at a time end up entirely skipping the padding,
resulting in uninitialized parts on the stack being passed around.

That's why I say that I think the semantics we would want is that
padding acts as unnamed entries, ie it should _act_ as if the
structure declaration was

  struct a {
        char a;
        char __padding[3];
        int b;
  };

and that initialization of 'a1' then only iterated over the actual
named entries, so that a1 = { 1, 2 } initializer would act like:

        struct a a1 = { .a = 1, .b = 2 };

which currently makes gcc generate

        movl    $0, 8(%rsp)
        movl    $2, 12(%rsp)
        movb    $1, 8(%rsp)

and initializes things properly. Ok, so gcc is admittedly oddly
_stupid_ about it (a little bit clever, a little bit stupid), but that
would still be quite acceptablke for the kernel.

Note that the "unnamed padding" fields would automatically do the
right thing for structure copies and everything else. It would
literally only affect the case of assigning fields one by one, and
force the padding to be zeroed too.

So as far as implementation would go, I would suggest that one
possible implementation is literally to create those dummy padding
entries in the struct definition, and add a flag for "real" entries,
and make unnamed initializers skip padding ones.

But I haven't looked at how the initializer generation works in gcc,
there may be some clever place that already knows about "oh, we
skipped over padding, so let's just initialize it".

                    Linus

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.