Page MenuHomePhabricator

[Inline] prevent inlining on stack protector mismatch
ClosedPublic

Authored by nickdesaulniers on Nov 19 2020, 12:34 PM.

Details

Summary

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

SSP attributes can be ordered by strength. Weakest to strongest, they
are: ssp, sspstrong, sspreq. Callees with differing SSP attributes may be
inlined into each other, and the strongest attribute will be applied to the
caller. (No change)

After this change:

  • A callee with no SSP attributes will no longer be inlined into a caller with SSP attributes.
  • The reverse is also true: a callee with an SSP attribute will not be inlined into a caller with no SSP attributes.
  • The alwaysinline attribute overrides these rules.

Functions that get synthesized by the compiler may not get inlined as a
result if they are not created with the same stack protector function
attribute as their callers.

Alternative approach to https://reviews.llvm.org/D87956.

Fixes ps/47479.

Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Nov 19 2020, 12:34 PM
void added a comment.Nov 20 2020, 12:17 AM

Are there tests to ensure that the appropriate stack protector level is added to the function after inlining?

nickdesaulniers added a comment.EditedNov 20 2020, 12:59 PM

Are there tests to ensure that the appropriate stack protector level is added to the function after inlining?

This patch doesn't change the behavior when inlining occurs. (adjustCallerSSPLevel is what's called post successful inline, which adds the appropriate stack protection level) This patch prevents inlining when the caller and callee do not match (when one does not have ssp, sspstrong, or sspreq fn attr), such that adjustCallerSSPLevel is not run on the caller.

So adjustCallerSSPLevel should already have existing code coverage for this behavior? Does it? Digging through git history, it looks like you wrote adjustCallerSSPLevel in d154e283f28525a5fcfb063d4b527bb9a68a8608 in 2013. It looks like you wrote llvm/test/Transforms/Inline/inline_ssp.ll which tests this behavior, which I've modified here. So my changes to llvm/test/Transforms/Inline/inline_ssp.ll are probably what you're looking for. Those should be carefully reviewed; basically, callers no longer get upgraded from "no ssp fn attrs" to their callees ssp fn attrs because inlining was blocked/prevented.

EDIT: last sentence should have read: callers no longer get upgraded from "no ssp fn attrs" to their callees ssp fn attrs because inlining was blocked/prevented.

rnk added a comment.Nov 24 2020, 4:38 PM

I'm not super familiar with the SSP stuff, so let me try to restate how this is supposed to work to see if I understand it.

  • SSP attributes can be ordered by strength. Weakest to strongest, they are: ssp, sspstrong, sspreq.
  • After this change, a callee that lacks any SSP attribute can no longer be inlined into a caller that has any SSP attribute.
    • The alwaysinline attribute overrides this rule, though.
  • Callees with differing SSP attributes may be inlined into each other, and the strongest attribute will be applied to the caller. (No change)

Is that more or less right?

llvm/test/ThinLTO/X86/nossp.ll
3

I have recently discovered the new split-file utility, and I like it more than adding files to the Inputs directory. Think it's worth giving it a try here? Up to you. It was added mainly for linker test cases, where you want to test linking multiple inputs together.

In D91816#2415127, @rnk wrote:

I'm not super familiar with the SSP stuff, so let me try to restate how this is supposed to work to see if I understand it.

  • SSP attributes can be ordered by strength. Weakest to strongest, they are: ssp, sspstrong, sspreq.
  • After this change, a callee that lacks any SSP attribute can no longer be inlined into a caller that has any SSP attribute.
    • The alwaysinline attribute overrides this rule, though.
  • Callees with differing SSP attributes may be inlined into each other, and the strongest attribute will be applied to the caller. (No change)

Is that more or less right?

Yes, that's precise. I can include that in the commit message if you wouldn't mind and if it helps makes think closer?

llvm/test/ThinLTO/X86/nossp.ll
3

@MaskRay suggested that in a previous version of this patch as well. I will look into it. Is the idea that I would concat llvm/test/ThinLTO/X86/Inputs/nossp.ll to this, then use split-file to split it off?

rnk accepted this revision.Nov 25 2020, 10:56 AM

lgtm

In D91816#2415127, @rnk wrote:

I'm not super familiar with the SSP stuff, so let me try to restate how this is supposed to work to see if I understand it.

  • SSP attributes can be ordered by strength. Weakest to strongest, they are: ssp, sspstrong, sspreq.
  • After this change, a callee that lacks any SSP attribute can no longer be inlined into a caller that has any SSP attribute.
    • The alwaysinline attribute overrides this rule, though.
  • Callees with differing SSP attributes may be inlined into each other, and the strongest attribute will be applied to the caller. (No change)

Is that more or less right?

Yes, that's precise. I can include that in the commit message if you wouldn't mind and if it helps makes think closer?

Sure, go for it. After reading it again, I think you also disallow inlining when the callee has SSP attrs, and the caller does not. IIUC, that is the problematic case for the Linux kernel. Here's how I'd try to work that into the wording:

  • SSP attributes can be ordered by strength. Weakest to strongest, they are: ssp, sspstrong, sspreq.
  • After this change, a callee with no SSP attributes will no longer be inlined into a caller with SSP attributes.
  • The reverse is also true: a callee with an SSP attribute will not be inlined into a caller with no SSP attributes.
  • The alwaysinline attribute overrides these rules.
  • Callees with differing SSP attributes may be inlined into each other, and the strongest attribute will be applied to the caller. (No change)
This revision is now accepted and ready to land.Nov 25 2020, 10:56 AM
MaskRay added inline comments.Nov 25 2020, 2:09 PM
llvm/lib/Analysis/InlineCost.cpp
2435

Is the block unrelated changes?

MaskRay added inline comments.Nov 25 2020, 2:39 PM
llvm/lib/IR/Attributes.cpp
1942

Hmm. Perhaps if (!Callee.hasFnAttribute(Attribute::AlwaysInline)) { assert(); assert(); } can simplify the conditions a bit.

llvm/test/CodeGen/X86/stack-protector-2.ll
165

Does the comment apply to bar_sspstrong? I don't get the meaning...

llvm/test/ThinLTO/X86/nossp.ll
3

Right. https://llvm.org/docs/TestingGuide.html#extra-files Inlining the extra file makes it slightly more readable.

llvm/lib/IR/Attributes.cpp
1942

Oh, sure, the two ifs I added share that condition. Should I wrap that in #ifndef NDEBUG?

MaskRay added inline comments.Nov 30 2020, 1:56 PM
llvm/lib/IR/Attributes.cpp
1942

LG

nickdesaulniers marked 5 inline comments as done.
nickdesaulniers edited the summary of this revision. (Show Details)Nov 30 2020, 2:46 PM
nickdesaulniers marked an inline comment as done.

Cool, that should be all of the feedback addressed. How does it look?

llvm/lib/Analysis/InlineCost.cpp
2435
llvm/test/CodeGen/X86/stack-protector-2.ll
165

reworded, WDYT?

MaskRay accepted this revision.Dec 2 2020, 9:47 AM