[PATCH] D147975: [StackProtector] don't check stack protector before calling nounwind functions

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.

In D147975#4256991 <https://reviews.llvm.org/D147975#4256991>, @LuoYuanke wrote:

> It seems the noreturn call do miss the stack canary checking. Is it the expected behaviour in security point of view?

It's my opinion, and I acknowledge that folks can have a reasonable disagreement, but reading https://download.vusec.net/papers/chop_ndss23.pdf, I think `noreturn` is a red-herring.  The attack relies on `__cxa_throw` being `noreturn`, and no stack check being emitted before that.  Once your in a function, it's hard to know if the callers stack frame has been corrupted, so it's better to check the stack canary in the call before the caller.

I agree with this point: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58245#c6.  Why don't we check the stack canary before non-noreturn calls?  Why not before EVERY operation involving stack memory?  //performance//

Whether the called function is noreturn or not is kind of irrelevant. Consider the following case:

  void caller () {
    int arr [42] = {};
    int (*callee)() = get_callback();
    vuln();
    callee(arr);
  }

`callee` may make control flow decisions based on stack allocated variables in `caller`. Those values may have been corrupted. (A traditional JOP attack)  Does it matter if `callee` is `noreturn`? (I think not)  So I think CHOP is a cute name, but it's just a JOP attack that finds gadgets in the C++ exception handling runtime.

My change undoes the excessive canary checking to C code (and `noexcept` C++ functions, or C++ code built with `-fno-exceptions`), but `-fexceptions` C++ code will have a penalty larger than just before calls to `__cxa_throw`, such as calls to explicitly marked `noreturn` functions.  I think the penalty introduced in https://reviews.llvm.org/rGd656ae28095726830f9beb8dbd4d69f5144ef821 (and https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=a25982ada523689c8745d7fb4b1b93c8f5dab2e7) could be reduced by simply checking the stack canary before a call to `__cxa_throw` alone (and whatever the equivalent is on non-Itanium ABI systems) since the CHOP paper demonstrates how powerful the C++ exception handling gadgets may be.  I'm just trying to undo the unreasonable cost for C programs with this change.

---

Android undid https://reviews.llvm.org/rGd656ae28095726830f9beb8dbd4d69f5144ef821 here due to measured 2% RSS increase: https://android-review.git.corp.google.com/c/platform/build/soong/+/2524336.  I will add that note to the commit message.  Unless there's more feedback about this change, such as something I may be missing, I plan to commit this tomorrow and request a cherry-pick to the release/16.x branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147975