From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x541.google.com (mail-ed1-x541.google.com [IPv6:2a00:1450:4864:20::541]) by sourceware.org (Postfix) with ESMTPS id 9338E385800A for ; Thu, 3 Dec 2020 08:45:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9338E385800A Received: by mail-ed1-x541.google.com with SMTP id b73so1179893edf.13 for ; Thu, 03 Dec 2020 00:45:17 -0800 (PST) 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=3vWBSfsTKBmTcGFjF3eWQTNu1+KWJNFJY7iVv2yEZpE=; b=hrlPZhiBrT+m2F4EJpn0DKCUDuWp7UJV4KwnWJoKhv0zKoXtmBBUX77ANxkMSh+65d GMlW9QG9las8OVZlR4XxGJsXR/eQCLlr0WiheK2CsuJLvg6X0zv3kTBpKajwqHYKMGWc w8v90SiIVSTjm7qk8w2CF/QS9xuxnpF6KuhSqr5kE7GiwYYRjdJetYHTznybThh23+o5 eS3hsJVoe8uzoKOMmSagPzpjX/Ah3vsuEftVtYThAVxy+0ypGX0FFq8q8fjTVpr+zzRW +cb0OnUDqOWIfR2/+BoMPBrE/YLM+5p9sSTsyvCpC86fr6BgwZYEbkwTddj2ySslAs/u qn1Q== X-Gm-Message-State: AOAM532z4z5la3tgbD5wWBy6mBxaFo8yGAhdpP3IGwFtZr+qQ9ut9dTd /UK3gZQ0c3zNJLl9hdWZGbhMhvjX37H9r/WBR0E= X-Google-Smtp-Source: ABdhPJy+Y78jQ1siQaREtHjFi2AktIWAqjSOlq2jcaYt2cZSv8WjAnJVJmeTa3GzPf8sxQBuPlpl1pr1g7Ju6ZfHtNg= X-Received: by 2002:aa7:ce94:: with SMTP id y20mr1797209edv.361.1606985116411; Thu, 03 Dec 2020 00:45:16 -0800 (PST) MIME-Version: 1.0 References: <217BE64F-A623-4453-B45B-D38B66B71B72@ORACLE.COM> <15EA64C7-D75F-4CE1-92C8-6940186A512A@ORACLE.COM> <7618DD31-87EC-4003-B1DD-E318E5369A71@ORACLE.COM> <3D01F6C7-2E67-4081-BD5F-4EDEC0227627@ORACLE.COM> In-Reply-To: <3D01F6C7-2E67-4081-BD5F-4EDEC0227627@ORACLE.COM> From: Richard Biener Date: Thu, 3 Dec 2020 09:45:05 +0100 Message-ID: Subject: Re: How to traverse all the local variables that declared in the current routine? To: Qing Zhao Cc: Richard Sandiford , gcc Patches , kees Cook Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 03 Dec 2020 08:45:20 -0000 On Wed, Dec 2, 2020 at 4:36 PM Qing Zhao wrote: > > > > On Dec 2, 2020, at 2:45 AM, Richard Biener w= rote: > > On Tue, Dec 1, 2020 at 8:49 PM Qing Zhao wrote: > > > Hi, Richard, > > Could you please comment on the following approach: > > Instead of adding the zero-initializer quite late at the pass =E2=80=9Cpa= ss_expand=E2=80=9D, we can add it as early as during gimplification. > However, we will mark these new added zero-initializers as =E2=80=9Cartif= icial=E2=80=9D. And passing this =E2=80=9Cartificial=E2=80=9D information t= o > =E2=80=9Cpass_early_warn_uninitialized=E2=80=9D and =E2=80=9Cpass_late_wa= rn_uninitialized=E2=80=9D, in these two uninitialized variable analysis pas= ses, > (i.e., in tree-sea-uninit.c) We will update the checking on =E2=80=9Cssa_= undefined_value_p=E2=80=9D to consider =E2=80=9Cartificial=E2=80=9D zero-i= nitializers. > (i.e, if the def_stmt is marked with =E2=80=9Cartificial=E2=80=9D, then i= t=E2=80=99s a undefined value). > > With such approach, we should be able to address all those conflicts. > > Do you see any obvious issue with this approach? > > > Yes, DSE will happily elide an explicit zero-init following the > artificial one leading to false uninit diagnostics. > > > Indeed. This is a big issue. And other optimizations might also be impac= ted by the new zero-init, resulting changed behavior > of uninitialized analysis in the later stage. I don't see how the issue can be resolved, you can't get both, uninit warnings and no uninitialized memory. People can compile twice, once without -fzero-init to get uninit warnings and once with -fzero-init to get the extra "security". Richard. > > What's the intended purpose of the zero-init? > > > > The purpose of this new option is: (from the original LLVM patch submissi= on): > > "Add an option to initialize automatic variables with either a pattern or= with > zeroes. The default is still that automatic variables are uninitialized. = Also > add attributes to request uninitialized on a per-variable basis, mainly t= o disable > initialization of large stack arrays when deemed too expensive. > > This isn't meant to change the semantics of C and C++. Rather, it's meant= to be > a last-resort when programmers inadvertently have some undefined behavior= in > their code. This patch aims to make undefined behavior hurt less, which > security-minded people will be very happy about. Notably, this means that > there's no inadvertent information leak when: > > =E2=80=A2 The compiler re-uses stack slots, and a value is used uninitial= ized. > =E2=80=A2 The compiler re-uses a register, and a value is used uninitiali= zed. > =E2=80=A2 Stack structs / arrays / unions with padding are copied. > This patch only addresses stack and register information leaks. There's m= any > more infoleaks that we could address, and much more undefined behavior th= at > could be tamed. Let's keep this patch focused, and I'm happy to address r= elated > issues elsewhere." > > For more details, please refer to the LLVM code review discussion on this= patch: > https://reviews.llvm.org/D54604 > > > I also wrote a simple writeup for this task based on my study and discuss= ion with > Kees Cook (cc=E2=80=99ing him) as following: > > > thanks. > > Qing > > Support stack variables auto-initialization in GCC > > 11/19/2020 > > Qing Zhao > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D > > > ** Background of the task: > > The correponding GCC bugzilla RFE was created on 9/3/2018: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D87210 > > A similar option for LLVM (around Nov, 2018) > https://lists.llvm.org/pipermail/cfe-dev/2018-November/060172.html > had invoked a lot of discussion before committed. > > (The following are quoted from the comments of Alexander Potapenko in > GCC bug 87210): > > Finally, on Oct, 2019, upstream Clang supports force initialization > of stack variables under the -ftrivial-auto-var-init flag. > > -ftrivial-auto-var-init=3Dpattern initializes local variables with a 0xAA= pattern > (actually it's more complicated, see https://reviews.llvm.org/D54604) > > -ftrivial-auto-var-init=3Dzero provides zero-initialization of locals. > This mode isn't officially supported yet and is hidden behind an addition= al > -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang = flag. > This is done to avoid creating a C++ dialect where all variables are > zero-initialized. > > Starting v5.2, Linux kernel has a CONFIG_INIT_STACK_ALL config that perfo= rms > the build with -ftrivial-auto-var-init=3Dpattern. This one isn't widely = adopted > yet, partially because initializing locals with 0xAA isn't fast enough. > > Linus Torvalds is quite positive about zero-initializing the locals thoug= h, > see https://lkml.org/lkml/2019/7/30/1303: > > "when a compiler has an option to initialize stack variables, it > would probably _also_ be a very good idea for that compiler to then > support a variable attribute that says "don't initialize _this_ > variable, I will do that manually". > I also think that the "initialize with poison" is > pointless and wrong. Yes, it can find bugs, but it doesn't really help > improve the general situation, and people see it as a debugging tool, > not a "improve code quality and improve the life of kernel developers" > tool. > > So having a flag similar to -ftrivial-auto-var-init=3Dzero in GCC will be > appreciated by the Linux kernel community. > > currently, kernel is using a gcc plugin to support stack variables > auto-initialization: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/s= cripts/gcc-plugins/structleak_plugin.c > > ** Current situation: > > A. Both Microsoft compiler and CLANG (APPLE AND GOOGLE) support pattern i= nit and > zero init already; > http://lists.llvm.org/pipermail/cfe-dev/2020-April/065221.html > https://msrc-blog.microsoft.com/2020/05/13/solving-uninitialized-stack-me= mory-on-windows/ > Pattern init is used in development build for debugging purpose, zero ini= t is > used in production build for security purpose. > > B. for CLANG, even though zero init is controlled by > "-fenable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clan= g", > many end users have used it for production build. > this functionality cannot be removed anymore. > "-fenable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clan= g" > might be changed to more meaningful name later in CLANG. > > > ** My proposal: > > A. add a new GCC option: (same name and meaning as CLANG) > -ftrivial-auto-var-init=3D[pattern|zero], similar pattern init as CLANG; > > B. add a new attribute for variable: > __attribute((uninitialized) > the marked variable is uninitialized intentionaly for performance purpose= . > > C. The implementation needs to keep the current static warning on uniniti= alized > variables untouched in order to avoid "forking the language=E2=80=9D. > > > > On Nov 25, 2020, at 3:11 AM, Richard Biener = wrote: > > > > I am planing to add a new phase immediately after =E2=80=9Cpass_late_warn= _uninitialized=E2=80=9D to initialize all auto-variables that are > not explicitly initialized in the declaration, the basic idea is followin= g: > > ** The proposal: > > A. add a new GCC option: (same name and meaning as CLANG) > -ftrivial-auto-var-init=3D[pattern|zero], similar pattern init as CLANG; > > B. add a new attribute for variable: > __attribute((uninitialized) > the marked variable is uninitialized intentionaly for performance purpose= . > > C. The implementation needs to keep the current static warning on uniniti= alized > variables untouched in order to avoid "forking the language". > > > ** The implementation: > > There are two major requirements for the implementation: > > 1. all auto-variables that do not have an explicit initializer should be = initialized to > zero by this option. (Same behavior as CLANG) > > 2. keep the current static warning on uninitialized variables untouched. > > In order to satisfy 1, we should check whether an auto-variable has initi= alizer > or not; > In order to satisfy 2, we should add this new transformation after > "pass_late_warn_uninitialized". > > So, we should be able to check whether an auto-variable has initializer o= r not after =E2=80=9Cpass_late_warn_uninitialized=E2=80=9D, > If Not, then insert an initialization for it. > > For this purpose, I guess that =E2=80=9CFOR_EACH_LOCAL_DECL=E2=80=9D migh= t be better? > > > I think both as long as they are source-level auto-variables. Then which = one is better? > > > Another issue is, in order to check whether an auto-variable has initiali= zer, I plan to add a new bit in =E2=80=9Cdecl_common=E2=80=9D as: > /* In a VAR_DECL, this is DECL_IS_INITIALIZED. */ > unsigned decl_is_initialized :1; > > /* IN VAR_DECL, set when the decl is initialized at the declaration. */ > #define DECL_IS_INITIALIZED(NODE) \ > (DECL_COMMON_CHECK (NODE)->decl_common.decl_is_initialized) > > set this bit when setting DECL_INITIAL for the variables in FE. then keep= it > even though DECL_INITIAL might be NULLed. > > > For locals it would be more reliable to set this flag-Wmaybe-uninitialize= d. > > > > You mean I can set the flag =E2=80=9CDECL_IS_INITIALIZED (decl)=E2=80=9D = inside the routine =E2=80=9Cgimpley_decl_expr=E2=80=9D (gimplify.c) as fol= lowing: > > if (VAR_P (decl) && !DECL_EXTERNAL (decl)) > { > tree init =3D DECL_INITIAL (decl); > ... > if (init && init !=3D error_mark_node) > { > if (!TREE_STATIC (decl)) > { > DECL_IS_INITIALIZED(decl) =3D 1; > } > > Is this enough for all Frontends? Are there other places that I need to m= aintain this bit? > > > > Do you have any comment and suggestions? > > > As said above - do you want to cover registers as well as locals? > > > All the locals from the source-code point of view should be covered. (F= rom my study so far, looks like that Clang adds that phase in FE). > If GCC adds this phase in FE, then the following design requirement > > C. The implementation needs to keep the current static warning on uniniti= alized > variables untouched in order to avoid "forking the language=E2=80=9D. > > cannot be satisfied. Since gcc=E2=80=99s uninitialized variables analysi= s is applied quite late. > > So, we have to add this new phase after =E2=80=9Cpass_late_warn_uninitial= ized=E2=80=9D. > > I'd do > the actual zeroing during RTL expansion instead since otherwise you > have to figure youself whether a local is actually used (see expand_stack= _vars) > > > Adding this new transformation during RTL expansion is okay. I will che= ck on this in more details to see how to add it to RTL expansion phase. > > > Note that optimization will already made have use of "uninitialized" stat= e > of locals so depending on what the actual goal is here "late" may be too = late. > > > This is a really good point=E2=80=A6 > > In order to avoid optimization to use the =E2=80=9Cuninitialized=E2=80= =9D state of locals, we should add the zeroing phase as early as possible (= adding it in FE might be best > for this issue). However, if we have to met the following requirement: > > > So is optimization supposed to pick up zero or is it supposed to act > as if the initializer > is unknown? > > C. The implementation needs to keep the current static warning on uniniti= alized > variables untouched in order to avoid "forking the language=E2=80=9D. > > We have to move the new phase after all the uninitialized analysis is don= e in order to avoid =E2=80=9Cforking the language=E2=80=9D. > > So, this is a problem that is not easy to resolve. > > > Indeed, those are conflicting goals. > > Do you have suggestion on this? > > > No, not any easy ones. Doing more of the uninit analysis early (there > is already an early > uninit pass) which would mean doing IPA analysis turing GCC into more > of a static analysis > tool. Theres the analyzer now, not sure if that can employ an early > LTO phase for example. > > > > > Richard. > >