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

[Florian Weimer <fweimer at redhat dot com>]

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?

--
Florian Weimer / Red Hat Product Security Team