On Wed, Jul 14, 2021 at 1:17 AM Qing Zhao <qing.z...@oracle.com> wrote:
>
> 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?
I think this can be addressed in the gimplifier by adjusting
gimplify_init_constructor to clear
the object before the initialization (if it's not done via aggregate
copying). The clearing
could be done via .DEFERRED_INIT.
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.
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>
>
