This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][RFA/RFC] Stack clash mitigation patch 01/08
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?
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).
> 1. We never probe ahead of need. ie, if a function requests N bytes of
> stack space, then we'll never probe beyond N bytes. In contrast
> -fstack-check=specific will tend to probe 2-3 pages beyond the N byte
> request.
s/need/immediate need/. Nod.
> 2. We probe as each page is allocated. in contrast most
> -fstack-check=specific implementations allocate all the space, then
> probe into the space.
Right, and that leaves some dangerous openings for exploitation.
> 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.
> > 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).
> 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.
> >> /* 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.
> >> +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/ ?
Segher