Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc

[Kees Cook <keescook@chromium.org>]

On Fri, Jun 11, 2021 at 01:04:09PM +0200, Richard Biener wrote:
> On Tue, 8 Jun 2021, Kees Cook wrote:
> 
> > On Tue, Jun 08, 2021 at 09:41:38AM +0200, Richard Biener wrote:
> > > On Mon, 7 Jun 2021, Qing Zhao wrote:
> > > 
> > > > Hi, 
> > > > 
> > > > > On Jun 7, 2021, at 2:53 AM, Richard Biener <rguenther@suse.de> wrote:
> > > > > 
> > > > >> 
> > > > >> To address the above suggestion:
> > > > >> 
> > > > >> My study shows: the call to __builtin_clear_padding is expanded during gimplification phase.
> > > > >> And there is no __bultin_clear_padding expanding during rtx expanding phase.
> > > > >> However, for -ftrivial-auto-var-init, padding initialization should be done both in gimplification phase and rtx expanding phase.
> > > > >> since the __builtin_clear_padding might not be good for rtx expanding, reusing __builtin_clear_padding might not work.
> > > > >> 
> > > > >> Let me know if you have any more comments on this.
> > > > > 
> > > > > Yes, I didn't suggest to literally emit calls to __builtin_clear_padding 
> > > > > but instead to leverage the lowering code, more specifically share the
> > > > > code that figures _what_ is to be initialized (where the padding is)
> > > > > and eventually the actual code generation pieces.  That might need some
> > > > > refactoring but the code where padding resides should be present only
> > > > > a single time (since it's quite complex).
> > > > 
> > > > Okay, I see your point here.
> > > > 
> > > > > 
> > > > > Which is also why I suggested to split out the padding initialization
> > > > > bits to a separate patch (and option).
> > > > 
> > > > Personally, I am okay with splitting padding initialization from this current patch,
> > > > Kees, what’s your opinion on this? i.e, the current -ftrivial-auto-var-init will NOT initialize padding, we will add another option to 
> > > > Explicitly initialize padding.
> > > 
> > > It would also be possible to have -fauto-var-init, -fauto-var-init-padding
> > > and have -ftrivial-auto-var-init for clang compatibility enabling both.
> > 
> > Sounds good to me!
> > 
> > > Or -fauto-var-init={zero,pattern,padding} and allow
> > > -fauto-var-init=pattern,padding to be specified.  Note there's also
> > > padding between auto variables on the stack - that "trailing"
> > > padding isn't initialized either?  (yes, GCC sorts variables to minimize
> > > that padding)  For example for
> > > 
> > > void foo()
> > > {
> > >   char a[3];
> > >   bar (a);
> > > }
> > > 
> > > there's 12 bytes padding after 'a', shouldn't we initialize that?  If not,
> > > why's other padding important to be initialized?
> > 
> > This isn't a situation that I'm aware of causing real-world problems.
> > The issues have all come from padding within an addressable object. I
> > haven't tested Clang's behavior on this (and I have no kernel tests for
> > this padding), but I do check for trailing padding, like:
> > 
> > struct test_trailing_hole {
> >         char *one;
> >         char *two;
> >         char *three;
> >         char four;
> >         /* "sizeof(unsigned long) - 1" byte padding hole here. */
> > };
> 
> Any justification why tail padding for
> 
>  struct foo { double x; char x[3]; } a;
> 
> is important but not for
> 
>  char x[3];
> 
> ?  It does look like an odd inconsistency to me.

The problem is with sizeof() and the various compounding results related
to it. Namely, things that do whole-struct copies (which is unfortunately
common in the kernel) will include the padding for "a" since it is within
the object, as represented by sizeof(), but not for x:

#include <stdio.h>

int main(void)
{
    struct foo { double y; char x[3]; } a;
    char x[3];

    printf("a: %zu (a.y: %zu, a.x: %zu)\n", sizeof(a), sizeof(a.y), sizeof(a.x));
    printf("x: %zu\n", sizeof(x));

    return 0;
}

a: 16 (a.y: 8, a.x: 3)
x: 3

And it gets worse with structs-within-structs, etc.

-- 
Kees Cook