This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add a new option "-fstack-protector-strong"

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
+ || (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?

Florian Weimer / Red Hat Product Security Team

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]