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

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

Hi, Kees,

Thanks a lot for your testing on kernel testing cases. 

I have some question in below:

> On Jul 12, 2021, at 12:56 PM, Kees Cook <keescook@chromium.org> wrote:
> 
> On Wed, Jul 07, 2021 at 05:38:02PM +0000, Qing Zhao wrote:
>> Hi, 
>> 
>> This is the 4th version of the patch for the new security feature for GCC.
>> 
>> I have tested it with bootstrap on both x86 and aarch64, regression testing on both x86 and aarch64.
>> Also compile and run CPU2017, without any issue.
>> 
>> Please take a look and let me know your comments and suggestions.
> 
> Thanks for the update!
> 
> It looks like padding initialization has regressed to where things where
> in version 1[1] (it was, however, working in version 2[2]). I'm seeing
> these failures again in the kernel self-test:
> 
> test_stackinit: small_hole_static_all FAIL (uninit bytes: 3)
> test_stackinit: big_hole_static_all FAIL (uninit bytes: 61)
> test_stackinit: trailing_hole_static_all FAIL (uninit bytes: 7)
> test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
> test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61)
> test_stackinit: trailing_hole_dynamic_all FAIL (uninit bytes: 7)
 
Are the above failures for -ftrivial-auto-var-init=zero or -ftrivial-auto-var-init=pattern?  Or both?

For the current implementation, I believe that all paddings should be initialized with this option, 
for -ftrivial-auto-var-init=zero, the padding will be initialized to zero as before, however, for
-ftrivial-auto-var-init=pattern, the padding will be initialized to 0xFE byte-repeatable patterns.

> 
> In looking at the gcc test cases, I think the wrong thing is
> being checked: we want to verify the padding itself. For example,
> in auto-init-17.c, the actual bytes after "four" need to be checked,
> rather than "four" itself.

******For the current auto-init-17.c

  1 /* Verify zero initialization for array type with structure element with
  2    padding.  */
  3 /* { dg-do compile } */
  4 /* { dg-options "-ftrivial-auto-var-init=zero" } */
  5 
  6 struct test_trailing_hole {
  7         int one;
  8         int two;
  9         int three;
 10         char four;
 11         /* "sizeof(unsigned long) - 1" byte padding hole here. */
 12 };
 13 
 14 
 15 int foo ()
 16 {
 17   struct test_trailing_hole var[10];
 18   return var[2].four;
 19 }
 20 
 21 /* { dg-final { scan-assembler "movl\t\\\$0," } } */
 22 /* { dg-final { scan-assembler "movl\t\\\$20," } } */
 23 /* { dg-final { scan-assembler "rep stosq" } } */
~  
******We have the assembly as: (-ftrivial-auto-var-init=zero)

        .file   "auto-init-17.c"
        .text
        .globl  foo
        .type   foo, @function
foo:
.LFB0:
        .cfi_startproc
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset 6, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register 6
        subq    $40, %rsp
        leaq    -160(%rbp), %rax
        movq    %rax, %rsi
        movl    $0, %eax
        movl    $20, %edx
        movq    %rsi, %rdi
        movq    %rdx, %rcx
        rep stosq
        movzbl  -116(%rbp), %eax
        movsbl  %al, %eax
        leave
        .cfi_def_cfa 7, 8
        ret
        .cfi_endproc
.LFE0:
        .size   foo, .-foo
        .section        .note.GNU-stack,"",@progbits

From the above, we can see,  “zero” will be used to initialize 8 * 20 = 16 * 10 bytes of memory starting from the beginning of “var”, that include all the padding holes inside
This array of structure. 

I didn’t see issue with padding initialization here.

Do I miss anything here?

For pattern initialization, since we currently initialize all paddings with 0xFE byte-repeatable patterns, if the testing case still assumes zero initialization, then the testing cases
Need to be updated with this fact.

> For example, something like this:
> 
> struct test_trailing_hole {
>        int one;
>        int two;
>        int three;
>        char four;
>        /* "sizeof(unsigned long) - 1" byte padding hole here. */
> };
> 
> #define offsetofend(STRUCT, MEMBER) \
>  (__builtin_offsetof(STRUCT, MEMBER) + sizeof((((STRUCT *)0)->MEMBER)))
> 
> int foo ()
> { 
>  struct test_trailing_hole var[10];
>  unsigned char *ptr = (unsigned char *)&var[2];
>  int i;
> 
>  for (i = 0; i < sizeof(var[2]) - offsetofend(typeof(var[2]), four); i++) {
>    if (ptr[i] != 0)
>      return 1;
>  } 
>  return 0;
> }
> 
> But this isn't actually sufficient because they may _accidentally_
> be zero already. The kernel tests specifically make sure to fill the
> about-to-be-used stack with 0xff before calling a function like foo()
> above.
> 
> (And as an aside, it seems like naming the test cases with some details
> about what is being tested in the filename would be nice -- it was
> a little weird having to dig through their numeric names to find the
> padding tests.)

Yes, I will fix the testing names to more reflect the testing details. 

thanks.

Qing
> 
> Otherwise, this looks like it's coming along; I remain very excited!
> Thank you for sticking with it. :)
> 
> -Kees
> 
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565840.html
> [2] https://gcc.gnu.org/pipermail/gcc-patches/2021-April/567754.html
> 
> -- 
> Kees Cook