diff mbox

[4.7,google] Adding a new option -fstack-protector-strong. (issue5461043)

Message ID 20111208010750.E7FC86215E@shenhan.mtv.corp.google.com
State New
Headers show

Commit Message

Han Shen Dec. 8, 2011, 1:07 a.m. UTC
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".


--
This patch is available for review at http://codereview.appspot.com/5461043

Comments

Andrew Pinski Dec. 8, 2011, 1:19 a.m. UTC | #1
On Wed, Dec 7, 2011 at 5:07 PM, Han Shen <shenhan@google.com> wrote:
> +  /* Examine each basic block for address taking of local variables. */

I don't think you need to look at the basic blocks to figure out if a
local variable has its address taken.  You can look through referenced
variables and see if it is a local variable and TREE_ADDRESSABLE is
set.  This should speed up the code a lot.  Though that might has some
false positives with arrays but that is ok because you are checking if
there are any arrays already anyways.

Thanks,
Andrew Pinski
Richard Biener Dec. 8, 2011, 9:07 a.m. UTC | #2
On Thu, Dec 8, 2011 at 2:19 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Dec 7, 2011 at 5:07 PM, Han Shen <shenhan@google.com> wrote:
>> +  /* Examine each basic block for address taking of local variables. */
>
> I don't think you need to look at the basic blocks to figure out if a
> local variable has its address taken.  You can look through referenced
> variables and see if it is a local variable and TREE_ADDRESSABLE is
> set.  This should speed up the code a lot.  Though that might has some
> false positives with arrays but that is ok because you are checking if
> there are any arrays already anyways.

Indeed.

Also your patch does not adhere to the GCC coding standards.
For example all functions need toplevel comments documenting
their purpose and their parameters.

Richard.

> Thanks,
> Andrew Pinski
Diego Novillo Dec. 8, 2011, 1:18 p.m. UTC | #3
On Wed, Dec 7, 2011 at 20:07, Han Shen <shenhan@google.com> wrote:

> Status - implemented internally, to be up-streamed or merged to google branch only.

Why would you not consider sending this for trunk at the next stage 1?

(patch review in progress)

Diego.
diff mbox

Patch

=========================
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 } } */
+
+