On Mon, Jul 12, 2021 at 08:28:55PM +0000, Qing Zhao wrote:
> > 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:
> >> This is the 4th version of the patch for the new security feature for GCC.
> > 
> > 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?

Yes, I was only testing =zero (the kernel test handles =pattern as well:
it doesn't explicitly test for 0x00). I've verified with =pattern now,
too.

> 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.

I've double-checked that I'm using the right gcc, with the flag.

> > 
> > 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.

Hm, agreed -- this test does do the right thing.

> > 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.

I've extracted the kernel test to build for userspace, and it behaves
the same way. See attached "stackinit.c".

$ gcc-build/auto-var-init.4/installed/bin/gcc -O2 -Wall -o stackinit stackinit.c
$ ./stackinit 2>&1 | grep failures:
stackinit: failures: 23
$ gcc-build/auto-var-init.4/installed/bin/gcc -O2 -Wall -ftrivial-auto-var-init=zero -o stackinit stackinit.c
stackinit.c: In function ‘__leaf_switch_none’:
stackinit.c:326:26: warning: statement will never be executed
[-Wswitch-unreachable]
  326 |                 uint64_t var;
      |                          ^~~
$ ./stackinit 2>&1 | grep failures:
stackinit: failures: 6

Same failures as seen in the kernel test (and an expected warning
about the initialization that will never happen for a pre-case switch
statement).

> > 
> > (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. 

Great!

-- 
Kees Cook