Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Kees Cook]

On Mon, Mar 12, 2018 at 2:21 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> > Is it possible to implement this "safe automatic variable initialization" language
>> > feature via a GCC plugin robustly, while still keeping code generation sane? (i.e.
>> > no forced allocation of stack slots, etc.) It should be a superset of
>> > CONFIG_GCC_PLUGIN_STRUCTLEAK=y.
>>
>> I think that should be feasible, yes.
>>
>> It would be worth trying whether the current code can be simplified, though: it
>> currently takes care not to add such an initialization if it can already spot
>> one, but I wonder whether just blindly adding one at the start and letting the
>> compiler optimize it away again is safer.
>
> Absolutely - followed by:
>
>  - a good look at the resulting code generation with a reasonably uptodate GCC
>
>  - a look at the resulting code with older GCCs, to make sure there's no
>    pathological code generation
>
> ... because IMHO in the end it is the practical effects that will make (or break)
> any such attempt.
>
> ( BTW., initializing all automatic variables might even speed up certain
>   optimization passes, FWIIW. )

[attempting to merge threads...]

On Tue, Feb 27, 2018 at 3:15 AM, P J P <ppandit@redhat.com> wrote:
> Please see:
>   -> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html
>
> This experimental patch by Florian Weimer(CC'd) adds an option
> '-finit-local-vars' to gcc(1) compiler. When a program(or kernel)
> is built using this option, its automatic(local) variables are
> initialised with zero(0). This could significantly reduce the kernel
> information leakage issues.
>
> A dnf(8) repository of the latest gcc-7.3.1 package built with the above
> patch and kernel-4.15.5 package built using '-finit-local-vars' option
> on Fedora-27 is available below
>
>   -> https://pjp.fedorapeople.org/init-vars/
>
> This same kernel is running on my F27 test machine as I write this.
> There is no slowness or notice-able performance impact as such.

Getting this implemented directly in the compiler would be preferred
over a plugin. (Prototyping may be easier in the plugin, though.)

Considerations:

- the kernel needs a way to "opt out" of this for places where later
functions will do the initialization. Something like
__attribute__((no_automatic_variable_init)) ?

- initialization _must include structure padding_. Without this, we're
only solving part of the exposure. Does -finit-local-vars do this?

- Can we retain the uninitialized variable usage warning? (with an
updated text; maybe "variable used without explicit initialization,
using zero-initialization"?)

It sounds like Fedora-27's default kernel build is already using this
option, is that correct?

-Kees

-- 
Kees Cook
Pixel Security

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Linus Torvalds]

On Mon, Mar 12, 2018 at 9:42 AM, Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Feb 27, 2018 at 3:15 AM, P J P <ppandit@redhat.com> wrote:
>> Please see:
>>   -> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00615.html
>>
>> This experimental patch by Florian Weimer(CC'd) adds an option
>> '-finit-local-vars' to gcc(1) compiler. When a program(or kernel)
>> is built using this option, its automatic(local) variables are
>> initialised with zero(0). This could significantly reduce the kernel
>> information leakage issues.

Oh, I love that patch.

THAT is the kind of thing we should do. It's small, it's trivial, and
it's done early in the parsing stage, so later stages will almost
certainly end up optimizing things away.

The only complaint I have is that the Fortran front-end already has
(some of) that functionality, and calls it something else, and allows
you to specify more exactly what you want to clear.

Florian, are you aware of the Fortran flags?

  -finit-local-zero
  -finit-derived
  -finit-integer=n
  -finit-real=<zero|inf|-inf|nan|snan>
  -finit-logical=<true|false>
  -finit-character=n

they aren't perfect for C, but some of them do make sense at least
conceptually: things like being able to specify whether arrays and
structures should be initialized (-finit-derived is kind of relevant)
might make sense.

In the kernel we'd likely always do it unconditionally (apart from
optional per-function or per-declaration considerations) but I could
easily see some other projects saying "do it for scalars but not for
aggregate types". Because they might want the convenience, but not the
expense.

> Getting this implemented directly in the compiler would be preferred
> over a plugin. (Prototyping may be easier in the plugin, though.)

Absolutely.  I think it's better in gcc simply because I suspect a
number of other projects might use this flag too. A plugin is rather
cumbersome.

That said, the patch is literally right at the PLUGIN_FINISH_DECL, so
doing it as a plugin for older compiler versions, or for just testing,
might still be useful.

> Considerations:
>
> - the kernel needs a way to "opt out" of this for places where later
> functions will do the initialization. Something like
> __attribute__((no_automatic_variable_init)) ?

Yes, that's going to make it more palatable to some uses.

However, somebody who knows gcc needs to verify that it does the right
things when inlining happens.

And honestly, it would be better to make it per-variable, not per
function. That would fit the structure of that patch too, I think. It
iterates over variables anyway to generate the initialization.

> - initialization _must include structure padding_. Without this, we're
> only solving part of the exposure. Does -finit-local-vars do this?

Good point. It uses build_constructor() with an empty constructor, so
it *should* be 100% equivalent to

    struct xyz var = { };

if I understand correctly, but I'm not sure what that will do with padding.

> - Can we retain the uninitialized variable usage warning? (with an
> updated text; maybe "variable used without explicit initialization,
> using zero-initialization"?)

I think that fundamentally goes away, because all later phases see
fully initialized state.

               Linus

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Kees Cook]

On Mon, Mar 12, 2018 at 10:09 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Mar 12, 2018 at 9:42 AM, Kees Cook <keescook@chromium.org> wrote:
>> - initialization _must include structure padding_. Without this, we're
>> only solving part of the exposure. Does -finit-local-vars do this?
>
> Good point. It uses build_constructor() with an empty constructor, so
> it *should* be 100% equivalent to
>
>     struct xyz var = { };
>
> if I understand correctly, but I'm not sure what that will do with padding.

AIUI, this does not guarantee padding initialization (yet another
"undefined behavior"). This is why we've had to sprinkle memset(&var,
0, sizeof(var)) in places where a structure has padding and got
leaked. :(

I assume this may be orthogonal to -finit-local-vars, and maybe we'll
need some -finit-padding or something. (Though, honestly, is there
anyone that wants to get _padding_ correct, but not variable
initialization?)

>> - Can we retain the uninitialized variable usage warning? (with an
>> updated text; maybe "variable used without explicit initialization,
>> using zero-initialization"?)
>
> I think that fundamentally goes away, because all later phases see
> fully initialized state.

I'm fine with it going away, though I share Jeff Law's observation in
Florian's gcc thread that we lose some potentially useful warnings
("oops, it took a while to track down this bug, since that variable
had been zero initialized; I wish I knew that had happened", etc.) And
when the kernel entirely depends on auto-zero-init, we could just add
-Wno-maybe-uninitialized. *shrug*

-Kees

-- 
Kees Cook
Pixel Security

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Linus Torvalds]

On Mon, Mar 12, 2018 at 10:17 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Mar 12, 2018 at 10:09 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>>     struct xyz var = { };
>>
>> I'm not sure what that will do with padding.
>
> AIUI, this does not guarantee padding initialization (yet another
> "undefined behavior"). This is why we've had to sprinkle memset(&var,
> 0, sizeof(var)) in places where a structure has padding and got
> leaked. :(
>
> I assume this may be orthogonal to -finit-local-vars, and maybe we'll
> need some -finit-padding or something. (Though, honestly, is there
> anyone that wants to get _padding_ correct, but not variable
> initialization?)

We would definitely have wanted it over the years, yes. And
conceptually it's a separate issue, so a separate flag makes sense.

But for the kernel, if we have -finit-local-vars, we'd just use that,
so you're right that _we_ don't care, and I don't know if anybody else
does either.

Maybe some security-conscious project would, though. I can definitely
see some project going "we always initialize our stuff ourselves, but
we still worry about padding".

So a separate flag for that would make a lot of sense.

> I'm fine with it going away, though I share Jeff Law's observation in
> Florian's gcc thread that we lose some potentially useful warnings
> ("oops, it took a while to track down this bug, since that variable
> had been zero initialized; I wish I knew that had happened", etc.) And
> when the kernel entirely depends on auto-zero-init, we could just add
> -Wno-maybe-uninitialized. *shrug*

I think it's actually _fundamentally_ hard to give the "might be used
uninitialized" warning together with "-finit-local-vars".

You have to introduce a whole new "zero but counts as uninitialized" model.

So I suspect the gcc people would be better off doing the reverse of
what you suggest: tell people to simply do test builds without
"-finit-local-vars".

For example, for the kernel, we already have that
"CONFIG_COMPILE_TEST" explicitly for the case of "build but don't use"
for build coverage. So it would make sense for us to only use
"-finit-local-vars" when COMPILE_TEST is _not_ set. That would give us
the warnings for our coverage, but all "real" builds would be built
with the initializations.

                    Linus

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Florian Weimer]

On 03/12/2018 06:45 PM, Linus Torvalds wrote:
> On Mon, Mar 12, 2018 at 10:17 AM, Kees Cook<keescook@chromium.org>  wrote:
>> On Mon, Mar 12, 2018 at 10:09 AM, Linus Torvalds
>> <torvalds@linux-foundation.org>  wrote:
>>>      struct xyz var = { };
>>>
>>> I'm not sure what that will do with padding.
>> AIUI, this does not guarantee padding initialization (yet another
>> "undefined behavior"). This is why we've had to sprinkle memset(&var,
>> 0, sizeof(var)) in places where a structure has padding and got
>> leaked. :(
>>
>> I assume this may be orthogonal to -finit-local-vars, and maybe we'll
>> need some -finit-padding or something. (Though, honestly, is there
>> anyone that wants to get_padding_  correct, but not variable
>> initialization?)

> We would definitely have wanted it over the years, yes. And
> conceptually it's a separate issue, so a separate flag makes sense.

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.

Thanks,
Florian

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Kees Cook]

On Wed, Mar 14, 2018 at 7:21 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 03/12/2018 06:45 PM, Linus Torvalds wrote:
>>
>> On Mon, Mar 12, 2018 at 10:17 AM, Kees Cook<keescook@chromium.org>  wrote:
>>>
>>> On Mon, Mar 12, 2018 at 10:09 AM, Linus Torvalds
>>> <torvalds@linux-foundation.org>  wrote:
>>>>
>>>>      struct xyz var = { };
>>>>
>>>> I'm not sure what that will do with padding.
>>>
>>> AIUI, this does not guarantee padding initialization (yet another
>>> "undefined behavior"). This is why we've had to sprinkle memset(&var,
>>> 0, sizeof(var)) in places where a structure has padding and got
>>> leaked. :(
>>>
>>> I assume this may be orthogonal to -finit-local-vars, and maybe we'll
>>> need some -finit-padding or something. (Though, honestly, is there
>>> anyone that wants to get_padding_  correct, but not variable
>>> initialization?)
>
>
>> We would definitely have wanted it over the years, yes. And
>> conceptually it's a separate issue, so a separate flag makes sense.
>
>
> 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.

I think the common expectation is that copying does a full copy from
the source. Zeroing of padding should be guaranteed by the constructor
only, IMO.

-Kees

-- 
Kees Cook
Pixel Security

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Linus Torvalds]

On Wed, Mar 14, 2018 at 7:21 AM, Florian Weimer <fweimer@redhat.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

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Linus Torvalds]

On Wed, Mar 14, 2018 at 9:29 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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

Side note: this obviously implies that your patch already fixes
things, but funnily, it only if the structure didn't have an
initializer.

IOW, with your patch,

        struct a a;

        a = (struct a) ( 1, 2 };

would do the right thing for us because of an odd internal gcc
implementation detail, because that initial definition of 'a' doesn't
have an initializer, so your patch adds an empty one, and gcc seems to
always treat that as "clear the whole stricture". After that, the
structure assignment works fine and leaves the padding zero.

But a plain

        struct a a = { 1, 2 };

doesn't DTRT, and leaves uninitialized bytes to be passed around.

Of course, there might be other special cases that I simply didn't
happen to trigger with my overly stupid example and testing. Maybe
sometimes gcc does structure copies or clearing using the
member-by-member model?

My quick testing seems to indicate that it's _only_ initializers that
cause this, whether they are part of the variable declaration or
afterwards.

              Linus

Re: Fully initialized stack usage (was Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter())

[Florian Weimer]

On 03/12/2018 06:09 PM, Linus Torvalds wrote:
> The only complaint I have is that the Fortran front-end already has
> (some of) that functionality, and calls it something else, and allows
> you to specify more exactly what you want to clear.
> 
> Florian, are you aware of the Fortran flags?
> 
>    -finit-local-zero
>    -finit-derived
>    -finit-integer=n
>    -finit-real=<zero|inf|-inf|nan|snan>
>    -finit-logical=<true|false>
>    -finit-character=n
> 
> they aren't perfect for C, but some of them do make sense at least
> conceptually: things like being able to specify whether arrays and
> structures should be initialized (-finit-derived is kind of relevant)
> might make sense.

I think I was aware of those when I put together the GCC patch, but it's 
been a while, so I don't know for sure.

>> - the kernel needs a way to "opt out" of this for places where later
>> functions will do the initialization. Something like
>> __attribute__((no_automatic_variable_init)) ?
> 
> Yes, that's going to make it more palatable to some uses.
> 
> However, somebody who knows gcc needs to verify that it does the right
> things when inlining happens.

Given that the initializer is added in the parser, we should be able to 
look at the current function definition and get the attribute from 
there.  Only if you want to do some special for nested functions, it's 
going to be tricky.  Once the initializer is there, it will survive 
inlining, of course.

> Good point. It uses build_constructor() with an empty constructor, so
> it *should* be 100% equivalent to
> 
>     struct xyz var = { };
> 
> if I understand correctly, but I'm not sure what that will do with padding.

There is no guarantee WRT padding if I understand this correctly.  But 
not initializing padding would be less efficient, so GCC should do the 
right thing here.  But this really needs to be careful checking by 
someone familiar with the middle-end and the relevant backends.

Note that this patch was only for measuring the performance impact, so I 
didn't polish these finer points.

>> - Can we retain the uninitialized variable usage warning? (with an
>> updated text; maybe "variable used without explicit initialization,
>> using zero-initialization"?)
> 
> I think that fundamentally goes away, because all later phases see
> fully initialized state.

Yes, retaining the warning will need a completely different approach.

Thanks,
Florian