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?
Thanks.
Qing
> On Jul 13, 2021, at 4:29 PM, Kees Cook <keesc...@chromium.org> wrote:
>
> On Mon, Jul 12, 2021 at 08:28:55PM +0000, Qing Zhao wrote:
>>> On Jul 12, 2021, at 12:56 PM, Kees Cook <keesc...@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
> <stackinit.c>
