Extend -fstack-protector-strong to cover calls with return slot

Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

This patch fixes a loophole in the -fstack-protector-strong protection. 
  If a function call uses the return slot optimization, the caller needs 
stack protector instrumentation because the return slot is addressable.

Bootstrapped and regression-tested on x86_64-redhat-linux-gnu, with 
C/C++/Java enabled.  Okay for trunk?

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: ssp-strong-nrv.patch --]
[-- Type: text/x-patch, Size: 4221 bytes --]

gcc/

2014-01-03  Florian Weimer  <fweimer@redhat.com>

	* cfgexpand.c (call_with_return_slot_opt_p): New function.
	(expand_used_vars): Emit stack protector instrumentation in strong
	mode if call_with_return_slot_opt_p.

gcc/testsuite/

2014-01-03  Florian Weimer  <fweimer@redhat.com>

	* gcc.dg/fstack-protector-strong.c: Add coverage for named return
	values.
	* g++.dg/fstack-protector-strong.C: Likewise.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 206311)
+++ gcc/cfgexpand.c	(working copy)
@@ -1599,6 +1599,22 @@
   return 0;
 }
 
+/* Check if the basic block has a call which uses a return slot.  */
+
+static bool
+call_with_return_slot_opt_p (basic_block bb)
+{
+  for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
+       !gsi_end_p (gsi); gsi_next (&gsi))
+    {
+      gimple stmt = gsi_stmt (gsi);
+      if (gimple_code (stmt) == GIMPLE_CALL
+	  && gimple_call_return_slot_opt_p (stmt))
+	return true;
+    }
+  return false;
+}
+
 /* Expand all variables used in the function.  */
 
 static rtx
@@ -1669,22 +1685,35 @@
   pointer_map_destroy (ssa_name_decls);
 
   if (flag_stack_protect == SPCT_FLAG_STRONG)
-    FOR_EACH_LOCAL_DECL (cfun, i, var)
-      if (!is_global_var (var))
+    {
+      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 = true;
+		break;
+	      }
+	  }
+      /* The return slot introduces addressable local variables.  */
+      if (!gen_stack_protect_signal)
 	{
-	  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))))
+	  basic_block bb;
+	  FOR_ALL_BB_FN (bb, cfun)
 	    {
-	      gen_stack_protect_signal = true;
-	      break;
+	      gen_stack_protect_signal = call_with_return_slot_opt_p (bb);
+	      if (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.  */
Index: gcc/testsuite/g++.dg/fstack-protector-strong.C
===================================================================
--- gcc/testsuite/g++.dg/fstack-protector-strong.C	(revision 206311)
+++ gcc/testsuite/g++.dg/fstack-protector-strong.C	(working copy)
@@ -32,4 +32,39 @@
   return global_func (a);
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */
+/* Frame addressed exposed through return slot. */
+
+struct B
+{
+  /* Discourage passing this struct in registers. */
+  int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+};
+
+B global_func ();
+void noop ();
+
+int foo3 ()
+{
+  return global_func ().a1;
+}
+
+int foo4 ()
+{
+  try {
+    noop ();
+    return 0;
+  } catch (...) {
+    return global_func ().a1;
+  }
+}
+
+int foo5 ()
+{
+  try {
+    return global_func ().a1;
+  } catch (...) {
+    return 0;
+  }
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 5 } } */
Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
===================================================================
--- gcc/testsuite/gcc.dg/fstack-protector-strong.c	(revision 206311)
+++ gcc/testsuite/gcc.dg/fstack-protector-strong.c	(working copy)
@@ -131,4 +131,17 @@
   return bb.three;
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
+struct B
+{
+  /* Discourage passing this struct in registers. */
+  int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+};
+
+struct B global3 (void);
+
+int foo11 ()
+{
+  return global3 ().a1;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 11 } } */

Re: Extend -fstack-protector-strong to cover calls with return slot

[Jakub Jelinek]

On Fri, Jan 03, 2014 at 02:15:51PM +0100, Florian Weimer wrote:
> This patch fixes a loophole in the -fstack-protector-strong
> protection.  If a function call uses the return slot optimization,
> the caller needs stack protector instrumentation because the return
> slot is addressable.
> 
> Bootstrapped and regression-tested on x86_64-redhat-linux-gnu, with
> C/C++/Java enabled.  Okay for trunk?
> 
> -- 
> Florian Weimer / Red Hat Product Security Team

> 2014-01-03  Florian Weimer  <fweimer@redhat.com>
> 
> 	* cfgexpand.c (call_with_return_slot_opt_p): New function.
> 	(expand_used_vars): Emit stack protector instrumentation in strong
> 	mode if call_with_return_slot_opt_p.
> 
> gcc/testsuite/
> 
> 2014-01-03  Florian Weimer  <fweimer@redhat.com>
> 
> 	* gcc.dg/fstack-protector-strong.c: Add coverage for named return
> 	values.
> 	* g++.dg/fstack-protector-strong.C: Likewise.
> 
> Index: gcc/cfgexpand.c
> ===================================================================
> --- gcc/cfgexpand.c	(revision 206311)
> +++ gcc/cfgexpand.c	(working copy)
> @@ -1599,6 +1599,22 @@
>    return 0;
>  }
>  
> +/* Check if the basic block has a call which uses a return slot.  */
> +
> +static bool
> +call_with_return_slot_opt_p (basic_block bb)
> +{
> +  for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
> +       !gsi_end_p (gsi); gsi_next (&gsi))
> +    {
> +      gimple stmt = gsi_stmt (gsi);
> +      if (gimple_code (stmt) == GIMPLE_CALL

That would be is_gimple_call (stmt) instead.

Also, I'd think the function is misnamed, given that it checks if there
are any calls with return_slot_opt_p in a bb.  I think it would be
better to move FOR_ALL_BB_FN (bb, cfun) loop also into the
function and call it any_call_...

Lastly, I wonder if gimple_call_return_slot_opt_p is really what you are
after, why does NRV matter here?  Isn't what you are looking for instead
whether the called function returns value through invisible reference,
because then you'll always have some (aggregate) addressable object
in the caller's frame and supposedly you are after making sure that the
callee doesn't overflow the return object.

So, looking at tree-nrv.c, that check would be roughly:
          if (is_gimple_call (stmt)
              && gimple_call_lhs (stmt)
              && aggregate_value_p (TREE_TYPE (gimple_call_lhs (stmt)),
                                    gimple_call_fndecl (stmt)))

	Jakub

Re: Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

On 01/03/2014 07:57 PM, Jakub Jelinek wrote:

>> +/* Check if the basic block has a call which uses a return slot.  */
>> +
>> +static bool
>> +call_with_return_slot_opt_p (basic_block bb)
>> +{
>> +  for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
>> +       !gsi_end_p (gsi); gsi_next (&gsi))
>> +    {
>> +      gimple stmt = gsi_stmt (gsi);
>> +      if (gimple_code (stmt) == GIMPLE_CALL
>
> That would be is_gimple_call (stmt) instead.

Ah, it's not used consistently everywhere, and I got it from of the 
leftover places.

> Also, I'd think the function is misnamed, given that it checks if there
> are any calls with return_slot_opt_p in a bb.  I think it would be
> better to move FOR_ALL_BB_FN (bb, cfun) loop also into the
> function and call it any_call_...

I should probably move both loops (the one for declarations and the one 
for basic blocks) into its own function.

> Lastly, I wonder if gimple_call_return_slot_opt_p is really what you are
> after, why does NRV matter here?

The C code we generate does not construct the returned value in place 
(presumably because the partial write would be visible with threads, 
longjmp etc.), unlike the C++ code.

That's why I'm interested in instrumenting NRV-able calls only.  But 
gimple_call_return_slot_opt_p doesn't actually give me that.  The GIMPLE 
from the C test case looks like this (before and after applying your 
proposal):

foo11 ()
{
   int D.2265;
   struct B D.2266;

   D.2266 = global3 (); [return slot optimization]
   D.2265 = D.2266.a1;
   return D.2265;
}

In both cases, SSP instrumentation is applied:

         .type   foo11, @function
foo11:
.LFB24:
         .cfi_startproc
         subq    $56, %rsp
         .cfi_def_cfa_offset 64
         movq    %rsp, %rdi
         movq    %fs:40, %rax
         movq    %rax, 40(%rsp)
         xorl    %eax, %eax
         call    global3
         movq    40(%rsp), %rdx
         xorq    %fs:40, %rdx
         movl    (%rsp), %eax
         jne     .L50
         addq    $56, %rsp
         .cfi_remember_state
         .cfi_def_cfa_offset 8
         ret
.L50:
         .cfi_restore_state
         call    __stack_chk_fail
         .cfi_endproc

> Isn't what you are looking for instead
> whether the called function returns value through invisible reference,
> because then you'll always have some (aggregate) addressable object
> in the caller's frame and supposedly you are after making sure that the
> callee doesn't overflow the return object.
>
> So, looking at tree-nrv.c, that check would be roughly:
>            if (is_gimple_call (stmt)
>                && gimple_call_lhs (stmt)
>                && aggregate_value_p (TREE_TYPE (gimple_call_lhs (stmt)),
>                                      gimple_call_fndecl (stmt)))

When I do that, I get SSP instrumentation even when the struct is small 
enough to be returned in registers.  gimple_call_return_slot_opt_p 
returns false in this case.  So gimple_call_return_slot_opt_p appears to 
be misnamed (it's an ABI thing, not really an optimization), but it's 
closer to what I want.

-- 
Florian Weimer / Red Hat Product Security Team

Re: Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

On 01/03/2014 10:43 PM, Florian Weimer wrote:

>> Lastly, I wonder if gimple_call_return_slot_opt_p is really what you are
>> after, why does NRV matter here?
>
> The C code we generate does not construct the returned value in place
> (presumably because the partial write would be visible with threads,
> longjmp etc.), unlike the C++ code.
>
> That's why I'm interested in instrumenting NRV-able calls only.  But
> gimple_call_return_slot_opt_p doesn't actually give me that.  The GIMPLE
> from the C test case looks like this (before and after applying your
> proposal):

I thought about this some more and I think it makes sense to add the 
instrumentation each time the return slot is used, both for C and C++. 
We don't if the called function is implemented in C or C++, so 
language-specific instrumentation is not entirely accurate.

I'm attaching a second version of the patch, splitting out the decl and 
bb analysis and using is_gimple_call.  Bootstrapped and 
regression-tested on x86_64-redhat-linux-gnu.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: ssp-strong-return-slot.patch --]
[-- Type: text/x-patch, Size: 4169 bytes --]

gcc/

2014-01-07  Florian Weimer  <fweimer@redhat.com>

	* cfgexpand.c (stack_protect_decl_p): New function, extracted from
	expand_used_vars.
	(stack_protect_return_slot_p): New function.
	(expand_used_vars): Call stack_protect_decl_p and
	stack_protect_return_slot_p for -fstack-protector-strong.

gcc/testsuite/

2014-01-07  Florian Weimer  <fweimer@redhat.com>

	* gcc.dg/fstack-protector-strong.c: Add coverage for return slots.
	* g++.dg/fstack-protector-strong.C: Likewise.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 206311)
+++ gcc/cfgexpand.c	(working copy)
@@ -1599,6 +1599,47 @@
   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 (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))))
+	  return true;
+      }
+  return false;
+}
+
+/* Check if the current function has calls that use a return slot.  */
+
+static bool
+stack_protect_return_slot_p ()
+{
+  basic_block bb;
+  
+  FOR_ALL_BB_FN (bb, cfun)
+    for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
+	 !gsi_end_p (gsi); gsi_next (&gsi))
+      {
+	gimple stmt = gsi_stmt (gsi);
+	if (is_gimple_call (stmt) && gimple_call_return_slot_opt_p (stmt))
+	  return true;
+      }
+  return false;
+}
+
 /* Expand all variables used in the function.  */
 
 static rtx
@@ -1669,22 +1710,8 @@
   pointer_map_destroy (ssa_name_decls);
 
   if (flag_stack_protect == SPCT_FLAG_STRONG)
-    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 = true;
-	      break;
-	    }
-	}
+      gen_stack_protect_signal
+	= stack_protect_decl_p () || 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.  */
Index: gcc/testsuite/g++.dg/fstack-protector-strong.C
===================================================================
--- gcc/testsuite/g++.dg/fstack-protector-strong.C	(revision 206311)
+++ gcc/testsuite/g++.dg/fstack-protector-strong.C	(working copy)
@@ -32,4 +32,39 @@
   return global_func (a);
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */
+/* Frame addressed exposed through return slot. */
+
+struct B
+{
+  /* Discourage passing this struct in registers. */
+  int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+};
+
+B global_func ();
+void noop ();
+
+int foo3 ()
+{
+  return global_func ().a1;
+}
+
+int foo4 ()
+{
+  try {
+    noop ();
+    return 0;
+  } catch (...) {
+    return global_func ().a1;
+  }
+}
+
+int foo5 ()
+{
+  try {
+    return global_func ().a1;
+  } catch (...) {
+    return 0;
+  }
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 5 } } */
Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
===================================================================
--- gcc/testsuite/gcc.dg/fstack-protector-strong.c	(revision 206311)
+++ gcc/testsuite/gcc.dg/fstack-protector-strong.c	(working copy)
@@ -131,4 +131,17 @@
   return bb.three;
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
+struct B
+{
+  /* Discourage passing this struct in registers. */
+  int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+};
+
+struct B global3 (void);
+
+int foo11 ()
+{
+  return global3 ().a1;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 11 } } */

Re: Extend -fstack-protector-strong to cover calls with return slot

[Jakub Jelinek]

On Tue, Jan 07, 2014 at 01:51:00PM +0100, Florian Weimer wrote:
> +static bool
> +stack_protect_return_slot_p ()
> +{
> +  basic_block bb;
> +  
> +  FOR_ALL_BB_FN (bb, cfun)
> +    for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
> +	 !gsi_end_p (gsi); gsi_next (&gsi))
> +      {
> +	gimple stmt = gsi_stmt (gsi);
> +	if (is_gimple_call (stmt) && gimple_call_return_slot_opt_p (stmt))

I have to repeat, this is not the right test, it really is just an
optimization hint, nothing else.  Just look where it is set (unless it is
C++ where some NRV is performed in the FE as mandated by the languages)
- in pass_return_slot, which is an optimization pass, not run with -O0 or
-Og at all.  So, you'd stack protect something only if not -O0 or -Og, that
is just wrong, and only if it was suitable for NRV.  You really want to
protect all functions that call something that is returned by hidden
reference.

	Jakub

Re: Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

On 01/07/2014 02:07 PM, Jakub Jelinek wrote:
> On Tue, Jan 07, 2014 at 01:51:00PM +0100, Florian Weimer wrote:
>> +static bool
>> +stack_protect_return_slot_p ()
>> +{
>> +  basic_block bb;
>> +
>> +  FOR_ALL_BB_FN (bb, cfun)
>> +    for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
>> +	 !gsi_end_p (gsi); gsi_next (&gsi))
>> +      {
>> +	gimple stmt = gsi_stmt (gsi);
>> +	if (is_gimple_call (stmt) && gimple_call_return_slot_opt_p (stmt))
>
> I have to repeat, this is not the right test, it really is just an
> optimization hint, nothing else.  Just look where it is set (unless it is
> C++ where some NRV is performed in the FE as mandated by the languages)
> - in pass_return_slot, which is an optimization pass, not run with -O0 or
> -Og at all.

I don't understand.  The corresponding tree flag is set by 
gimplify_modify_expr_rhs, in the CALL_EXPR case:

	      if (use_target)
		{
		  CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
		  mark_addressable (*to_p);
		}

Isn't this an integral part of the gimplifier which also runs at -O0?  I 
certainly see the flag in the generated gimple at -O0, with the amended 
C test case:

foo11 ()
{
   int D.2037;
   struct B D.2038;

   D.2038 = global3 (); [return slot optimization]
   D.2037 = D.2038.a1;
   return D.2037;
}

As I said, I think the function gimple_call_return_slot_opt_p is 
misnamed because it's not actually an optimization.

-- 
Florian Weimer / Red Hat Product Security Team

Re: Extend -fstack-protector-strong to cover calls with return slot

[Jakub Jelinek]

On Tue, Jan 07, 2014 at 02:27:04PM +0100, Florian Weimer wrote:
> gimplify_modify_expr_rhs, in the CALL_EXPR case:
> 
> 	      if (use_target)
> 		{
> 		  CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
> 		  mark_addressable (*to_p);
> 		}

Yeah, that sets it in some cases too, not in other testcases.

Just look at how the flag is used when actually expanding it:

        if (target && MEM_P (target) && CALL_EXPR_RETURN_SLOT_OPT (exp))
          structure_value_addr = XEXP (target, 0);
        else
          {
            /* For variable-sized objects, we must be called with a target
               specified.  If we were to allocate space on the stack here,
               we would have no way of knowing when to free it.  */
            rtx d = assign_temp (rettype, 1, 1);
            structure_value_addr = XEXP (d, 0);
            target = 0;
          }

so, if it is set, the address of the var on the LHS is passed to the
function as hidden argument, if it is not set, we pass address of
a stack temporary instead.  Both the automatic var and the stack temporary
can overflow, if the callee does something wrong.

	Jakub

Re: Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 1925 bytes --]

On 01/07/2014 02:37 PM, Jakub Jelinek wrote:
> On Tue, Jan 07, 2014 at 02:27:04PM +0100, Florian Weimer wrote:
>> gimplify_modify_expr_rhs, in the CALL_EXPR case:
>>
>> 	      if (use_target)
>> 		{
>> 		  CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
>> 		  mark_addressable (*to_p);
>> 		}
>
> Yeah, that sets it in some cases too, not in other testcases.
>
> Just look at how the flag is used when actually expanding it:
>
>          if (target && MEM_P (target) && CALL_EXPR_RETURN_SLOT_OPT (exp))
>            structure_value_addr = XEXP (target, 0);
>          else
>            {
>              /* For variable-sized objects, we must be called with a target
>                 specified.  If we were to allocate space on the stack here,
>                 we would have no way of knowing when to free it.  */
>              rtx d = assign_temp (rettype, 1, 1);
>              structure_value_addr = XEXP (d, 0);
>              target = 0;
>            }

Okay, I'm beginning to understand.  I tried to actually reach the second 
branch, and ended up with PR59711. :)

foo12 in the new C testcase covers it in part without a variable-sized 
object.

> so, if it is set, the address of the var on the LHS is passed to the
> function as hidden argument, if it is not set, we pass address of
> a stack temporary instead.  Both the automatic var and the stack temporary
> can overflow, if the callee does something wrong.

What about the attached version?  It still does not exactly match your 
original suggestion because gimple_call_lhs (stmt) can be NULL_TREE if 
the result is ignored and this case needs instrumentation, as you 
explained, so I use the function return type in the aggregate_value_p check.

Testing is still under way, but looks good so far.  I'm bootstrapping 
with BOOT_CFLAGS="-O2 -g -fstack-protector-strong" with Ada enabled, for 
additional coverage.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: ssp-strong-return-20140108.patch --]
[-- Type: text/x-patch, Size: 5247 bytes --]

gcc/

2014-01-08  Florian Weimer  <fweimer@redhat.com>

	* cfgexpand.c (stack_protect_decl_p): New function, extracted from
	expand_used_vars.
	(stack_protect_return_slot_p): New function.
	(expand_used_vars): Call stack_protect_decl_p and
	stack_protect_return_slot_p for -fstack-protector-strong.

gcc/testsuite/

2014-01-08  Florian Weimer  <fweimer@redhat.com>

	* gcc.dg/fstack-protector-strong.c: Add coverage for return slots.
	* g++.dg/fstack-protector-strong.C: Likewise.
	* gcc.target/i386/ssp-strong-reg.c: New file.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 206311)
+++ gcc/cfgexpand.c	(working copy)
@@ -1599,6 +1599,52 @@
   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 (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))))
+	  return true;
+      }
+  return false;
+}
+
+/* Check if the current function has calls that use a return slot.  */
+
+static bool
+stack_protect_return_slot_p ()
+{
+  basic_block bb;
+  
+  FOR_ALL_BB_FN (bb, cfun)
+    for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
+	 !gsi_end_p (gsi); gsi_next (&gsi))
+      {
+	gimple stmt = gsi_stmt (gsi);
+	/* This assumes that calls to internal-only functions never
+	   use a return slot.  */
+	if (is_gimple_call (stmt)
+	    && !gimple_call_internal_p (stmt)
+	    && aggregate_value_p (TREE_TYPE (gimple_call_fntype (stmt)),
+				  gimple_call_fndecl (stmt)))
+	  return true;
+      }
+  return false;
+}
+
 /* Expand all variables used in the function.  */
 
 static rtx
@@ -1669,22 +1715,8 @@
   pointer_map_destroy (ssa_name_decls);
 
   if (flag_stack_protect == SPCT_FLAG_STRONG)
-    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 = true;
-	      break;
-	    }
-	}
+      gen_stack_protect_signal
+	= stack_protect_decl_p () || 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.  */
Index: gcc/testsuite/g++.dg/fstack-protector-strong.C
===================================================================
--- gcc/testsuite/g++.dg/fstack-protector-strong.C	(revision 206311)
+++ gcc/testsuite/g++.dg/fstack-protector-strong.C	(working copy)
@@ -32,4 +32,52 @@
   return global_func (a);
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */
+/* Frame addressed exposed through return slot. */
+
+struct B
+{
+  /* Discourage passing this struct in registers. */
+  int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+  int method ();
+  B return_slot();
+};
+
+B global_func ();
+void noop ();
+
+int foo3 ()
+{
+  return global_func ().a1;
+}
+
+int foo4 ()
+{
+  try {
+    noop ();
+    return 0;
+  } catch (...) {
+    return global_func ().a1;
+  }
+}
+
+int foo5 ()
+{
+  try {
+    return global_func ().a1;
+  } catch (...) {
+    return 0;
+  }
+}
+
+int foo6 ()
+{
+  B b;
+  return b.method ();
+}
+
+int foo7 (B *p)
+{
+  return p->return_slot ().a1;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 7 } } */
Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
===================================================================
--- gcc/testsuite/gcc.dg/fstack-protector-strong.c	(revision 206311)
+++ gcc/testsuite/gcc.dg/fstack-protector-strong.c	(working copy)
@@ -131,4 +131,22 @@
   return bb.three;
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
+struct B
+{
+  /* Discourage passing this struct in registers. */
+  int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+};
+
+struct B global3 (void);
+
+int foo11 ()
+{
+  return global3 ().a1;
+}
+
+void foo12 ()
+{
+  global3 ();
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 12 } } */
Index: gcc/testsuite/gcc.target/i386/ssp-strong-reg.c
===================================================================
--- gcc/testsuite/gcc.target/i386/ssp-strong-reg.c	(revision 0)
+++ gcc/testsuite/gcc.target/i386/ssp-strong-reg.c	(working copy)
@@ -0,0 +1,19 @@
+/* Test that structs returned in registers do not lead to
+   instrumentation with -fstack-protector-strong.  */
+
+/* { dg-do compile { target { ! { ia32 } } } } */
+/* { dg-options "-O2 -fstack-protector-strong" } */
+
+struct S {
+  int a;
+  int b;
+};
+
+struct S f (void);
+
+int g (void)
+{
+  return f ().a;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 0 } } */

Re: Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

On 01/08/2014 03:57 PM, Florian Weimer wrote:

> What about the attached version?  It still does not exactly match your
> original suggestion because gimple_call_lhs (stmt) can be NULL_TREE if
> the result is ignored and this case needs instrumentation, as you
> explained, so I use the function return type in the aggregate_value_p
> check.
>
> Testing is still under way, but looks good so far.  I'm bootstrapping
> with BOOT_CFLAGS="-O2 -g -fstack-protector-strong" with Ada enabled, for
> additional coverage.

Testing passed without new regressions.  Is this okay for trunk?

-- 
Florian Weimer / Red Hat Product Security Team

[PATCH Ping] Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

On 01/17/2014 11:26 AM, Florian Weimer wrote:
> On 01/08/2014 03:57 PM, Florian Weimer wrote:
>
>> What about the attached version?  It still does not exactly match your
>> original suggestion because gimple_call_lhs (stmt) can be NULL_TREE if
>> the result is ignored and this case needs instrumentation, as you
>> explained, so I use the function return type in the aggregate_value_p
>> check.
>>
>> Testing is still under way, but looks good so far.  I'm bootstrapping
>> with BOOT_CFLAGS="-O2 -g -fstack-protector-strong" with Ada enabled, for
>> additional coverage.
>
> Testing passed without new regressions.  Is this okay for trunk?

Ping?  Thanks.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH Ping v2] Extend -fstack-protector-strong to cover calls with return slot

[Florian Weimer]

On 02/03/2014 10:05 AM, Florian Weimer wrote:
> On 01/17/2014 11:26 AM, Florian Weimer wrote:
>> On 01/08/2014 03:57 PM, Florian Weimer wrote:
>>
>>> What about the attached version?  It still does not exactly match your
>>> original suggestion because gimple_call_lhs (stmt) can be NULL_TREE if
>>> the result is ignored and this case needs instrumentation, as you
>>> explained, so I use the function return type in the aggregate_value_p
>>> check.
>>>
>>> Testing is still under way, but looks good so far.  I'm bootstrapping
>>> with BOOT_CFLAGS="-O2 -g -fstack-protector-strong" with Ada enabled, for
>>> additional coverage.
>>
>> Testing passed without new regressions.  Is this okay for trunk?
>
> Ping?  Thanks.

Now that 4.9 is released, I'd like to propose again this fix for inclusion:

<http://gcc.gnu.org/ml/gcc-patches/2014-01/msg00374.html>

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH Ping v2] Extend -fstack-protector-strong to cover calls with return slot

[Jeff Law]

On 05/05/14 05:58, Florian Weimer wrote:
> On 02/03/2014 10:05 AM, Florian Weimer wrote:
>> On 01/17/2014 11:26 AM, Florian Weimer wrote:
>>> On 01/08/2014 03:57 PM, Florian Weimer wrote:
>>>
>>>> What about the attached version?  It still does not exactly match your
>>>> original suggestion because gimple_call_lhs (stmt) can be NULL_TREE if
>>>> the result is ignored and this case needs instrumentation, as you
>>>> explained, so I use the function return type in the aggregate_value_p
>>>> check.
>>>>
>>>> Testing is still under way, but looks good so far.  I'm bootstrapping
>>>> with BOOT_CFLAGS="-O2 -g -fstack-protector-strong" with Ada enabled,
>>>> for
>>>> additional coverage.
>>>
>>> Testing passed without new regressions.  Is this okay for trunk?
>>
>> Ping?  Thanks.
>
> Now that 4.9 is released, I'd like to propose again this fix for inclusion:
>
> <http://gcc.gnu.org/ml/gcc-patches/2014-01/msg00374.html>
OK for the trunk.

jeff