Hi, this patch provides a new stack protection option - "fstack-protector-strong".
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". Benefit - gain big performance while sacrificing little security (for scenarios using -fstack-protector-all) Status - implemented internally, to be up-streamed or merged to google branch only. Detail - https://docs.google.com/a/google.com/document/d/1xXBH6rRZue4f296vGt9YQcuLVQHeE516stHwt8M9xyU/edit?hl=en_US Tested - manually, built chrome browser using the modified compiler with "-fstack-protector-strong". ========================= gcc/ChangeLog: * cfgexpand.c (expand_used_vars): Add logic handling stack-protector-strong. (is_local_address_taken): Internal function that returns true when gimple contains an address taken on function local variables. (record_or_union_type_has_array): New, tests if a record or union type contains an array. * common.opt (fstack-protector-all): New option. gcc/testsuite/ChangeLog * gcc.dg/fstack-protector-strong.c: New. ========================== diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 8684721..1d9df87 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1507,6 +1507,50 @@ estimated_stack_frame_size (struct cgraph_node *node) return size; } +static int is_local_address_taken(tree t) { + if (t && TREE_CODE(t) == ADDR_EXPR) { + int i; + tree local_var; + tree v = TREE_OPERAND(t, 0); + switch (TREE_CODE(v)) { + case MEM_REF: + for (i = 0; i < TREE_OPERAND_LENGTH(v); ++i) + if (is_local_address_taken(TREE_OPERAND(v, i))) + return 1; + return 0; + case COMPONENT_REF: + while (v && TREE_CODE(v) == COMPONENT_REF) + v = TREE_OPERAND(v, 0); + break; + case VAR_DECL: + default: + ; + } + if (v && TREE_CODE(v) == VAR_DECL && !is_global_var(v)) { + FOR_EACH_VEC_ELT(tree, cfun->local_decls, i, local_var) { + if (local_var == v) + return 1; + } + } + } + return 0; +} + +static int record_or_union_type_has_array(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(field_type); + if (TREE_CODE(field_type) == ARRAY_TYPE) + return 1; + } + } + return 0; +} + /* Expand all variables used in the function. */ static void @@ -1516,6 +1560,8 @@ expand_used_vars (void) VEC(tree,heap) *maybe_local_decls = NULL; unsigned i; unsigned len; + int gen_stack_protect_signal = 0; + basic_block bb; /* Compute the phase of the stack frame for this function. */ { @@ -1548,6 +1594,63 @@ expand_used_vars (void) } } + /* Examine each basic block for address taking of local variables. */ + FOR_EACH_BB(bb) { + gimple_stmt_iterator si; + /* Scanning phis. */ + for (si = gsi_start_phis(bb); !gsi_end_p(si); gsi_next(&si)) { + gimple phi_stmt = gsi_stmt(si); + unsigned int i; + for (i = 0; i < gimple_phi_num_args(phi_stmt); ++i) + if (is_local_address_taken(gimple_phi_arg(phi_stmt, i)->def)) + ++gen_stack_protect_signal; + } + /* Scanning assignments and calls. */ + for (si = gsi_start_bb(bb); !gen_stack_protect_signal && !gsi_end_p(si); + gsi_next(&si)) { + gimple stmt = gsi_stmt (si); + if (is_gimple_assign(stmt)) { + switch(gimple_assign_rhs_class(stmt)) { + case GIMPLE_TERNARY_RHS: + if (is_local_address_taken(gimple_assign_rhs3(stmt))) { + ++gen_stack_protect_signal; + break; + } + /* Otherwise, fall through. */ + case GIMPLE_BINARY_RHS: + if (is_local_address_taken(gimple_assign_rhs2(stmt))) { + ++gen_stack_protect_signal; + break; + } + case GIMPLE_SINGLE_RHS: + case GIMPLE_UNARY_RHS: + if (is_local_address_taken(gimple_assign_rhs1(stmt))) + ++gen_stack_protect_signal; + break; + case GIMPLE_INVALID_RHS: + break; + } + } + + if (!gen_stack_protect_signal && is_gimple_call(stmt)) { + int ii, num_arg = gimple_call_num_args(stmt); + for (ii = 0; !gen_stack_protect_signal && ii < num_arg; ++ii) + if (is_local_address_taken(gimple_call_arg(stmt, ii))) + ++gen_stack_protect_signal; + } + } + } + + /* Examine local variable declaration. */ + if (!gen_stack_protect_signal) + FOR_EACH_LOCAL_DECL (cfun, i, var) + if (TREE_CODE(var) == VAR_DECL && !is_global_var(var)) { + tree var_type = TREE_TYPE(var); + gen_stack_protect_signal += (TREE_CODE(var_type) == ARRAY_TYPE) || + (RECORD_OR_UNION_TYPE_P(var_type) && + record_or_union_type_has_array(var_type)); + } + /* At this point all variables on the local_decls with TREE_USED set are not associated with any block scope. Lay them out. */ @@ -1640,9 +1743,13 @@ expand_used_vars (void) /* 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))) + if (flag_stack_protect == 2 /* -fstack-protector-all */ + || (flag_stack_protect == 1 /* -fstack-protector */ + && (cfun->calls_alloca || has_protected_decls))) { + create_stack_guard (); + } else if (flag_stack_protect == 3 && /* -fstack-protector-strong */ + (gen_stack_protect_signal || + 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 55d3f2d..1ad9717 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1848,6 +1848,10 @@ fstack-protector-all Common Report RejectNegative Var(flag_stack_protect, 2) Use a stack protection method for every function +fstack-protector-strong +Common Report RejectNegative Var(flag_stack_protect, 3) +Use a smart stack protection method for certain functions + fstack-usage Common RejectNegative Var(flag_stack_usage) Output stack usage information on a per-function basis 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..44225f5 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c @@ -0,0 +1,110 @@ +/* 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)); +} + +/* Address taken on struct. */ +int foo10() { + struct BB bb; + int i; + memset(&bb, 5, sizeof bb); + for (i = 0; i < 10; ++i) { + bb.one = 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 } } */ + + -- This patch is available for review at http://codereview.appspot.com/5461043
