From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x532.google.com (mail-ed1-x532.google.com [IPv6:2a00:1450:4864:20::532]) by sourceware.org (Postfix) with ESMTPS id C75133861834 for ; Thu, 15 Jul 2021 07:56:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C75133861834 Received: by mail-ed1-x532.google.com with SMTP id x17so6785977edd.12 for ; Thu, 15 Jul 2021 00:56:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=b8X/oTrNQ2z2lqYXGtIppNG0sE268mDCrgHjloYxKbw=; b=I0qTRg6/HGW0u+BgCQDlZli4N20RC/Z1bG5nFv0okeuEeco9oTtnxmtAcLALF5SDQK i40AgafyElsWlJCcvLVpU5bIhaBBOW+Db1G3eY+D07dAVbqAR7cQO9eefT36KXlhDqt5 /D1rrV+czwXPIpmDmQ2tUomQs9is6yPCyanYOBY4ieognsHgiSNfuUNLLQTws95u8/SF 3Z5+QrI4wEOz6i3TZ0DSxdmisNWJmCHiD9wwNaR2SsVcMxsO5Z4L5TuP0t8brj4DhU3r pWb0DLaYmRuPwqaPvAC3gaBQlPxO2V7pAGe7jPp98mb+TmVedq7ga2jasUBp589WGx+e hL9Q== X-Gm-Message-State: AOAM533QjRLMxUMLcJIHZ2aifCbB8IEUFWIKckCvvgJulwu6dbAtfVdT vYHfqPLi1bsZsJwmQ0c9aapo3bRoO/rUXS/kdAs= X-Google-Smtp-Source: ABdhPJzCOcZ7h/QHAAoRuwJhTred/q8JGZBvsjBD5fo3kob8/hwCUTEVnkk90wMb+0rDlysFcufBjTz4iBNQgpVNoWk= X-Received: by 2002:a05:6402:438c:: with SMTP id o12mr3767393edc.361.1626335785787; Thu, 15 Jul 2021 00:56:25 -0700 (PDT) MIME-Version: 1.0 References: <202107121030.295D4E590@keescook> <80CAE4C0-237B-4F1A-9569-7EC789563CB8@oracle.com> <202107131227.C6DF131@keescook> <6550FBBC-E954-478D-8BAC-F7B0392ED2F5@oracle.com> In-Reply-To: From: Richard Biener Date: Thu, 15 Jul 2021 09:56:14 +0200 Message-ID: Subject: Re: [patch][version 4]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc To: Qing Zhao Cc: Kees Cook , Richard Sandiford , gcc-patches Qing Zhao via Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Jul 2021 07:56:28 -0000 On Wed, Jul 14, 2021 at 4:10 PM Qing Zhao wrote: > > Hi, Richard, > > > On Jul 14, 2021, at 2:14 AM, Richard Biener wrote: > > > > On Wed, Jul 14, 2021 at 1:17 AM Qing Zhao 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 =3D { .one =3D arg->one, = \ > >> .two =3D arg->two, = \ > >> .three =3D arg->three, = \ > >> .four =3D arg->four, = \ > >> } > >> > >> i.e, when the structure type auto variable has been explicitly initial= ized 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-v= ar-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 completel= y 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 patc= h for this additional functionality, but not with this patch. > >> > >> Richard, what=E2=80=99s 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). > > I did this in the previous versions of the patch like the following: > > @@ -5001,6 +5185,17 @@ gimplify_init_constructor (tree *expr_p, gimple_se= q *pre_p, gimple_seq *post_p, > /* If a single access to the target must be ensured and all ele= ments > are zero, then it's optimal to clear whatever their number. = */ > cleared =3D true; > + else if (flag_trivial_auto_var_init > AUTO_INIT_UNINITIALIZED > + && !TREE_STATIC (object) > + && type_has_padding (type)) > + /* If the user requests to initialize automatic variables with > + paddings inside the type, we should initialize the paddings = too. > + C guarantees that brace-init with fewer initializers than me= mbers > + aggregate will initialize the rest of the aggregate as-if it= were > + static initialization. In turn static initialization guaran= tees > + that pad is initialized to zero bits. > + So, it's better to clear the whole record under such situati= on. */ > + cleared =3D true; > else > cleared =3D false; > > Then the paddings are also initialized to zeroes with this option. (Even = for -ftrivial-auto-var-init=3Dpattern). > > Is the above change Okay? (With this change, when -ftrivial-auto-var-init= =3Dpattern, the paddings for the > structure variables that have explicit initializer will be ZEROed, not 0x= FE) I guess that would be the simplest way, yes. > > The clearing > > could be done via .DEFERRED_INIT. > > You mean to add additional calls to .DEFERRED_INIT for each individual pa= dding of the structure in =E2=80=9Cgimplify_init_constructor"? > Then later during RTL expand, expand these calls the same as other calls= ? No, I actually meant to in your patch above set defered_padding_init =3D true; and where 'cleared' is processed do sth like if (defered_padding_init) .. emit .DEFERRED_INIT for the _whole_ variable .. else if (cleared) .. original cleared handling ... that would retain the pattern init but possibly be less efficient in the en= d. > > > > 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 =3D *p; struct S r =3D q; bar (r); return r; } we don't get a .DEFERRED_INIT for 'r' (do we?) and SRA decomposes the init = to : q =3D *p_2(D); q$i_9 =3D p_2(D)->i; q$j_10 =3D p_2(D)->j; r.i =3D q$i_9; r.j =3D q$j_10; bar (r); D.1953 =3D r; r =3D{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. Richard. > Thanks. > > Qing > > > > Richard. > > > >> Thanks. > >> > >> Qing > >> > >>> On Jul 13, 2021, at 4:29 PM, Kees Cook wrote: > >>> > >>> On Mon, Jul 12, 2021 at 08:28:55PM +0000, Qing Zhao wrote: > >>>>> On Jul 12, 2021, at 12:56 PM, Kees Cook wro= te: > >>>>> 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 see= ing > >>>>> 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=3Dzero or -ftrivi= al-auto-var-init=3Dpattern? Or both? > >>> > >>> Yes, I was only testing =3Dzero (the kernel test handles =3Dpattern a= s well: > >>> it doesn't explicitly test for 0x00). I've verified with =3Dpattern n= ow, > >>> too. > >>> > >>>> For the current implementation, I believe that all paddings should b= e initialized with this option, > >>>> for -ftrivial-auto-var-init=3Dzero, the padding will be initialized = to zero as before, however, for > >>>> -ftrivial-auto-var-init=3Dpattern, the padding will be initialized t= o 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 elemen= t with > >>>> 2 padding. */ > >>>> 3 /* { dg-do compile } */ > >>>> 4 /* { dg-options "-ftrivial-auto-var-init=3Dzero" } */ > >>>> 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=3Dzero) > >>>> > >>>> .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, =E2=80=9Czero=E2=80=9D will be used to = initialize 8 * 20 =3D 16 * 10 bytes of memory starting from the beginning o= f =E2=80=9Cvar=E2=80=9D, that include all the padding holes inside > >>>> This array of structure. > >>>> > >>>> I didn=E2=80=99t 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 th= e > >>>>> 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-aut= o-var-init=3Dzero -o stackinit stackinit.c > >>> stackinit.c: In function =E2=80=98__leaf_switch_none=E2=80=99: > >>> 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 det= ails > >>>>> 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 th= e > >>>>> padding tests.) > >>>> > >>>> Yes, I will fix the testing names to more reflect the testing detail= s. > >>> > >>> Great! > >>> > >>> -- > >>> Kees Cook > >>> >