diff mbox series

Improve -fstack-protector-strong (PR middle-end/92825)

Message ID 20191210092715.GX10088@tucnak
State New
Headers show
Series Improve -fstack-protector-strong (PR middle-end/92825) | expand

Commit Message

Jakub Jelinek Dec. 10, 2019, 9:27 a.m. UTC 
Hi!

As mentioned in the PR, -fstack-protector-strong in some cases instruments
even functions which except for the stack canary setup and later testing of
that don't touch the stack at all.

This is because for -fstack-protector-strong we setup the canary
whenever stack_protect_decl_p returns true, and that goes through all local
decls and if it is a var that is not a global var and is either address
taken or is array or has arrays embedded in its type, returns true.
Now, the problem is that by going through all non-global variables, it goes
even through variables that are or have arrays, but we allocate them in
registers like in the testcase below, or in theory for nested functions
it could be variables from the parent function too.
Variables which live in registers, even when they have arrays in them,
shouldn't really cause stack overflows of any kind, out of bounds accesses
to them never cause out of bounds stack accesses, usually they ought to be
already optimized away, arrays that are accessed through non-constant
indexes should cause variables to live in memory.

Another issue is that the record_or_union_type_has_array_p function duplicates
the behavior of stack_protect_classify_type when it returns the SPCT_HAS_ARRAY
bit set.

Initially, I thought I'd just move the stack_protect_decl_p call later when
the stack_vars array is computed and instead of FOR_EACH_LOCAL_DECL
walk the stack_vars array.  But for the discovery of stack_vars that are or
have arrays that is a waste of time, all such variables are already picked
for phase 1 or phase 2 and thus has_protected_decls bool will be set if
there are any and we already do create the stack canary if
has_protected_decls is set.  So, the only thing that remains is catch
variables that aren't or don't contain arrays, but are address taken.  Those
go into the last unnumbered phase, but we still want to create stack canary.
Instead of adding yet another walk I've done that in a function that already
walks them.

In addition to this, I've tried to clarify this in the documentation.  For
-fstack-protector, we've been doing it this way already before, optimized
away or arrays living in registers didn't count, because we only walked
stack-vars.  And, the documentation also said that only > 8 byte arrays are
protected with -stack-protector, but it is actually >= 8 byte (or whatever
the ssp-buffer-size param is).

Bootstrapped/regtested on x86_64-linux and i686-linux, in addition regtested
(just check-gcc, check-c++-all and check-target-libstdc++-v3) with
--target_board=unix/-fstack-protector-strong on both.
Ok for trunk?

2019-12-10  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/92825
	* cfgexpand.c (add_stack_protection_conflicts): Change return type
	from void to bool, return true if at least one stack_vars[i].decl
	is addressable.
	(record_or_union_type_has_array_p, stack_protect_decl_p): Remove.
	(expand_used_vars): Don't call stack_protect_decl_p, instead for
	-fstack-protector-strong set gen_stack_protect_signal to true
	if add_stack_protection_conflicts returned true.  Formatting fixes.
	* doc/invoke.texi (-fstack-protector-strong): Clarify that optimized
	out variables or variables not living on the stack don't count.
	(-fstack-protector): Likewise.  Clarify it affects >= 8 byte arrays
	rather than > 8 byte.

	* gcc.target/i386/pr92825.c: New test.


	Jakub

Comments

Jeff Law Dec. 10, 2019, 8:52 p.m. UTC | #1 
On Tue, 2019-12-10 at 10:27 +0100, Jakub Jelinek wrote:
> Hi!
> 
> As mentioned in the PR, -fstack-protector-strong in some cases
> instruments
> even functions which except for the stack canary setup and later
> testing of
> that don't touch the stack at all.
> 
> This is because for -fstack-protector-strong we setup the canary
> whenever stack_protect_decl_p returns true, and that goes through all
> local
> decls and if it is a var that is not a global var and is either
> address
> taken or is array or has arrays embedded in its type, returns true.
> Now, the problem is that by going through all non-global variables,
> it goes
> even through variables that are or have arrays, but we allocate them
> in
> registers like in the testcase below, or in theory for nested
> functions
> it could be variables from the parent function too.
> Variables which live in registers, even when they have arrays in
> them,
> shouldn't really cause stack overflows of any kind, out of bounds
> accesses
> to them never cause out of bounds stack accesses, usually they ought
> to be
> already optimized away, arrays that are accessed through non-constant
> indexes should cause variables to live in memory.
> 
> Another issue is that the record_or_union_type_has_array_p function
> duplicates
> the behavior of stack_protect_classify_type when it returns the
> SPCT_HAS_ARRAY
> bit set.
> 
> Initially, I thought I'd just move the stack_protect_decl_p call
> later when
> the stack_vars array is computed and instead of FOR_EACH_LOCAL_DECL
> walk the stack_vars array.  But for the discovery of stack_vars that
> are or
> have arrays that is a waste of time, all such variables are already
> picked
> for phase 1 or phase 2 and thus has_protected_decls bool will be set
> if
> there are any and we already do create the stack canary if
> has_protected_decls is set.  So, the only thing that remains is catch
> variables that aren't or don't contain arrays, but are address
> taken.  Those
> go into the last unnumbered phase, but we still want to create stack
> canary.
> Instead of adding yet another walk I've done that in a function that
> already
> walks them.
> 
> In addition to this, I've tried to clarify this in the
> documentation.  For
> -fstack-protector, we've been doing it this way already before,
> optimized
> away or arrays living in registers didn't count, because we only
> walked
> stack-vars.  And, the documentation also said that only > 8 byte
> arrays are
> protected with -stack-protector, but it is actually >= 8 byte (or
> whatever
> the ssp-buffer-size param is).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, in addition
> regtested
> (just check-gcc, check-c++-all and check-target-libstdc++-v3) with
> --target_board=unix/-fstack-protector-strong on both.
> Ok for trunk?
> 
> 2019-12-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/92825
> 	* cfgexpand.c (add_stack_protection_conflicts): Change return
> type
> 	from void to bool, return true if at least one
> stack_vars[i].decl
> 	is addressable.
> 	(record_or_union_type_has_array_p, stack_protect_decl_p):
> Remove.
> 	(expand_used_vars): Don't call stack_protect_decl_p, instead
> for
> 	-fstack-protector-strong set gen_stack_protect_signal to true
> 	if add_stack_protection_conflicts returned true.  Formatting
> fixes.
> 	* doc/invoke.texi (-fstack-protector-strong): Clarify that
> optimized
> 	out variables or variables not living on the stack don't count.
> 	(-fstack-protector): Likewise.  Clarify it affects >= 8 byte
> arrays
> 	rather than > 8 byte.
> 
> 	* gcc.target/i386/pr92825.c: New test.
OK
jeff
>
diff mbox series

Patch

--- gcc/cfgexpand.c.jj	2019-12-06 09:51:56.917899249 +0100
+++ gcc/cfgexpand.c	2019-12-09 13:29:16.518031095 +0100
@@ -1888,17 +1888,23 @@  asan_decl_phase_3 (size_t i)
 }
 
 /* Ensure that variables in different stack protection phases conflict
-   so that they are not merged and share the same stack slot.  */
+   so that they are not merged and share the same stack slot.
+   Return true if there are any address taken variables.  */
 
-static void
+static bool
 add_stack_protection_conflicts (void)
 {
   size_t i, j, n = stack_vars_num;
   unsigned char *phase;
+  bool ret = false;
 
   phase = XNEWVEC (unsigned char, n);
   for (i = 0; i < n; ++i)
-    phase[i] = stack_protect_decl_phase (stack_vars[i].decl);
+    {
+      phase[i] = stack_protect_decl_phase (stack_vars[i].decl);
+      if (TREE_ADDRESSABLE (stack_vars[i].decl))
+	ret = true;
+    }
 
   for (i = 0; i < n; ++i)
     {
@@ -1909,6 +1915,7 @@  add_stack_protection_conflicts (void)
     }
 
   XDELETEVEC (phase);
+  return ret;
 }
 
 /* Create a decl for the guard at the top of the stack frame.  */
@@ -1993,50 +2000,6 @@  estimated_stack_frame_size (struct cgrap
   return estimated_poly_value (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;
-}
-
-/* Check if the current function has local referenced variables that
-   have their addresses taken, contain an array, or are arrays.  */
-
-static bool
-stack_protect_decl_p ()
-{
-  unsigned i;
-  tree var;
-
-  FOR_EACH_LOCAL_DECL (cfun, i, var)
-    if (!is_global_var (var))
-      {
-	tree var_type = TREE_TYPE (var);
-	if (VAR_P (var)
-	    && (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))))
-	  return true;
-      }
-  return false;
-}
-
 /* Check if the current function has calls that use a return slot.  */
 
 static bool
@@ -2103,8 +2066,7 @@  expand_used_vars (void)
     }
 
   if (flag_stack_protect == SPCT_FLAG_STRONG)
-      gen_stack_protect_signal
-	= stack_protect_decl_p () || stack_protect_return_slot_p ();
+    gen_stack_protect_signal = stack_protect_return_slot_p ();
 
   /* At this point all variables on the local_decls with TREE_USED
      set are not associated with any block scope.  Lay them out.  */
@@ -2180,6 +2142,8 @@  expand_used_vars (void)
 
   if (stack_vars_num > 0)
     {
+      bool has_addressable_vars = false;
+
       add_scope_conflicts ();
 
       /* If stack protection is enabled, we don't share space between
@@ -2189,7 +2153,10 @@  expand_used_vars (void)
 	      || (flag_stack_protect == SPCT_FLAG_EXPLICIT
 		  && lookup_attribute ("stack_protect",
 				       DECL_ATTRIBUTES (current_function_decl)))))
-	add_stack_protection_conflicts ();
+	has_addressable_vars = add_stack_protection_conflicts ();
+
+      if (flag_stack_protect == SPCT_FLAG_STRONG && has_addressable_vars)
+	gen_stack_protect_signal = true;
 
       /* Now that we have collected all stack variables, and have computed a
 	 minimal interference graph, attempt to save some stack space.  */
@@ -2206,14 +2173,16 @@  expand_used_vars (void)
 
     case SPCT_FLAG_STRONG:
       if (gen_stack_protect_signal
-	  || cfun->calls_alloca || has_protected_decls
+	  || cfun->calls_alloca
+	  || has_protected_decls
 	  || lookup_attribute ("stack_protect",
 			       DECL_ATTRIBUTES (current_function_decl)))
 	create_stack_guard ();
       break;
 
     case SPCT_FLAG_DEFAULT:
-      if (cfun->calls_alloca || has_protected_decls
+      if (cfun->calls_alloca
+	  || has_protected_decls
 	  || lookup_attribute ("stack_protect",
 			       DECL_ATTRIBUTES (current_function_decl)))
 	create_stack_guard ();
@@ -2224,8 +2193,9 @@  expand_used_vars (void)
 			    DECL_ATTRIBUTES (current_function_decl)))
 	create_stack_guard ();
       break;
+
     default:
-      ;
+      break;
     }
 
   /* Assign rtl to each variable based on these partitions.  */
--- gcc/doc/invoke.texi.jj	2019-12-06 00:40:39.468705299 +0100
+++ gcc/doc/invoke.texi	2019-12-09 13:44:16.172408953 +0100
@@ -13006,9 +13006,12 @@  on Intel Control-flow Enforcement Techno
 Emit extra code to check for buffer overflows, such as stack smashing
 attacks.  This is done by adding a guard variable to functions with
 vulnerable objects.  This includes functions that call @code{alloca}, and
-functions with buffers larger than 8 bytes.  The guards are initialized
-when a function is entered and then checked when the function exits.
-If a guard check fails, an error message is printed and the program exits.
+functions with buffers larger than or equal to 8 bytes.  The guards are
+initialized when a function is entered and then checked when the function
+exits.  If a guard check fails, an error message is printed and the program
+exits.  Only variables that are actually allocated on the stack are
+considered, optimized away variables or variables allocated in registers
+don't count.
 
 @item -fstack-protector-all
 @opindex fstack-protector-all
@@ -13018,7 +13021,9 @@  Like @option{-fstack-protector} except t
 @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.
+references to local frame addresses.  Only variables that are actually
+allocated on the stack are considered, optimized away variables or variables
+allocated in registers don't count.
 
 @item -fstack-protector-explicit
 @opindex fstack-protector-explicit
--- gcc/testsuite/gcc.target/i386/pr92825.c.jj	2019-12-09 14:49:38.691965021 +0100
+++ gcc/testsuite/gcc.target/i386/pr92825.c	2019-12-09 15:01:31.385176831 +0100
@@ -0,0 +1,15 @@ 
+/* PR middle-end/92825 */
+/* { dg-do compile { target fstack_protector } } */
+/* { dg-options "-O2 -fstack-protector-strong" } */
+/* { dg-final { scan-assembler-not "__stack_chk_fail" } } */
+
+int
+foo (int r, int g, int b)
+{
+  union U { int rgba; char p[4]; } u;
+  u.p[0] = r;
+  u.p[1] = g;
+  u.p[2] = b;
+  u.p[3] = -1;
+  return u.rgba;
+}