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

[Kees Cook <keescook@chromium.org>]

On Wed, Jul 14, 2021 at 07:30:45PM +0000, Qing Zhao wrote:
> Hi, Kees,
> 
> 
> > On Jul 14, 2021, at 2:11 PM, Kees Cook <keescook@chromium.org> wrote:
> > 
> > On Wed, Jul 14, 2021 at 02:09:50PM +0000, Qing Zhao wrote:
> >> Hi, Richard,
> >> 
> >>> On Jul 14, 2021, at 2:14 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> >>> 
> >>> On Wed, Jul 14, 2021 at 1:17 AM Qing Zhao <qing.zhao@oracle.com> wrote:
> >>>> 
> >>>> Hi, Kees,
> >>>> 
> >>>> I took a look at the kernel testing case you attached in the previous email, and found the testing failed with the following case:
> >>>> 
> >>>> #define INIT_STRUCT_static_all          = { .one = arg->one,            \
> >>>>                                           .two = arg->two,            \
> >>>>                                           .three = arg->three,        \
> >>>>                                           .four = arg->four,          \
> >>>>                                       }
> >>>> 
> >>>> i.e, when the structure type auto variable has been explicitly initialized in the source code.  -ftrivial-auto-var-init in the 4th version
> >>>> does not initialize the paddings for such variables.
> >>>> 
> >>>> But in the previous version of the patches ( 2 or 3), -ftrivial-auto-var-init initializes the paddings for such variables.
> >>>> 
> >>>> I intended to remove this part of the code from the 4th version of the patch since the implementation for initializing such paddings is completely different from
> >>>> the initializing of the whole structure as a whole with memset in this version of the implementation.
> >>>> 
> >>>> If we really need this functionality, I will add another separate patch for this additional functionality, but not with this patch.
> >>>> 
> >>>> Richard, what’s your comment and suggestions on this?
> >>> 
> >>> I think this can be addressed in the gimplifier by adjusting
> >>> gimplify_init_constructor to clear
> >>> the object before the initialization (if it's not done via aggregate
> >>> copying).  
> >> 
> >> I did this in the previous versions of the patch like the following:
> >> 
> >> @@ -5001,6 +5185,17 @@ gimplify_init_constructor (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
> >> 	  /* If a single access to the target must be ensured and all elements
> >> 	     are zero, then it's optimal to clear whatever their number.  */
> >> 	  cleared = true;
> >> +	else if (flag_trivial_auto_var_init > AUTO_INIT_UNINITIALIZED
> >> +		 && !TREE_STATIC (object)
> >> +		 && type_has_padding (type))
> >> +	  /* If the user requests to initialize automatic variables with
> >> +	     paddings inside the type, we should initialize the paddings too.
> >> +	     C guarantees that brace-init with fewer initializers than members
> >> +	     aggregate will initialize the rest of the aggregate as-if it were
> >> +	     static initialization.  In turn static initialization guarantees
> >> +	     that pad is initialized to zero bits.
> >> +	     So, it's better to clear the whole record under such situation.  */
> >> +	  cleared = true;
> >> 	else
> >> 	  cleared = false;
> >> 
> >> Then the paddings are also initialized to zeroes with this option. (Even for -ftrivial-auto-var-init=pattern).
> > 
> > Thanks! I've tested with the attached patch to v4 and it passes all my
> > tests again.
> > 
> >> Is the above change Okay? (With this change, when -ftrivial-auto-var-init=pattern, the paddings for the
> >> structure variables that have explicit initializer will be ZEROed, not 0xFE)
> > 
> > Padding zeroing in the face of pattern-init is correct (and matches what
> > Clang does).
> 
> During the discussion before the 4th version of the patch, we have agreed that pattern-init will use 0xFE byte-repeatable patterns 
> to initialize all the types (this includes the paddings when the structure type variables are not explicitly initialized). And will not match
> Clang’s current behavior. 

Right, that's fine.

> If we initialize the paddings when the structure type variables are explicitly initialized to Zeroes, then there will be inconsistency 
> between values that are used to initialize structure paddings under different situations, This looks not good to me.
> 
> If we have agreed on using 0xFE byte-repeatable patterns for pattern-init, then all the paddings should be initialized with the same 
> pattern. 

Ah! By "situation", you mean how the compiler chooses to initialize the
structure members?

It sounds like for =zero mode, padding will be 0, but for =pattern,
padding may be either 0x00 or 0xFE, depending on which kind of
initialization is internally chosen. Is that right? I'm fine with this
since the =zero case is what I'm primarily focused on being as safe
as possible.

-Kees

-- 
Kees Cook