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

[Qing Zhao <qing.zhao@oracle.com>]

> On Jul 14, 2021, at 4:23 PM, Kees Cook <keescook@chromium.org> wrote:
> 
> 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?

There are three situations that we should initialize the paddings of a structure type auto-variable:

1. When there is no any explicit initializer;
2. When there is an explicit initializer, and the initializer only partially initialize the structure variable;
3. When there is an explicit initializer, and the initializer fully initialize the structure variable;

The code examples for the above 3 situations are:

struct test_small_hole {
        size_t one;
        char two;
        /* 3 byte padding hole here. */
        int three;
        unsigned long four;
};

1.  struct test_small_hole tmp1;
2.  struct test_small_hole tmp2 = {.two = 0, }
3.  Struct test_small_hole tmp3 = {.one =1, .two = 2, .three = 3, .four = 4,}

The current GCC without this new feature initializes the paddings of 2 to zeroes already inside “gimplify_init_constructor”, 
“If the constructor isn’t complete, clear the whole object beforehand”. 

But for 1 and 3, the current GCC does  not initialize the paddings. 

With the new feature (-ftrivial-auto-var-init ), We want all the paddings to be initialized, including all the above 3 situations. 

*******In the 2nd and 3rd version of the patches:
For situation 1, the padding initialization is handled in “store_constructor”  as following:

"
-- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -6539,14 +6539,19 @@ store_constructor (tree exp, rtx target, int cleared, poly_int64 size,
 	    cleared = 1;
 	  }
 
-        /* If the constructor has fewer fields than the structure or
-	   if we are initializing the structure to mostly zeros, clear
-	   the whole structure first.  Don't do this if TARGET is a
-	   register whose mode size isn't equal to SIZE since
-	   clear_storage can't handle this case.  */
+	/* If the constructor has fewer fields than the structure,
+	   or if we are initializing the structure to mostly zeros,
+	   or if the user requests to initialize automatic variables with
+	   paddings inside the type,
+	   we should clear the whole structure first.
+	   Don't do this if TARGET is a register whose mode size isn't equal
+	   SIZE since clear_storage can't handle this case.  */
 	else if (known_size_p (size)
 		 && (((int) CONSTRUCTOR_NELTS (exp) != fields_length (type))
-		     || mostly_zeros_p (exp))
+		     || mostly_zeros_p (exp)
+		     || (flag_trivial_auto_var_init > AUTO_INIT_UNINITIALIZED
+			 && !TREE_STATIC (exp)
+			 && type_has_padding (type)))
 		 && (!REG_P (target)
 		     || known_eq (GET_MODE_SIZE (GET_MODE (target)), size)))
 	  {
“

For situation 2, and 3,  the padding initialization is handled in “gimplify_init_constructor”, similarly as above. 
All the paddings are initialized to “zeroes.

******in the 4th version of the patch:

For situation 1, the padding initialization is included with the whole structure variable initialization by memset of zeroes or 0xFE byte repeatable patterns. 
For situation 2, the padding initialization is still applied as before with “gimplify_init_constructor”, but its value always is zeroes even for pattern-init;
For situation 3, no padding initialization is applied.

So, in order to complete the padding initialization implementation, we need to answer the following question first:

Should all the padding initialization for situation 1, 2, 3 use the same value for pattern-init? 

If YES, then we should use “0xFE” to initialize all paddings for pattern-init, the implementation for padding initialization of situation 3 will be more complicate.

> 
> It sounds like for =zero mode, padding will be 0,

Yes.

> but for =pattern,
> padding may be either 0x00 or 0xFE, depending on which kind of
> initialization is internally chosen. Is that right?

Right with the current patch, but we need to decide whether we want consistent value in paddings for pattern init.

Qing


> I'm fine with this
> since the =zero case is what I'm primarily focused on being as safe
> as possible.
> 
> -Kees
> 
> -- 
> Kees Cook