Forked from internal google bug b/166163480. If we have code that is compiled without a stack protector (such as code in the Linux kernel that's trying to restore registers containing the stack canaries), then inlining code that was compiled with stack protector into it can break code. LTO makes this more likely to occur across TUs which may or may not be compiled with the same -fstack-protector flag. The inliner probably should not inline functions if the caller does not use a stack protector, but the callee does. Toy example: // foo.c #include <alloca.h> void foo(void *); void bar(size_t n) { foo(alloca(n)); } __attribute__((no_stack_protector)) void baz(void) { bar(1024); } $ clang -O2 baz.c -c -fstack-protector-strong $ llvm-objdump -dr baz.o 0000000000000000 <bar>: ... 41: c3 retq 42: e8 00 00 00 00 callq 0x47 <bar+0x47> 0000000000000043: R_X86_64_PLT32 __stack_chk_fail-0x4 ... 0000000000000050 <baz>: ... 8a: c3 retq 8b: e8 00 00 00 00 callq 0x90 <baz+0x40> 000000000000008c: R_X86_64_PLT32 __stack_chk_fail-0x4 Oh, no! `bar` was compiled with stack protection, was inlined into `baz` (bad), which now has stack protection, even though we explicitly disabled it for that function.
Maybe this bug is in the front end? $ clang -O2 baz.c -c -fstack-protector-strong -emit-llvm -S produces bar and baz with the same function attributes: to have a stack protector. So it looks like the front end dropped the __attribute__((__no_stack_protector__)), (in IR, it looks like the attribute is called `sspstrong`.) or rather kept `sspstrong`?
Yeah, what... // baz2.c #include <alloca.h> void foo(void *); void bar(size_t n) { foo(alloca(n)); } __attribute__((no_stack_protector)) void baz(void) { bar(1024); } __attribute__((no_stack_protector)) void quux(size_t n) { foo(alloca(n)); } $ clang -O2 baz.c -c -fstack-protector-strong -emit-llvm -S bar -> sspstrong (good) baz -> sspstrong (bad) quux -> "" (good)
adjustCallerSSPLevel looks fishy.
Ah, adjustCallerSSPLevel can't disambiguate between the two cases of: 1. the caller had no attributes and should be "upgraded" to the inlined callee's stack protection level. 2. the caller had the C attribute "no_stack_protector" and should not be "upgraded" to the inlined callee's stack protection level, since nothing is left in the IR to show the existence of the C attribute. Another question is; would we prevent inlining, or prevent "upgrading" the stack protection level? I'd go for avoiding inlining, but then the inliner will face the same ambiguity. I think we need the no_stack_protector C attribute to leave behind a new function level IR attribute. We might need a new EnumAttr in llvm/include/llvm/IR/Attributes.td for "nostackp" to disambiguate between explicitly requesting no stack pointer, vs don't care.
+1 on a new enum value for "nostackp". Indeed there is a tri-state here that isn't being respected.
I've started on a patch for this: https://reviews.llvm.org/D87956, which chooses not to inline a stack protected callee into a no stack protector caller. I think the big question I have is: What should happen if the callee is marked always inline?
Landed https://reviews.llvm.org/rGb7926ce6d7a83cdf70c68d82bc3389c04009b841, but there's one more fix I want to do before closing: https://reviews.llvm.org/D87956#2348699.
Two follow up patches: 1. https://reviews.llvm.org/rG0b11d018cc2f2c6bea5dac8dc72140cdb502ca02 2. https://reviews.llvm.org/D90194
Ok, I backed out the previous patches that weren't cleanups, and went with a simpler approach based on feedback: https://reviews.llvm.org/rGbc044a88ee3c16e4164740732a7adc91a29b24f5
Cherry-picked commit bc044a88ee3 ("[Inline] prevent inlining on stack protector mismatch") on top of llvmorg-11.0.1-rc2 Git tag helps to fix the issue. With Nick's baz2.c test-case from comment #2: [ Debian clang version 11.0.1-+rc2-1 ] bar -> sspstrong (good) baz -> sspstrong (bad) quux -> "" (good) [ Selfmade llvmorg-11.0.1-rc2 toolchain + bc044a88ee3 ] bar -> sspstrong (good) baz -> "" (good) quux -> "" (good)
Sorry for all of the churn here, but based on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722#c9, it seems that GCC is not willing to also make this change. Based on that, I'm tempted to revert: commit 780741107e6f ("[LangRef] Update new ssp/sspstrong/sspreq semantics after D91816") commit ee571f87bf41 ("[ThinLTO][test] Fix X86/nossp.ll after D91816") commit bc044a88ee3c ("[Inline] prevent inlining on stack protector mismatch") and instead send the following Linux kernel patch. Thoughts? diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c index db1378c6ff26..baffccce851c 100644 --- a/arch/x86/power/cpu.c +++ b/arch/x86/power/cpu.c @@ -191,7 +191,7 @@ static void fix_processor_context(void) * The asm code that gets us here will have restored a usable GDT, although * it will be pointing to the wrong alias. */ -static void notrace __restore_processor_state(struct saved_context *ctxt) +static void noinline notrace __restore_processor_state(struct saved_context *ctxt) { struct cpuinfo_x86 *c;