> On Jul 15, 2021, at 9:16 AM, Qing Zhao via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>
>>
>>>>
>>>> Note that I think .DEFERRED_INIT can be elided for variables that do
>>>> not have their address
>>>> taken - otherwise we'll also have to worry about aggregate copy
>>>> initialization and SRA
>>>> decomposing the copy, initializing only the used parts.
>>>
>>> Please explain this a little bit more.
>>
>> For sth like
>>
>> struct S { int i; long j; };
>>
>> void bar (struct S);
>> struct S
>> foo (struct S *p)
>> {
>> struct S q = *p;
>> struct S r = q;
>> bar (r);
>> return r;
>> }
>>
>> we don't get a .DEFERRED_INIT for 'r' (do we?)
>
> No, we don’t emit .DEFERRED_INIT for both ‘q’ and ‘r’ since they are all
> explicitly initialized.
Another thought on this example:
I think for the auto variable ‘q’ and ‘r’ in function ‘foo’, their
initialization depend on the incoming parameter ‘p’.
If ‘p’ is an auto variable in ‘foo’s caller, then the incoming parameter should
be initialized well in the caller, including it’s padding.
So, I don’t think that we need to worry about such situation.
If every function guarantees all its own auto-variables to be initialized
completely including the paddings.
Then we can guarantee all such copy initialization through parameters all
initialized completely.
Let me know if I miss anything here.
Qing
> With the current 4th patch, the paddings inside this structure variable is
> not initialized.
>
> However, if we “clear” the whole structure in "gimplify_init_constructor “,
> the initialization might happen. I will check on this.
>
> and SRA decomposes the init to
>>
>>
>> <bb 2> :
>> q = *p_2(D);
>> q$i_9 = p_2(D)->i;
>> q$j_10 = p_2(D)->j;
>> r.i = q$i_9;
>> r.j = q$j_10;
>> bar (r);
>> D.1953 = r;
>> r ={v} {CLOBBER};
>> return D.1953;
>>
>> which leaves its padding uninitialized. Hmm, and that even happens when
>> you make bar take struct S * and thus pass the address of 'r' to bar.
>
> Will try this example and see how to resolve this issue.
>
> Thanks for your explanation.
>
> Qing
>>
>> Richard.
>>
>>
>>> Thanks.
>>>
>>> Qing
>>>>
>>>> Richard.
>>>>
>>>>> 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>
