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

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.

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?

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