Enhance stack protector for calling no return function
I find current stack protector didn't work for calling no return function.
This is an defect for "stack protector", so let's refine it.
Differential D139254
Enhance stack protector xiangzhangllvm on Dec 3 2022, 3:11 AM. Authored by
Details
Enhance stack protector for calling no return function I find current stack protector didn't work for calling no return function.
Diff Detail
Event TimelineComment Actions There's a problem in the previous iteration of this patch (https://reviews.llvm.org/D138774). It made clang hang while compiling the following short snippet reduced from an open-source library (compiled on x86-64, linux): $ cat q.c char *texsubexpr(char *expression, char *subexpr) { char *texchar(); if (*expression) return texchar(expression, subexpr); } void __stack_chk_fail (void) { __builtin_trap (); } $ clang -O1 -w -fstack-protector-all -c q.c -o q.o Please ensure this is solved here. Comment Actions I check it, the clang still hang, let me check and fix it. Thanks very much!
Comment Actions I root cause it, this is a very special case : ) This was a defect in StackProtector and exposed by current patch. Let me fix it. Thanks again!
Comment Actions Add test stack-protector-recursively.ll to check stack protector auto generated function (__stack_chk_fail).
Comment Actions This diff is a ~0.5% size regression under -Oz when building with -fstack-protector. Is this reasonably expected? And one that should be expected given that it's considered to be a correctness fix? cc @smeenai Comment Actions I think the size increasement is expected because it inserts more stack corruption checks. Do you find any unexpected check? Comment Actions Would there be any objections to adding a flag to restore the previous behavior? This is a fairly hefty size increase for us, and we don't think the additional checking is worth the size increase in our case. We also intend to look into the size increases more to determine how much is from guarding calls to e.g. __cxa_throw, where you will reuse the stack frame, vs. calls to e.g. abort or _Unwind_Resume, where the stack frame should never be reused and checking the stack cookie doesn't seem worthwhile (if my logic is sound, which I'm not 100% sure of). Comment Actions I agree to add a option to go in previous behavior. (e.g -fstack-protector-weak or -fstack-protector-return) Comment Actions Yup, I'm not at all doubting the value of this work :) Adding the option will just let everyone make the appropriate trade-off between size and security for their use cases. Comment Actions Hi @smeenai , do you have plan to implement it ? If not, I will do it, but may some days later (due to some other jobs in my hands). Comment Actions I'm going on an extended leave soon, and I won't have time to implement it before then. I've asked some coworkers to take a look, but if you have the time to add the option it'd be hugely appreciated :) Comment Actions
Hi @smeenai I first implement at an llvm option for your requirement, you can use -mllvm -option-name to quick solve your code size problem. Comment Actions Are there possible optimizations which can be implemented to reduce the code-size impact of this change? I'm wondering if this patch adds the checks conservatively to all instances where stack check could potentially, but not necessarily, be missed/useful? Comment Actions Seems good idea, but it is hard to guess how the attacker will attack our program. (which no-return should protect or not), and sorry for I am not a defense expert. Comment Actions I'm happy to see at least there's an option to turn this off. That said, rereading https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58245#c6 I kind of agree with Jakub. What are you protecting exactly? Why do so only for noreturn calls? Why not prior to every call? Why not insert a stack check guard after every operation? If you're that paranoid, do no meaningful work and just keep checking the stack protector. I think this would have been better implemented as a new level beyond -fstack-protector-strong rather than regressing the code size of every user of -fstack-protector-strong. If GCC doesn't do this, is this something ICC did? Comment Actions The full context that motivated this patch is now publicly available in https://bugs.chromium.org/p/llvm/issues/detail?id=30, and I have a much better understanding now. The problem is that you have noreturn functions like abort, which are actually going to terminate your program, and there's no need to check the stack canary before calling them. However, you also have noreturn functions like __cxa_throw and _Unwind_Resume which will cause your program to resume in the unwinder, and the unwinder is relying on saved return addresses on the stack to determine where to go, so not checking the canary before entering the unwinder can cause control flow to be hijacked. Note that this patch by itself is an incomplete solution. @t.p.northover put up D143637 recently to make all unwind paths visible so that the canary can be checked for them. Unfortunately, I've found that patch to basically negate any size benefit from -disable-check-no-return-call. I haven't had the chance to dig into that yet, but maybe Tim has some ideas. If you're also interested in this for size reasons, one specific potential enhancement would be distinguishing abort-like functions from unwind-like functions and avoiding the canary check before the former. I haven't been able to measure the ratio of the two in our code base yet. |
This should increase compiling time. Need refine it by moving the BB child to NewBB.