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