Patchwork Add a new option "-fstack-protector-strong"

login
register
mail settings
Submitter Han Shen
Date April 15, 2013, 9:15 p.m.
Message ID <CACkGtrhn_oyWWAN4xMSgZY4_NOtQ6W0Loxe0W6fxbQucUU7p+A@mail.gmail.com>
Download mbox | patch
Permalink /patch/236728/
State New
Headers show

Comments

Han Shen - April 15, 2013, 9:15 p.m.
Hi, I'm to bring up this patch about '-fstack-protector-strong' for trunk.

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".

Design - see end of email.

Benefit - gain big performance while sacrificing little security (for
scenarios comparing -fstack-protector-all vs.
-fstack-protector-strong)

Status - it has been in google/main for more than 1 year, building the
chrome browser and chromeos with no security degradation over this
period.

Test - dejagnu c/c++ test on 64-bit ubuntu, bootstrap, build/run
chrome browser and chromiumos.

Testifies - LLVM developers are refering my design docs to implement
this stack-protector-strong schema -
http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-October/053931.html

Thanks,

Patch -

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.

    4. Possible performance gain for atom and tegra board
    Replacing “stack-protector-all” with “stack-protector-st

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".

Design - see end of email.
rong” 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.


H.
Florian Weimer - April 16, 2013, 1:32 p.m.
On 04/15/2013 11:15 PM, Han Shen(沈涵) wrote:
> Hi, I'm to bring up this patch about '-fstack-protector-strong' for trunk.
>
> 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".

Thanks for posting this again.  What follows is not a real review, just 
some low-hanging fruits.

Please include the proposed changelog entries.

 > +  if (flag_stack_protect == 3)
 > +    cpp_define (pfile, "__SSP_STRONG__=3");
 >    if (flag_stack_protect == 2)
 >      cpp_define (pfile, "__SSP_ALL__=2");

3 and 2 should be replaced by SPCT_FLAG_STRONG and SPCT_FLAG_ALL.

> +/* 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)
> +      && record_or_union_type_has_array_p (field_type))
> +    return 1;
> +  if (TREE_CODE (field_type) == ARRAY_TYPE)
> +    return 1;
> + }
> +    }
> +  return 0;
> +}

Indentation is off (unless both mail clients I tried are clobbering your 
patch).  I think the GNU coding style prohibits the braces around the 
single-statement body of the outer 'for".

> +
>   /* Expand all variables used in the function.  */
>
>   static rtx
> @@ -1525,6 +1553,7 @@ expand_used_vars (void)
>     struct pointer_map_t *ssa_name_decls;
>     unsigned i;
>     unsigned len;
> +  int gen_stack_protect_signal = 0;

This should be a bool variable, initialized with false, and which is 
assigned true below.

>
>     /* Compute the phase of the stack frame for this function.  */
>     {
> @@ -1576,6 +1605,23 @@ expand_used_vars (void)
>       }
>     pointer_map_destroy (ssa_name_decls);
>
> +  FOR_EACH_LOCAL_DECL (cfun, i, var)
> +    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;
> +  }
> +      }
> +

Indentation again.  This analysis needs to be performed in 
SPCT_FLAG_STRONG mode only, it can be skipped in the other modes.  It 
might make sense to run it only if the other conditions checked below do 
not hold.

>     /* At this point all variables on the local_decls with TREE_USED
>        set are not associated with any block scope.  Lay them out.  */
>
> @@ -1662,11 +1708,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 == SPCT_FLAG_ALL
> +      || (flag_stack_protect == SPCT_FLAG_STRONG
> +  && (gen_stack_protect_signal || cfun->calls_alloca
> +      || has_protected_decls))
> +      || (flag_stack_protect == SPCT_FLAG_DEFAULT
> +  && (cfun->calls_alloca
> +      || has_protected_decls)))
>       create_stack_guard ();

Can you make the conditional more similar to the comment, perhaps using 
a switch statement on the value of the flag_stack_protect variable? 
That's going to be much easier to read.

> +@item -fstack-protector-strong
> +@opindex fstack-protector-strong
> +Like @option{-fstack-protector-strong} but includes additional functions to be
> +protected - those that have local array definitions, or have
> references to local
> +frame addresses.

"Like @option{-fstack-protector}", I presume.  Replace " - " with "---". 
  Does "reference local frame addresses" mean "take addresses of local 
variables" (at least for C/C++)?

> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-O2 -fstack-protector-strong" } */

Do we have a better target selection for the test cases?  I think at 
least rs6000 and s390x should support this as well.

>      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).

What about struct-returning functions?  Internally, an address is passed 
to the called function.  Would they trigger this?  What about the this 
pointer in C++ code?
Gerald Pfeifer - Aug. 18, 2013, 7:30 p.m.
Hi H.,

On Mon, 15 Apr 2013, Han Shen(沈涵) wrote:
> Hi, I'm to bring up this patch about '-fstack-protector-strong' for trunk.
> 
> 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".

the patch has been committed, but I see that the release notes
at http://gcc.gnu.org/gcc-4.9/changes.html do not mentioned this.

Can you please add a note?  (http://gcc.gnu.org/about.html has
some background on our web site setup, and I am working to
consolidate all our documentation in this area -- plus I am
happy to lend a helping hand.)

Gerald

Patch

diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index 3e210d9..0059626 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -888,6 +888,8 @@  c_cpp_builtins (cpp_reader *pfile)
   /* Make the choice of the stack protector runtime visible to source code.
      The macro names and values here were chosen for compatibility with an
      earlier implementation, i.e. ProPolice.  */
+  if (flag_stack_protect == 3)
+    cpp_define (pfile, "__SSP_STRONG__=3");
   if (flag_stack_protect == 2)
     cpp_define (pfile, "__SSP_ALL__=2");
   else if (flag_stack_protect == 1)
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a651d8c..8728842 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1291,6 +1291,10 @@  clear_tree_used (tree block)
     clear_tree_used (t);
 }

+#define SPCT_FLAG_ALL 2
+#define SPCT_FLAG_DEFAULT 1
+#define SPCT_FLAG_STRONG 3
+
 /* Examine TYPE and determine a bit mask of the following features.  */

 #define SPCT_HAS_LARGE_CHAR_ARRAY 1
@@ -1360,7 +1364,8 @@  stack_protect_decl_phase (tree decl)
   if (bits & SPCT_HAS_SMALL_CHAR_ARRAY)
     has_short_buffer = true;

-  if (flag_stack_protect == 2)
+  if (flag_stack_protect == SPCT_FLAG_ALL ||
+      flag_stack_protect == SPCT_FLAG_STRONG)
     {
       if ((bits & (SPCT_HAS_SMALL_CHAR_ARRAY | SPCT_HAS_LARGE_CHAR_ARRAY))
   && !(bits & SPCT_HAS_AGGREGATE))
@@ -1514,6 +1519,29 @@  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)
+      && record_or_union_type_has_array_p (field_type))
+    return 1;
+  if (TREE_CODE (field_type) == ARRAY_TYPE)
+    return 1;
+ }
+    }
+  return 0;
+}
+
 /* Expand all variables used in the function.  */

 static rtx
@@ -1525,6 +1553,7 @@  expand_used_vars (void)
   struct pointer_map_t *ssa_name_decls;
   unsigned i;
   unsigned len;
+  int gen_stack_protect_signal = 0;

   /* Compute the phase of the stack frame for this function.  */
   {
@@ -1576,6 +1605,23 @@  expand_used_vars (void)
     }
   pointer_map_destroy (ssa_name_decls);

+  FOR_EACH_LOCAL_DECL (cfun, i, var)
+    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.  */

@@ -1662,11 +1708,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 == SPCT_FLAG_ALL
+      || (flag_stack_protect == SPCT_FLAG_STRONG
+  && (gen_stack_protect_signal || cfun->calls_alloca
+      || has_protected_decls))
+      || (flag_stack_protect == SPCT_FLAG_DEFAULT
+  && (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 6b9b2e1..26b1fbb 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1914,6 +1914,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/doc/cpp.texi b/gcc/doc/cpp.texi
index 4e7b05c..c605b3b 100644
--- a/gcc/doc/cpp.texi
+++ b/gcc/doc/cpp.texi
@@ -2349,6 +2349,10 @@  use.
 This macro is defined, with value 2, when @option{-fstack-protector-all} is
 in use.

+@item __SSP_STRONG__
+This macro is defined, with value 3, when @option{-fstack-protector-strong} is
+in use.
+
 @item __SANITIZE_ADDRESS__
 This macro is defined, with value 1, when @option{-fsanitize=address} is
 in use.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 1652ebc..e5eb64e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -406,8 +406,8 @@  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
--fthread-jumps -ftracer -ftree-bit-ccp @gol
+-fstack-protector-all -fstack-protector-strong -fstrict-aliasing @gol
+-fstrict-overflow -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
 -ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol
@@ -8880,6 +8880,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-strong} 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/gcc.c b/gcc/gcc.c
index bcfbfc8..f098137 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -655,7 +655,7 @@  proper position among the other output files.  */
 #ifdef TARGET_LIBC_PROVIDES_SSP
 #define LINK_SSP_SPEC "%{fstack-protector:}"
 #else
-#define LINK_SSP_SPEC
"%{fstack-protector|fstack-protector-all:-lssp_nonshared -lssp}"
+#define LINK_SSP_SPEC
"%{fstack-protector|fstack-protector-strong|fstack-protector-all:-lssp_nonshared
-lssp}"
 #endif
 #endif

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