From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62f.google.com (mail-pl1-x62f.google.com [IPv6:2607:f8b0:4864:20::62f]) by sourceware.org (Postfix) with ESMTPS id 90EF93893676 for ; Mon, 12 Jul 2021 17:56:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 90EF93893676 Received: by mail-pl1-x62f.google.com with SMTP id z2so6730291plg.8 for ; Mon, 12 Jul 2021 10:56:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=TbLq2vqpmPgcEncdfsOm2ZJJsj8+veCqzLf7qFAk7h4=; b=gBJ45RZr9UhlJM7U9BwdeuJ4QtFQd3nenln0v0EjCHFOXxC2POO+TH8fgLleD3kUza Po6Oc3kR59iezX01rrXuI0XSVtevU9zyhY5/6ehi4cq4ciyy3+ZK6x73zNb/Ihzw8ZD5 gdZJQF3khuTjDaWbQkNdBxcuXzehExH7qrriG1XmEpfrC9uZsuSGZm3X4iUIwXAyR0IA TbTt0zSvWb12zUVicq03IRqGih11CnhK34I5jfR7CojMJFQi3xY1jTDe/PapT47T0V+l dJ/5U+sDzsRYLYbeUQzFTccetvyVHuINqU23AA5svWe0RG/ss7DjPe6ED1NipzsAY3uY cTMw== X-Gm-Message-State: AOAM530NNwZZe2cTgRCiFMsE8x1yld+O+B7dCZPP6NapPNDEzOswdrL9 llyrJ3K+VDccUC41SH/klq8b9Q== X-Google-Smtp-Source: ABdhPJyq75jlL/QhAmBj3+WF3ZBT0vZeftMrTxOuz1izNMXDG+gT7zPkU+sWBdwEsOtggqkM09IdFQ== X-Received: by 2002:a17:902:bc47:b029:129:dd30:1c30 with SMTP id t7-20020a170902bc47b0290129dd301c30mr278534plz.4.1626112589696; Mon, 12 Jul 2021 10:56:29 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id g7sm14598692pfm.5.2021.07.12.10.56.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Jul 2021 10:56:29 -0700 (PDT) Date: Mon, 12 Jul 2021 10:56:27 -0700 From: Kees Cook To: Qing Zhao Cc: Richard Biener , Richard Sandiford , gcc-patches Qing Zhao via Subject: Re: [patch][version 4]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc Message-ID: <202107121030.295D4E590@keescook> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, JMQ_SPF_NEUTRAL, KAM_SHORT, 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: Mon, 12 Jul 2021 17:56:32 -0000 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) 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 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.) 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