Re: [PATCH][RFA/RFC] Stack clash mitigation patch 01/08

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi!

On Tue, Jul 11, 2017 at 03:19:57PM -0600, Jeff Law wrote:
> It introduces a new style of stack probing -fstack-check=clash,
> including documentation of the new option, how it differs from
> -fstack-check=specific, etc.
> 
> FWIW -fstack-check=specific is dreadfully named.  I haven't tried to
> address that.

-fstack-check=clash is itself not such a super name either.  It's not
checking stack, and it isn't clashing: it just does a store to every
page the stack will touch (minus the first and last or something).

How does this work for targets that want to enable this by default?  How
does that interact with checking for stack size overflow?

> 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.

Currently GCC will never make a tail call to a noreturn function (see
calls.c:can_implement_as_sibling_call_p).  It would be nice (for code
size, if nothing else) if that was changed.  But you know all this :-)

> @@ -11333,7 +11334,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.

How so?  Semantics of valid (portable) code, as well?  It would be nice
to have some detail here, not just a threat :-)

> +@samp{clash} is designed to prevent jumping the stack guard page as seen in
> +stack clash style attacks.  However, it does not guarantee sufficient stack
> +space to handle a signal in the event that the program hits the stack guard
> +and is thus incompatible with Ada's needs.

Is there some way we could have both?

In principle stack-clash protection is completely orthogonal to
-fstack-check.

>    /* Check the stack and entirely rely on the target configuration
> -     files, i.e. do not use the generic mechanism at all.  */
> +     files, i.e. do not use the generic mechanism at all.  This
> +     does not prevent stack guard jumping and stack clash style
> +     attacks.  */
>    FULL_BUILTIN_STACK_CHECK
>  };

> +      else if (!strcmp (arg, "clash"))
> +	{
> +	  /* This is the stack checking method, designed to prevent
> +	     stack-clash attacks.  */
> +	  if (!STACK_GROWS_DOWNWARD)
> +	    sorry ("-fstack-check=clash not implemented on this target");
> +	  else
> +	    opts->x_flag_stack_check = (STACK_CHECK_BUILTIN
> +				        ? FULL_BUILTIN_STACK_CHECK
> +					: (STACK_CHECK_STATIC_BUILTIN
> +					   ? STACK_CLASH_BUILTIN_STACK_CHECK
> +					   : GENERIC_STACK_CHECK));
> +	}

So targets that define STACK_CHECK_BUILTIN (spu and alpha) do not get
stack clash protection if you asked for it specifically, without warning,
if I read that correctly?

> +# 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

Missing full stop, twice.  More later in the file.

> +proc check_effective_target_stack_clash_protected { } {

The name is maybe not so great: nothing is protected until you actually
use the option.  "supported", maybe?

> +# Return 1 if the target's calling sequence or its ABI
> +# create implicit stack probes at *sp at function
> +# entry.
> +proc check_effective_target_caller_implicit_probes { } {

"At function entry" isn't really true for Power ("when setting up a
stack frame", instead -- and you are required to set one up before
calling anything).

> +# The stack realignment often causes residual stack allocations and
> +# can also make it difficult to elide loops or residual allocations
> +# for dynamic allocations
> +proc check_effective_target_callee_realigns_stack { } {

Does this return 1 if the callee always realigns the stack?  Or maybe
realigns the stack?


Segher