[PATCH] D139254: Enhance stack protector

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a subscriber: t.p.northover.
smeenai added a comment.

In D139254#4231890 <https://reviews.llvm.org/D139254#4231890>, @nickdesaulniers wrote:

> In D139254#4191358 <https://reviews.llvm.org/D139254#4191358>, @xiangzhangllvm wrote:
>
>> In D139254#4190508 <https://reviews.llvm.org/D139254#4190508>, @hiraditya wrote:
>>
>>> 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?
>>
>> 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.
>> Welcome any good suggestion, or directly add me to related patch's reviewer or subsciber.
>
> I'm happy to see at least there's an option to turn this off.
> `-mllvm -disable-check-noreturn-call`
> https://reviews.llvm.org/D141556
>
> 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?

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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139254/new/

https://reviews.llvm.org/D139254