Hi, This is to port the patch from google/main to trunk, which provides a new stack protection option - "fstack-protector-strong".
Previous review for google trunk is here - http://codereview.appspot.com/5461043 Status - it has been used in google/main for 2 quarters, building the whole chromiumos with no securiy degradation. Benefit - gain big performance while sacrificing little security (for scenarios using -fstack-protector-all) Background - some times stack-protector is too-simple while stack-protector-all over-kills, for example, to build one of our core systems, we forcibly add "-fstack-protector-all" to all compile commands, which brings big performance penalty (due to extra stack guard/check insns on function prologue and epilogue) on both atom and arm. To use "-fstack-protector" is just regarded as not secure enough (only "protects" <2% functions) by the system secure team. So I'd like to add the option "-fstack-protector-strong", that hits the balance between "-fstack-protector" and "-fstack-protector-all". Tested - building chromiumos from scratch. Changelog - gcc/ChangeLog: * cfgexpand.c (expand_used_vars): Add logic handling stack-protector-strong. (record_or_union_type_has_array_p): New, tests if a record or union type contains an array. * common.opt (fstack-protector-all): New option. * doc/invoke.texi: Added reference to "-fstack-protector-strong". gcc/testsuite/ChangeLog * gcc.dg/fstack-protector-strong.c: New. * g++.dg/fstack-protector-strong.C: New. Patch - diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 8a31a9f..0911b6c 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1511,15 +1511,39 @@ estimated_stack_frame_size (struct cgraph_node *node) return size; } +/* Helper routine to check if a record or union contains an array field. */ + +static int +record_or_union_type_has_array_p (const_tree tree_type) +{ + tree fields = TYPE_FIELDS (tree_type); + tree f; + + for (f = fields; f; f = DECL_CHAIN (f)) + { + if (TREE_CODE (f) == FIELD_DECL) + { + tree field_type = TREE_TYPE (f); + if (RECORD_OR_UNION_TYPE_P (field_type)) + return record_or_union_type_has_array_p (field_type); + if (TREE_CODE (field_type) == ARRAY_TYPE) + return 1; + } + } + return 0; +} + /* Expand all variables used in the function. */ static void expand_used_vars (void) { tree var, outer_block = DECL_INITIAL (current_function_decl); + referenced_var_iterator rvi; VEC(tree,heap) *maybe_local_decls = NULL; unsigned i; unsigned len; + int gen_stack_protect_signal = 0; /* Compute the phase of the stack frame for this function. */ { @@ -1552,6 +1576,23 @@ expand_used_vars (void) } } + FOR_EACH_REFERENCED_VAR (cfun, var, rvi) + if (!is_global_var (var)) + { + tree var_type = TREE_TYPE (var); + /* Examine local referenced variables that have their addresses taken, + contain an array, or are arrays. */ + if (TREE_CODE (var) == VAR_DECL + && (TREE_CODE (var_type) == ARRAY_TYPE + || TREE_ADDRESSABLE (var) + || (RECORD_OR_UNION_TYPE_P (var_type) + && record_or_union_type_has_array_p (var_type)))) + { + ++gen_stack_protect_signal; + break; + } + } + /* At this point all variables on the local_decls with TREE_USED set are not associated with any block scope. Lay them out. */ @@ -1642,11 +1683,18 @@ expand_used_vars (void) dump_stack_var_partition (); } - /* There are several conditions under which we should create a - stack guard: protect-all, alloca used, protected decls present. */ - if (flag_stack_protect == 2 - || (flag_stack_protect - && (cfun->calls_alloca || has_protected_decls))) + /* Create stack guard, if + a) "-fstack-protector-all" - always; + b) "-fstack-protector-strong" - if there are arrays, memory + references to local variables, alloca used, or protected decls present; + c) "-fstack-protector" - if alloca used, or protected decls present */ + if (flag_stack_protect == 3 /* -fstack-protector-all */ + || (flag_stack_protect == 2 /* -fstack-protector-strong */ + && (gen_stack_protect_signal || cfun->calls_alloca + || has_protected_decls)) + || (flag_stack_protect == 1 /* -fstack-protector */ + && (cfun->calls_alloca + || has_protected_decls))) create_stack_guard (); /* Assign rtl to each variable based on these partitions. */ diff --git a/gcc/common.opt b/gcc/common.opt index 5b1b4d8..44447f6 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1846,8 +1846,12 @@ fstack-protector Common Report Var(flag_stack_protect, 1) Use propolice as a stack protection method -fstack-protector-all +fstack-protector-strong Common Report RejectNegative Var(flag_stack_protect, 2) +Use a smart stack protection method for certain functions + +fstack-protector-all +Common Report RejectNegative Var(flag_stack_protect, 3) Use a stack protection method for every function fstack-usage diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 9fa0085..44f2034 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -403,7 +403,7 @@ Objective-C and Objective-C++ Dialects}. -fsel-sched-pipelining -fsel-sched-pipelining-outer-loops @gol -fshrink-wrap -fsignaling-nans -fsingle-precision-constant @gol -fsplit-ivs-in-unroller -fsplit-wide-types -fstack-protector @gol --fstack-protector-all -fstrict-aliasing -fstrict-overflow @gol +-fstack-protector-all -fstack-protector-strong -fstrict-aliasing -fstrict-overflow @gol -fthread-jumps -ftracer -ftree-bit-ccp @gol -ftree-builtin-call-dce -ftree-ccp -ftree-ch @gol -ftree-coalesce-inline-vars -ftree-coalesce-vars -ftree-copy-prop @gol @@ -8564,6 +8564,12 @@ If a guard check fails, an error message is printed and the program exits. @opindex fstack-protector-all Like @option{-fstack-protector} except that all functions are protected. +@item -fstack-protector-strong +@opindex fstack-protector-strong +Like @option{-fstack-protector} but includes additional functions to be +protected - those that have local array definitions, or have references to +local frame addresses. + @item -fsection-anchors @opindex fsection-anchors Try to reduce the number of symbolic address calculations by using diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C b/gcc/testsuite/g++.dg/fstack-protector-strong.C new file mode 100644 index 0000000..a4f0f81 --- /dev/null +++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C @@ -0,0 +1,35 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +class A +{ +public: + A() {} + ~A() {} + void method(); + int state; +}; + +/* Frame address exposed to A::method via "this". */ +int +foo1 () +{ + A a; + a.method (); + return a.state; +} + +/* Possible destroying foo2's stack via &a. */ +int +global_func (A& a); + +/* Frame address exposed to global_func. */ +int foo2 () +{ + A a; + return global_func (a); +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */ diff --git a/gcc/testsuite/gcc.dg/fstack-protector-strong.c b/gcc/testsuite/gcc.dg/fstack-protector-strong.c new file mode 100644 index 0000000..5a5cf98 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c @@ -0,0 +1,135 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +#include<string.h> +#include<stdlib.h> + +extern int g0; +extern int* pg0; +int +goo (int *); +int +hoo (int); + +/* Function frame address escaped function call. */ +int +foo1 () +{ + int i; + return goo (&i); +} + +struct ArrayStruct +{ + int a; + int array[10]; +}; + +struct AA +{ + int b; + struct ArrayStruct as; +}; + +/* Function frame contains array. */ +int +foo2 () +{ + struct AA aa; + int i; + for (i = 0; i < 10; ++i) + { + aa.as.array[i] = i * (i-1) + i / 2; + } + return aa.as.array[5]; +} + +/* Address computation based on a function frame address. */ +int +foo3 () +{ + int a; + int *p; + p = &a + 5; + return goo (p); +} + +/* Address cast based on a function frame address. */ +int +foo4 () +{ + int a; + return goo (g0 << 2 ? (int *)(3 * (long)(void *)(&a)) : 0); +} + +/* Address cast based on a local array. */ +int +foo5 () +{ + short array[10]; + return goo ((int *)(array + 5)); +} + +struct BB +{ + int one; + int two; + int three; +}; + +/* Address computaton based on a function frame address.*/ +int +foo6 () +{ + struct BB bb; + return goo (&bb.one + sizeof(int)); +} + +/* Function frame address escaped via global variable. */ +int +foo7 () +{ + int a; + pg0 = &a; + goo (pg0); + return *pg0; +} + +/* Check that this covers -fstack-protector. */ +int +foo8 () +{ + char base[100]; + memcpy ((void *)base, (const void *)pg0, 105); + return (int)(base[32]); +} + +/* Check that this covers -fstack-protector. */ +int +foo9 () +{ + char* p = alloca (100); + return goo ((int *)(p + 50)); +} + +int +global2 (struct BB* pbb); + +/* Address taken on struct. */ +int +foo10 () +{ + struct BB bb; + int i; + bb.one = global2 (&bb); + for (i = 0; i < 10; ++i) + { + bb.two = bb.one + bb.two; + bb.three = bb.one + bb.two + bb.three; + } + return bb.three; +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */ Design doc - Proposal to add compiler option “-fstack-protector-strong” 1. Current stack protection options Currently, gcc only has 2 options regarding stack protector against SSA (stack smashing attack) - stack-protector Add stack protection to functions that have “alloca” or have a (signed or unsigned) char array with size > 8 (SSP_BUFFER_SIZE) - stack-protector-all Add stack protection to ALL functions. 2. Problems with current stack protection options The stack-protector option is over-simplified, which ignores pointer cast, address computation, while the stack-protector-all is over-killing, using this option brings too much performance overhead to both arm- and atom- chrome browser. 3. Propose a new stack protector option - stack-protector-strong This option tries to hit the balance between an over-simplified version and an over-killing protection schema. It chooses more functions to be protected than “stack-protector”, it is a superset of “stack-protector”, for functions not chosen by “stack-protector”, “stack-protector-strong” will apply protection if any of the following conditions meets. - if any of its local variable’s address is taken,as part of the RHS of an assignment - or if any of its local variable’s address is taken as part of a function argument. - or if it has an array, regardless of array type or length - or if it has a struct/union which contains an array, regardless of array type or length. - or if function has register local variables 4. Possible performance gain for atom and tegra board Replacing “stack-protector-all” with “stack-protector-strong” sees a good performance gain. 5. Ideas behind this implementation: The basic idea is that any stack smashing attack needs a frame address to perform the attack. The frame address of function F can be from one of the following: - (A) an address taking operator (&) on any local variables of F - (B) any address computation based on (A) - (C) any address casting operation from either (A) or (B) - (D) the name of a local array of F - (E) the address from calling “alloca” Function F is said to be vulnerable if its frame address is exposed via (A) ~ (E). “stack-protector-strong” just protects these vulnerable functions. 6. Stack smashing attack illustrated If in function F, we have pointer P, which points to function G's stack, you can only attack frames of function G and functions calling G, for example X and XX. To add guard0 to frame G protects everything above guard0 being attacked from P". (Note, v0 and v1 are still attack-able, this won't be fixed even if you add guard to all frames.) Suppose now you have Q, which points to XX's frame, guard0 will not prevent attacking from Q, so we have to add guard1. To summarize, we just need to add guard to functions whose frame address is exposed(escaped) - either by address taken operator (&), or by passing address (or array) around via function call. (See picture below) (I cannot upload pictures here, to see the origin picture - refer here - https://docs.google.com/a/google.com/document/d/1xXBH6rRZue4f296vGt9YQcuLVQHeE516stHwt8M9xyU/edit?hl=en_US) Patch also uploaded to http://codereview.appspot.com/6303078/ Regards, -Han
