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