This is an archive of the discontinued LLVM Phabricator instance.

[IR] add fn attr for no_stack_protector; prevent inlining on mismatch
ClosedPublic

Authored by nickdesaulniers on Sep 18 2020, 5:07 PM.

Details

Summary

It's currently ambiguous in IR whether the source language explicitly
did not want a stack a stack protector (in C, via function attribute
no_stack_protector) or doesn't care for any given function.

It's common for code that manipulates the stack via inline assembly or
that has to set up its own stack canary (such as the Linux kernel) would
like to avoid stack protectors in certain functions. In this case, we've
been bitten by numerous bugs where a callee with a stack protector is
inlined into an attribute((no_stack_protector)) caller, which
generally breaks the caller's assumptions about not having a stack
protector. LTO exacerbates the issue.

While developers can avoid this by putting all no_stack_protector
functions in one translation unit together and compiling those with
-fno-stack-protector, it's generally not very ergonomic or as
ergonomic as a function attribute, and still doesn't work for LTO. See also:
https://lore.kernel.org/linux-pm/20200915172658.1432732-1-rkir@google.com/
https://lore.kernel.org/lkml/20200918201436.2932360-30-samitolvanen@google.com/T/#u

Typically, when inlining a callee into a caller, the caller will be
upgraded in its level of stack protection (see adjustCallerSSPLevel()).
By adding an explicit attribute in the IR when the function attribute is
used in the source language, we can now identify such cases and prevent
inlining. Block inlining when the callee and caller differ in the case that one
contains nossp when the other has ssp, sspstrong, or sspreq.

Fixes pr/47479.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
nickdesaulniers requested review of this revision.Sep 18 2020, 5:07 PM
nickdesaulniers planned changes to this revision.Sep 18 2020, 5:09 PM

Not ready for review yet, posting a WIP fix for https://bugs.llvm.org/show_bug.cgi?id=47479.

TODO next week:

  • tests, tests, and more tests.
  • interaction with __attribute__((always_inline)).
  • fix TODOs in code.
  • IR verifier that the attributes never have more than one ssp attr; they should be mutually exclusive.
nickdesaulniers edited the summary of this revision. (Show Details)Sep 18 2020, 5:10 PM
nickdesaulniers removed a reviewer: jdoerfert.
nickdesaulniers added a subscriber: jdoerfert.
nickdesaulniers edited the summary of this revision. (Show Details)Sep 18 2020, 5:18 PM
  • add tests, make fn attrs mutually exclusive

I obviously have a few todos left to resolve, but CC'ing reviewers now for general feedback.

One thing I'm unsure of; it felt curious to me that nothing was enforcing that these different levels of stack protection were mutually exclusive. I've added that here, though I could fork that out to a separate patch. I do think it would be better if this was a "key"="value" attribute, which would simplify ensuring these were mutually exclusive (I think). I guess that would run afoul that previous IR would no longer be forward compatible; does LLVM have a policy on IR "breaks" like that? I guess making the mutually exclusive technically is a subtle change in behavior...should I not do that? Or if I do, should I document that somewhere else in addition?

Does the patch itself seem to be in good shape; and does the problem it solves make sense?

nickdesaulniers planned changes to this revision.Sep 22 2020, 1:40 PM
clang/test/CodeGen/stack-protector.c
39

should be attributes plural.

llvm/include/llvm/Bitcode/LLVMBitCodes.h
610

any comments from reviewers before I go and do a tedious reordering of these enum values?

llvm/lib/IR/Attributes.cpp
1901–1902

This should be an anomalous situation due to the added verifier check. Should I make it an assert instead?

llvm/lib/Transforms/Utils/InlineFunction.cpp
1687

Is there a better way to emit an OptimizationRemark? Having this feedback in -Rpass=inline might be nice.

bumping for review

void added inline comments.Sep 30 2020, 3:17 AM
llvm/lib/Transforms/Utils/InlineFunction.cpp
1679

s/the/that/

1682

Should we check whether the function with nossp needs an ssp before issuing the error?

nickdesaulniers marked 2 inline comments as done.
  • address todos and review feedback
llvm/include/llvm/Bitcode/LLVMBitCodes.h
610

Oof, it looks like inserting ATTR_KIND_NO_STACK_PROTECT = 26 then reordering the rest causes the following tests to fail:

Failed Tests (17):
  LLVM :: Bitcode/DIExpression-aggresult.ll
  LLVM :: Bitcode/DITemplateParameter-5.0.ll
  LLVM :: Bitcode/PR23310.test
  LLVM :: Bitcode/apple-clang-700-compat.test
  LLVM :: Bitcode/attributes-3.3.ll
  LLVM :: Bitcode/case-ranges-3.3.ll
  LLVM :: Bitcode/compatibility-3.6.ll
  LLVM :: Bitcode/compatibility-3.7.ll
  LLVM :: Bitcode/compatibility-3.8.ll
  LLVM :: Bitcode/compatibility-3.9.ll
  LLVM :: Bitcode/compatibility-4.0.ll
  LLVM :: Bitcode/compatibility-5.0.ll
  LLVM :: Bitcode/compatibility-6.0.ll
  LLVM :: Bitcode/drop-debug-info.3.5.ll
  LLVM :: Bitcode/invalid-functionptr-align.ll
  LLVM :: Object/nm-bitcode.test
  LLVM :: tools/llvm-nm/X86/IRobj.test

under the assumption that new fn attr's should get monotonically increasing numbers, I'll stick this new one at the end. That should preserve bitcode compatibility, at the cost of the similar function routines not being grouped in successive order (which is a stylistic or subjective complaint).

llvm/lib/IR/Attributes.cpp
1901–1902

I should update this comment to say they're not mutually exclusive.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1682

No; it should never be the case that "the function with nossp needs an ssp" since nossp and ssp are now mutually exclusive, nossp means "I know what I'm doing, and accept the consequences," and StackProtector::RequiresStackProtector will always return false in that case after my changes (even if it was a static method we could easily access, and not a private method of a separate pass).

  • add -passes='cgscc(inline)' to inline_nossp.ll
llvm/docs/BitCodeFormat.rst
953 ↗(On Diff #297969)

ah, I should remove this; this block is legacy encodings which I do not modify.

llvm/lib/IR/Verifier.cpp
1972–1979

I'd love to cast these booleans to unsigned and add them, but I find that all of the static_casts don't really make this more concise. Thoughts?

  • remove modification to docs about legacy fn attr encoding
nickdesaulniers retitled this revision from [WIP][IR] add fn attr for no_stack_protector; prevent inlining ssp into no-ssp to [IR] add fn attr for no_stack_protector; prevent inlining ssp into no-ssp.Oct 13 2020, 3:17 PM
nickdesaulniers retitled this revision from [IR] add fn attr for no_stack_protector; prevent inlining ssp into no-ssp to [IR] add fn attr for no_stack_protector; prevent inlining ssp into nossp.Oct 13 2020, 3:19 PM
nickdesaulniers edited the summary of this revision. (Show Details)

bumping for review

void added inline comments.Oct 20 2020, 8:12 PM
llvm/docs/LangRef.rst
1829 ↗(On Diff #297970)

Do we care about the opposite situation where a function requiring a stack protector doesn't inline a function with the nossp attribute?

llvm/lib/IR/Attributes.cpp
1901–1902

If it should never happen, then please make it an assert.

llvm/lib/IR/Verifier.cpp
1972–1979

I think what you have is fine.

1981

Could use stronger language here. :-)

"nssp, ssp, sspreq, and sspstrong fn attrs are mutually exclusive."

nickdesaulniers marked 3 inline comments as done.
  • rebase, use stronger language about mutual exclusion, prefer assert
nickdesaulniers planned changes to this revision.Oct 21 2020, 1:51 PM
nickdesaulniers added inline comments.
llvm/docs/LangRef.rst
1829 ↗(On Diff #297970)

oh, shoot, you're right. I should have thought of that case. I need to address that in this patch. Ok, let me add more tests/code/docs for that.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1687

See llvm::setInlineRemark for how these are set.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1687

InlineAdvisor::recordUnsuccessfulInliningImpl already handles this.

  • handle additional case of ssp caller w/ nossp callee, update docs, add -Rpass-missed=inline test
nickdesaulniers retitled this revision from [IR] add fn attr for no_stack_protector; prevent inlining ssp into nossp to [IR] add fn attr for no_stack_protector; prevent inlining on mismatch.Oct 21 2020, 3:13 PM
nickdesaulniers edited the summary of this revision. (Show Details)
void accepted this revision.Oct 21 2020, 3:44 PM

Only a couple of nits, but I think this looks good.

llvm/docs/BitCodeFormat.rst
1072 ↗(On Diff #299813)

I'm tempted to say that these two adds should be a separate commit, but it's a nit and I'll leave it up to you.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1691

Suggestion: Improve the failure message to be a bit more descriptive. I.e. mention that it can't inline a function with stack protection into a function without stack protection and vice versa.

This revision is now accepted and ready to land.Oct 21 2020, 3:44 PM
llvm/lib/IR/Attributes.cpp
1901–1902

Ah this assertion is not quite right; we still would like to permit nossp callee being inlined into nossp caller. Let me add a test case for that, then make the assertion more precise about a mismatch between caller and callee.

nickdesaulniers planned changes to this revision.Oct 22 2020, 1:54 PM

In phab here, it looks like my newly added clang/test/Frontend/optimization-remark-missed-inline-stack-protectors.c fails on windows because I redirect stdout to /dev/null. How does that work for other tests? I see other tests in that dir write to /dev/null. Oh, I guess -o /dev/null will just create a file called "/dev/null" on Windows? So I should do that rather than 1> /dev/null?

void added a comment.Oct 22 2020, 2:06 PM

In phab here, it looks like my newly added clang/test/Frontend/optimization-remark-missed-inline-stack-protectors.c fails on windows because I redirect stdout to /dev/null. How does that work for other tests? I see other tests in that dir write to /dev/null. Oh, I guess -o /dev/null will just create a file called "/dev/null" on Windows? So I should do that rather than 1> /dev/null?

Why not just use "expected-error" and the like?

nickdesaulniers marked 3 inline comments as done.
  • rebase, add alwaysinline tests, use expected-remark, improve remarks
This revision is now accepted and ready to land.Oct 22 2020, 4:43 PM

Why not just use "expected-error" and the like?

Ah, brilliant! Thank you! Will wait to submit until tomorrow morning.

llvm/docs/BitCodeFormat.rst
1072 ↗(On Diff #299813)

As a follow up, I probably want to make -fno-stack-protector sprinkle this new fn attr nossp all over everything; that and this should fix our LTO bug with no changes to the source, otherwise we can add __attribute__((no_stack_protector)) to the source (though GCC only landed support for that TODAY, so portability would be an issue there)..

whitequark resigned from this revision.Oct 22 2020, 9:47 PM
This revision was landed with ongoing or failed builds.Oct 23 2020, 11:56 AM
This revision was automatically updated to reflect the committed changes.

I missed adding to llvm/lib/Bitcode/Reader/BitcodeReader.cpp, so LTO is broken with this fn attribute...todo...fix

diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index f8069b93103f..a99d6baa8d9d 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -1537,6 +1537,8 @@ static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
     return Attribute::ByRef;
   case bitc::ATTR_KIND_MUSTPROGRESS:
     return Attribute::MustProgress;
+  case bitc::ATTR_KIND_NO_STACK_PROTECT:
+    return Attribute::NoStackProtect;
   }
 }
MaskRay added a subscriber: MaskRay.Nov 4 2020, 5:05 PM
MaskRay added inline comments.
clang/test/Frontend/optimization-remark-missed-inline-stack-protectors.c
1 ↗(On Diff #300365)

Emm. I am a bit unsure that we need the clang/test/Frontend test. The inlining decision logic is coded into Utils/InlineFunction.cpp and tested by Transforms/Inline/inline_nossp.ll

clang is at a higher layer and does not need to understand the inlining decision.

llvm/docs/LangRef.rst
1827 ↗(On Diff #300365)

Would this be confusing now that both nossp and ssp exist?

Is there an alternative design which can make this extensible? @jdoerfert

aeubanks added inline comments.
llvm/test/Transforms/Inline/inline_nossp.ll
3 ↗(On Diff #300365)

This test fails with the NPM,
opt -passes=always-inline ...

Does llvm::isInlineViable() need to be updated?

llvm/test/Transforms/Inline/inline_nossp.ll
3 ↗(On Diff #300365)

Thanks for the report. Is there a Cmake configuration I need to explicitly set to reproduce? LLVM_USE_NEWPM?

aeubanks added inline comments.Nov 16 2020, 4:32 PM
llvm/test/Transforms/Inline/inline_nossp.ll
3 ↗(On Diff #300365)

Just a new RUN line: RUN: opt -passes=always-inline -o - -S %s | FileCheck %s

llvm/test/Transforms/Inline/inline_nossp.ll
3 ↗(On Diff #300365)

I think I'm going to back out this patch and the fix ups, and pursue a much simpler approach: https://github.com/ClangBuiltLinux/llvm-project/commit/501847c5239be52bbe32a9c5bd0723e4d0ad2990.

llvm/test/Transforms/Inline/inline_nossp.ll
3 ↗(On Diff #300365)