[PATCH] Add a new option "-fstack-protector-strong" (patch / doc inside)

[Han Shen(ææ) <shenhan at google dot com>]

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