On Wed, Dec 2, 2020 at 4:36 PM Qing Zhao <qing.z...@oracle.com> wrote: > > > > On Dec 2, 2020, at 2:45 AM, Richard Biener <richard.guent...@gmail.com> wrote: > > On Tue, Dec 1, 2020 at 8:49 PM Qing Zhao <qing.z...@oracle.com> wrote: > > > Hi, Richard, > > Could you please comment on the following approach: > > Instead of adding the zero-initializer quite late at the pass “pass_expand”, > we can add it as early as during gimplification. > However, we will mark these new added zero-initializers as “artificial”. And > passing this “artificial” information to > “pass_early_warn_uninitialized” and “pass_late_warn_uninitialized”, in these > two uninitialized variable analysis passes, > (i.e., in tree-sea-uninit.c) We will update the checking on > “ssa_undefined_value_p” to consider “artificial” zero-initializers. > (i.e, if the def_stmt is marked with “artificial”, then it’s 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 impacted > 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 submission): > > "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 to > 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: > > • The compiler re-uses stack slots, and a value is used uninitialized. > • The compiler re-uses a register, and a value is used uninitialized. > • Stack structs / arrays / unions with padding are copied. > This patch only addresses stack and register information leaks. There's many > more infoleaks that we could address, and much more undefined behavior that > could be tamed. Let's keep this patch focused, and I'm happy to address > related > 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 discussion > with > Kees Cook (cc’ing him) as following: > > > thanks. > > Qing > > Support stack variables auto-initialization in GCC > > 11/19/2020 > > Qing Zhao > > ======================================================= > > > ** Background of the task: > > The correponding GCC bugzilla RFE was created on 9/3/2018: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87210 > > 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=pattern initializes local variables with a 0xAA > pattern > (actually it's more complicated, see https://reviews.llvm.org/D54604) > > -ftrivial-auto-var-init=zero provides zero-initialization of locals. > This mode isn't officially supported yet and is hidden behind an additional > -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 performs > the build with -ftrivial-auto-var-init=pattern. 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 though, > 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=zero 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/scripts/gcc-plugins/structleak_plugin.c > > ** Current situation: > > A. Both Microsoft compiler and CLANG (APPLE AND GOOGLE) support pattern init > 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-memory-on-windows/ > Pattern init is used in development build for debugging purpose, zero init 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-clang", > 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-clang" > 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=[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 > uninitialized > variables untouched in order to avoid "forking the language”. > > > > On Nov 25, 2020, at 3:11 AM, Richard Biener <richard.guent...@gmail.com> > wrote: > > > > I am planing to add a new phase immediately after > “pass_late_warn_uninitialized” to initialize all auto-variables that are > not explicitly initialized in the declaration, the basic idea is following: > > ** The proposal: > > A. add a new GCC option: (same name and meaning as CLANG) > -ftrivial-auto-var-init=[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 > uninitialized > 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 > initializer > 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 or > not after “pass_late_warn_uninitialized”, > If Not, then insert an initialization for it. > > For this purpose, I guess that “FOR_EACH_LOCAL_DECL” might 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 initializer, > I plan to add a new bit in “decl_common” 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-uninitialized. > > > > You mean I can set the flag “DECL_IS_INITIALIZED (decl)” inside the routine > “gimpley_decl_expr” (gimplify.c) as following: > > if (VAR_P (decl) && !DECL_EXTERNAL (decl)) > { > tree init = DECL_INITIAL (decl); > ... > if (init && init != error_mark_node) > { > if (!TREE_STATIC (decl)) > { > DECL_IS_INITIALIZED(decl) = 1; > } > > Is this enough for all Frontends? Are there other places that I need to > maintain 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. (From > 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 > uninitialized > variables untouched in order to avoid "forking the language”. > > cannot be satisfied. Since gcc’s uninitialized variables analysis is applied > quite late. > > So, we have to add this new phase after “pass_late_warn_uninitialized”. > > 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 check > 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" state > of locals so depending on what the actual goal is here "late" may be too late. > > > This is a really good point… > > In order to avoid optimization to use the “uninitialized” 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 > uninitialized > variables untouched in order to avoid "forking the language”. > > We have to move the new phase after all the uninitialized analysis is done in > order to avoid “forking the language”. > > 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. > >
