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 <keesc...@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.guent...@gmail.com>
> >>> wrote:
> >>>
> >>> On Wed, Jul 14, 2021 at 1:17 AM Qing Zhao <qing.z...@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
