diff mbox

[RFA/RFC] Stack clash mitigation patch 01/08 V2

Message ID 87360ef8-85ba-3bc3-18a0-f1333d9f6889@redhat.com
State New
Headers show

Commit Message

Jeff Law July 19, 2017, 5:17 a.m. UTC
The biggest change in this update to patch 01/08 is moving of stack
clash protection out of -fstack-check= and into its own option,
-fstack-clash-protection.  I believe other issues raised by reviewers
have been addressed as well.

--

This patch introduces a new option -fstack-clash-protection designed to
protect the code GCC generates against stack-clash style attacks.

This patch also introduces dejagnu bits that are later used in tests.
The idea was to introduce dejagnu functions which describe aspects of
the target and have the tests adjust their expectations based on those
dejagnu functions rather than on a target name.

Finally, this patch introduces one new test of note.  Some targets have
call instructions that store a return pointer into the stack and we take
advantage of that ISA feature to avoid some explicit probes.

This optimization is restricted to cases where the caller does not have
a frame of its own (because there's no reasonable way to tear that frame
down on the return path).

However, a sufficiently smart compiler could realize that a call to a
noreturn function could be converted into a jump, even if the caller has
a frame because that frame need not be torn down.  Thus it would be
possible for a function calling a noreturn function to advance the stack
into the guard without actually touching the guard page, which breaks
the assumption that the call instruction would touch the guard
triggering a fault for that case.

GCC doesn't currently optimize that case for various reasons, but it
seemed prudent to go ahead and explicitly verify that with a test.


Thoughts?  OK for the trunk?
* common.opt (-fstack-clash-protection): New option.
	* flag-types.h (enum stack_check_type): Note difference between
	-fstack-check= and -fstack-clash-protection.
	* toplev.c (process_options): Handle -fstack-clash-protection.
	* doc/invoke.texi (-fstack-clash-protection): Document new option.
	(-fstack-check): Note additional problem with -fstack-check=generic.
	Note that -fstack-check is primarily for Ada and refer users
	to -fstack-clash-protection for stack-clash-protection.
	(-fstack-clash-protection): Document.

testsuite/

	* gcc.dg/stack-check-2.c: New test.
	* lib/target-supports.exp
	(check_effective_target_supports_stack_clash_protection): New function.
	(check_effective_target_frame_pointer_for_non_leaf): Likewise.
	(check_effective_target_caller_implicit_probes): Likewise.

Comments

Segher Boessenkool July 20, 2017, 1:23 p.m. UTC | #1
Hi Jeff,

On Tue, Jul 18, 2017 at 11:17:19PM -0600, Jeff Law wrote:
> 
> The biggest change in this update to patch 01/08 is moving of stack
> clash protection out of -fstack-check= and into its own option,
> -fstack-clash-protection.  I believe other issues raised by reviewers
> have been addressed as well.

I think the documentation for the new option should say this only
provides partial protection on targets that do not have fuller protection
enabled.  Is there some way for users to find out which targets those
are / if their target is one of those?


Segher
Jeff Law July 21, 2017, 5:36 p.m. UTC | #2
On 07/20/2017 07:23 AM, Segher Boessenkool wrote:
> Hi Jeff,
> 
> On Tue, Jul 18, 2017 at 11:17:19PM -0600, Jeff Law wrote:
>>
>> The biggest change in this update to patch 01/08 is moving of stack
>> clash protection out of -fstack-check= and into its own option,
>> -fstack-clash-protection.  I believe other issues raised by reviewers
>> have been addressed as well.
> 
> I think the documentation for the new option should say this only
> provides partial protection on targets that do not have fuller protection
> enabled.  Is there some way for users to find out which targets those
> are / if their target is one of those?
Agreed.  I've added a note in the documentation.

Sadly, there's no way short of listing them and keeping that list
up-to-date over time by hand.  If we want to do that, I would suggest
we note the processors with full support as well as those with partial
support using the -fstack-check=specific prologues.

I think we ought to provide that target specific information in the
documentation, though I worry it will get out of date.  Thoughts?

jeff
Segher Boessenkool July 24, 2017, 8:57 a.m. UTC | #3
On Fri, Jul 21, 2017 at 11:36:05AM -0600, Jeff Law wrote:
> On 07/20/2017 07:23 AM, Segher Boessenkool wrote:
> > On Tue, Jul 18, 2017 at 11:17:19PM -0600, Jeff Law wrote:
> > I think the documentation for the new option should say this only
> > provides partial protection on targets that do not have fuller protection
> > enabled.  Is there some way for users to find out which targets those
> > are / if their target is one of those?
> Agreed.  I've added a note in the documentation.

Thanks.

> Sadly, there's no way short of listing them and keeping that list
> up-to-date over time by hand.  If we want to do that, I would suggest
> we note the processors with full support as well as those with partial
> support using the -fstack-check=specific prologues.

Oh, I thought everything got the partial support.  Targets that get _no_
protection should just warn whenever someone asks for it, I think.

> I think we ought to provide that target specific information in the
> documentation, though I worry it will get out of date.  Thoughts?

It's the same as with any other target-specific note in the generic
documentation...  You'll just have to hope if someone implements a
cool feature for some target they remember to advertise that.


Segher
Jeff Law July 24, 2017, 8:43 p.m. UTC | #4
On 07/24/2017 02:57 AM, Segher Boessenkool wrote:
>> Sadly, there's no way short of listing them and keeping that list
>> up-to-date over time by hand.  If we want to do that, I would suggest
>> we note the processors with full support as well as those with partial
>> support using the -fstack-check=specific prologues.
> 
> Oh, I thought everything got the partial support.  Targets that get _no_
> protection should just warn whenever someone asks for it, I think.
Almost everything gets support for probing the dynamic area (exceptions
would be ports that define their own expander for allocating dynamic
stack space).

We could probably set up a hook to query the backend WRT what, if any,
prologue support for either stack-clash protection, or
-fstack=check=specific protection they provide.

THe default hook would probably look something like


enum target_stack_clash_protection_status {
  /* Target provides no protection for stack clash attacks.  */
  NO_PROTECTION,

  /* Target provides partial protection in its prologue using
     -fstack-check=specific prologues.  */
  STACK_CHECK_PROTECTION,

  /* Target provides full protection in its prologue using
     custom prologue sequences for -fstack-clash-protection.   */
  FULL_STACK_CLASH_PROTECTION
}

enum target_stack_clash_protection_status
foo ()
{
  if (STACK_CHECK_STATIC_BUILTIN)
    return STACK_CHECK_PROTECTION
  return NO_PROTECTION
}


Note any target providing stack-clash protection via custom prologues
would have to override the default.  BUt that seems reasonable to me.


Given something like that, we could do something like

if (flag_stack_clash_protection)
  {
    if (foo () == NO_PROTECTION)
      warning ("Static stack allocations are not protected from
stack-clash.);
    else if (foo () == STACK_CHECK_PROTECTION)
      warning ("Static stack allocations are partially protected from
stack-clash");
    else if (foo () == FULL_STACK_CLASH_PROTECTION)
      ;
  }

We could so something similar for the dynamic area.  The default would
key off having a viable expander for allocate_stack.  If it does, then
it would default to unprotected dynamic allocations.  Targets such as
ppc would override the default as it has the expander, but the expander
knows how to deal with stack clash protection.

Jeff
diff mbox

Patch

diff --git a/gcc/common.opt b/gcc/common.opt
index e81165c..cfaf2bc 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2306,6 +2306,11 @@  fstack-check
 Common Alias(fstack-check=, specific, no)
 Insert stack checking code into the program.  Same as -fstack-check=specific.
 
+fstack-clash-protection
+Common Report Var(flag_stack_clash_protection)
+Insert code to probe each page of stack space as it is allocated to protect
+from stack-clash style attacks
+
 fstack-limit
 Common Var(common_deferred_options) Defer
 
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 3e5cee8..bf1298d 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -11333,7 +11333,8 @@  target support in the compiler but comes with the following drawbacks:
 @enumerate
 @item
 Modified allocation strategy for large objects: they are always
-allocated dynamically if their size exceeds a fixed threshold.
+allocated dynamically if their size exceeds a fixed threshold.  Note this
+may change the semantics of some code.
 
 @item
 Fixed limit on the size of the static frame of functions: when it is
@@ -11348,6 +11349,19 @@  generic implementation, code performance is hampered.
 Note that old-style stack checking is also the fallback method for
 @samp{specific} if no target support has been added in the compiler.
 
+@samp{-fstack-check=} is designed for Ada's needs to detect infinite recursion
+and stack overflows.  @samp{specific} is an excellent choice when compiling
+Ada code.  It is not generally sufficient to protect against stack-clash
+attacks.  To protect against those you want @samp{-fstack-clash-protection}.
+
+@item -fstack-clash-protection
+@opindex fstack-clash-protection
+Generate code to prevent stack clash style attacks.  When this option is
+enabled, the compiler will only allocate one page of stack space at a time
+and each page is accessed immediately after allocation.  Thus, it prevents
+allocations from jumping over any stack guard page provided by the
+operating system.
+
 @item -fstack-limit-register=@var{reg}
 @itemx -fstack-limit-symbol=@var{sym}
 @itemx -fno-stack-limit
diff --git a/gcc/flag-types.h b/gcc/flag-types.h
index 5faade5..8874cba 100644
--- a/gcc/flag-types.h
+++ b/gcc/flag-types.h
@@ -166,7 +166,14 @@  enum permitted_flt_eval_methods
   PERMITTED_FLT_EVAL_METHODS_C11
 };
 
-/* Type of stack check.  */
+/* Type of stack check.
+
+   Stack checking is designed to detect infinite recursion for Ada
+   programs.  Furthermore stack checking tries to ensure that scenario
+   that enough stack space is left to run a signal handler.
+
+   -fstack-check= does not prevent stack-clash style attacks.  For that
+   you want -fstack-clash-protection.  */
 enum stack_check_type
 {
   /* Do not check the stack.  */
diff --git a/gcc/testsuite/gcc.dg/stack-check-2.c b/gcc/testsuite/gcc.dg/stack-check-2.c
new file mode 100644
index 0000000..196c4bb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/stack-check-2.c
@@ -0,0 +1,66 @@ 
+/* The goal here is to ensure that we never consider a call to a noreturn
+   function as a potential tail call.
+
+   Right now GCC discovers potential tail calls by looking at the
+   predecessors of the exit block.  A call to a non-return function
+   has no successors and thus can never match that first filter.
+
+   But that could change one day and we want to catch it.  The problem
+   is the compiler could potentially optimize a tail call to a nonreturn
+   function, even if the caller has a frame.  That breaks the assumption
+   that calls probe *sp when saving the return address that some targets
+   depend on to elide stack probes.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O2 -fstack-clash-protection -fdump-tree-tailc -fdump-tree-optimized" } */
+/* { dg-require-effective-target supports_stack_clash_protection } */
+
+extern void foo (void) __attribute__ ((__noreturn__));
+
+
+void
+test_direct_1 (void)
+{
+  foo ();
+}
+
+void
+test_direct_2 (void)
+{
+  return foo ();
+}
+
+void (*indirect)(void)__attribute__ ((noreturn));
+
+
+void
+test_indirect_1 ()
+{
+  (*indirect)();
+}
+
+void
+test_indirect_2 (void)
+{
+  return (*indirect)();;
+}
+
+
+typedef void (*pvfn)() __attribute__ ((noreturn));
+
+void (*indirect_casted)(void);
+
+void
+test_indirect_casted_1 ()
+{
+  (*(pvfn)indirect_casted)();
+}
+
+void
+test_indirect_casted_2 (void)
+{
+  return (*(pvfn)indirect_casted)();
+}
+/* { dg-final { scan-tree-dump-not "tail call" "tailc" } } */
+/* { dg-final { scan-tree-dump-not "tail call" "optimized" } } */
+
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index fe5e777..25c3c80 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -8468,3 +8468,78 @@  proc check_effective_target_arm_coproc4_ok { } {
     return [check_cached_effective_target arm_coproc4_ok \
 		check_effective_target_arm_coproc4_ok_nocache]
 }
+
+# Return 1 if the target has support for stack probing designed
+# to avoid stack-clash style attacks.
+#
+# This is used to restrict the stack-clash mitigation tests to
+# just those targets that have been explicitly supported.
+# 
+# In addition to the prologue work on those targets, each target's
+# properties should be described in the functions below so that
+# tests do not become a mess of unreadable target conditions.
+# 
+proc check_effective_target_supports_stack_clash_protection { } {
+  if { [istarget aarch*-*-*] || [istarget x86_64-*-*]
+       || [istarget i?86-*-*] || [istarget s390*-*-*]
+       || [istarget powerpc*-*-*] || [istarget rs6000*-*-*] } {
+	return 1
+  }
+  return 0
+}
+
+# Return 1 if the target creates a frame pointer for non-leaf functions
+# Note we ignore cases where we apply tail call optimization here.
+proc check_effective_target_frame_pointer_for_non_leaf { } {
+  if { [istarget aarch*-*-*] } {
+	return 1
+  }
+  return 0
+}
+
+# Return 1 if the target's calling sequence or its ABI
+# create implicit stack probes at or prior to function entry.
+proc check_effective_target_caller_implicit_probes { } {
+
+  # On x86/x86_64 the call instruction itself pushes the return
+  # address onto the stack.  That is an implicit probe of *sp.
+  if { [istarget x86_64-*-*] || [istarget i?86-*-*] } {
+	return 1
+  }
+
+  # On PPC, the ABI mandates that the address of the outer
+  # frame be stored at *sp.  Thus each allocation of stack
+  # space is itself an implicit probe of *sp.
+  if { [istarget powerpc*-*-*] || [istarget rs6000*-*-*] } {
+	return 1
+  }
+
+  # s390's ABI has a register save area allocated by the
+  # caller for use by the callee.  The mere existence does
+  # not constitute a probe by the caller, but when the slots
+  # used by the callee those stores are implicit probes.
+  if { [istarget s390*-*-*] } {
+	return 1
+  }
+
+  # Not strictly true on aarch64, but we have agreed that we will
+  # consider any function that pushes SP more than 3kbytes into
+  # the guard page as broken.  This essentially means that we can
+  # consider the aarch64 as having a caller implicit probe at
+  # *(sp + 1k).
+  if { [istarget aarch64*-*-*] } {
+	return 1;
+  }
+
+  return 0
+}
+
+# Targets that potentially realign the stack pointer often cause residual
+# stack allocations and make it difficult to elimination loops or residual
+# allocations for dynamic stack allocations
+proc check_effective_target_callee_realigns_stack { } {
+  if { [istarget x86_64-*-*] || [istarget i?86-*-*] } {
+	return 1
+  }
+  return 0
+}
diff --git a/gcc/toplev.c b/gcc/toplev.c
index e6c69a4..24a00df 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1591,6 +1591,26 @@  process_options (void)
       flag_associative_math = 0;
     }
 
+  /* -fstack-clash-protection is not currently supported on targets
+     where the stack grows up.  */
+  if (flag_stack_clash_protection && !STACK_GROWS_DOWNWARD)
+    {
+      warning_at (UNKNOWN_LOCATION, 0,
+		  "-fstack-clash_protection is not supproted on targets "
+		  "where the stack grows from lower to higher addresses");
+      flag_stack_clash_protection = 0;
+    }
+
+  /* We can not support -fstack-check= and -fstack-clash-protection at
+     the same time.  */
+  if (flag_stack_check != NO_STACK_CHECK && flag_stack_clash_protection)
+    {
+      warning_at (UNKNOWN_LOCATION, 0,
+		  "-fstack-check= and -fstack-clash_protection are mutually "
+		  "exclusive.  Disabling -fstack-check=");
+      flag_stack_check = NO_STACK_CHECK;
+    }
+
   /* With -fcx-limited-range, we do cheap and quick complex arithmetic.  */
   if (flag_cx_limited_range)
     flag_complex_method = 0;