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

[Jeff Law <law at redhat dot com>]

On 07/13/2017 03:32 PM, Segher Boessenkool wrote:
> Hi again,
> 
> On Wed, Jul 12, 2017 at 09:56:09PM -0600, Jeff Law wrote:
>>>> 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).
>> Yea.  I don't particularly like it either.  Suggestions?  I considered
>> "probe" as well, but "specific" also does probing.  In the end I used
>> the part of the marketing name of the exploits.
> 
> I don't think it should be inside -fstack-check at all.  Sure, the
> mechanisms implementing it overlap a bit (more on some targets, less
> on others), but how will a user ask for clash protection _and_ for
> stack checking?
The biggest problem with separating them is we would end up with a fair
amount code that ultimately looks like

if (flag_stack_check == STATIC_BUILTIN_STACK_CHECK
    || flag_whatever_the_new_thing_is)
  {
    probe the stack
  }


We can certainly do that.  It'll touch a few more places (particularly
in backends that have checks for Ada stack overflow, but for which I
haven't written stack clash protection), but the changes would be simple
and repetitive.




> So something like -fstack-clash-protection, -fstack-touch-all-pages,
> together with whatever -fstack-check option is wanted (and yes the
> existing ones have very non-specific, non-descriptive, non-obvious names).
Given the heavy use of "stack clash" in the press I'd lean towards
-fstack-clash-protection.  THat makes it easier for folks to find the
right option to protect themselves.

>> Certainly open to ideas on the interface aspects.
> 
> The interface is much harder to change than any aspect of the GCC
> implementation.  Have to get it right at once, almost.
Yup.  It gets backed in even faster than normal in this case I suspect.
Red Hat will almost certainly pick up the bits and start backporting
them to RHEL 7, RHEL 6 and RHEL 5.  So the flag will be out in the wild
before gcc-8 hits the streets.

If you go back to my original message from several weeks ago, getting
the UI right and the basic concepts were my primary goals.  Details like
what probe instruction to use are, IMHO, much less important.


> 
>>> How does this work for targets that want to enable this by default?  How
>>> does that interact with checking for stack size overflow?
>> I don't currently have a way to enable it by default -- for my tests I
>> just slam the value I want into the initializer in common.opt :-)
>>
>> It's independent of stack size overflow checking.  They could (in
>> theory) even co-exist on ports that support stack size overflow
>> checking, but I didn't test that.
> 
> Okay, that was my impression as well.  But that interface won't allow
> it (unless every -fstack-check=X gets an -fstack-check=X+clash twin).
THere may have been some mis-communication here, but it's a good place
to spend some time thinking about interplay between options.

First, there's -fstack-limit-*.  Essentially there's an RTX which
defines the limit for the stack pointer.  If you cross that limit a trap
gets executed.  PPC, s390 and likely other ports have support for this.
It should interoperate with -fstack-clash-protection and/or -fstack-check.

Some ports have their own probing option.  x86 comes to mind.  The x86
uses that for stack probing on windows where extending the stack has to
happen explicitly (which is why they're not subject to stack-clash style
attacks).  The options probably shouldn't be mixed.


There's -fstack-check and -fstack-clash-protection.  I think with the
direction we're going they are fundamentally incompatible because
neither the compiler nor kernel do anything to guarantee enough stack is
available after hitting the guard for -fstack-clash-protection.

And there's shrink wrapping.  This was originally raised by the aarch64
guys and deserves some thought.  I'm particularly worried about aarch64
if it was to shrink-wrap some particular register saves that we wanted
to use as an implicit probe.

There may be others that I'm missing.



> 
>> We created that alloca'd object at the wrong lexical scope which mucked
>> up its expected persistence.  I'm sure I'd spot it trivially once I set
>> up the test again.
> 
> That sounds like it might be a bug even.
That was my thought as well.  It's c-torture/execute/20071029-1.c.


> 
>>>>    /* 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?
>> That's an unknown.  I'd have to dig into the guts of the alpha and spu
>> to understand precisely how their STACK_CHECK_BUILTIN works.
> 
> Both just define it to 1.  So this code then specialises to
> 
>       else if (!strcmp (arg, "clash"))
> 	{
> 	  opts->x_flag_stack_check = FULL_BUILTIN_STACK_CHECK;
> 	}
> 
> which it says above does *not* protect against stack clash attacks.
> Which seems backward.
Right.  That was one of the other reasons why my initial patches left
those enums alone and introduced a new flag -- probing policy.

Those enum constants, depending on how you read them almost imply levels
of backend support for Ada style stack overflow checking.   That was one
of the reasons why early (not posted) versions of my patch left that
stuff alone -- instead they introduced the separate concept of probing
policy.  Jakub argued internally to put things under -fstack-check.






> 
>>>> +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?
>> I hate all the names...  "supports_stack_clash_protection" perhaps?
> 
> Works for me!
> 
>>>> +# 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).
>> I think it's close enough -- I'll ponder a better name.  s390x doesn't
>> really have caller implicit probes either, but stack saves in the callee
>> act like them (because the caller allocates the space for the callee's
>> save area).
> 
> Oh the name is fine, but maybe expand the comment a bit.  Or even just
> s/at function entry/for every function/ ?
I'll use "at function entry" if I can't come up with something better
and expand the comments.

None of the options are strictly correct for s390, but "at function
entry" is less incorrect than "for every function".

jeff