Page MenuHomePhabricator

Don't override __attribute__((no_stack_protector)) by inlining
ClosedPublic

Authored by hans on Jan 4 2022, 3:12 AM.

Details

Summary

Since 26c6a3e736d3, LLVM's inliner will "upgrade" the caller's stack protector attribute based on the callee. This leads to surprising results with Clang's no_stack_protector attribute added in 4fbf84c1732f (D46300). Consider the following code compiled with clang -fstack-protector-strong -Os (https://godbolt.org/z/7s3rW7a1q).

extern void h(int* p);

inline __attribute__((always_inline)) int g() {
  return 0;
} 

int __attribute__((__no_stack_protector__)) f() {
  int a[1];
  h(a);
  return g();
}

LLVM will inline g() into f(), and f() will get a stack protector, against the users explicit wishes, potentially breaking the program e.g. if h() changes the value of the stack cookie. That's a miscompile.

More recently, bc044a88ee3c (D91816) addressed this problem by preventing inlining when the stack protector is disabled in the caller and enabled in the callee or vice versa. However, the problem remains if the callee is marked always_inline as in the example above. This is affecting users, see e.g. http://crbug.com/1274129 and http://llvm.org/pr52886.

One way to fix this would be to prevent inlining also in the always_inline case. Despite the name, always_inline does not guarantee inlining, so this would be legal but potentially surprising to users.

However, I think the better fix is to not enable the stack protector in a caller based on the callee. The motivation for the old behaviour is unclear, it seems counter-intuitive, and causes real problems as we've seen.

This patch implements that fix, which means in the example above, g() gets inlined into f() (also without always_inline), and f() is emitted without stack protector. I think that matches most developers' expectations, and that's also what GCC does.

Another effect of this change is that a no_stack_protector function can now be inlined into a stack protected function, e.g. (https://godbolt.org/z/hafP6W856)

extern void h(int* p);

inline int __attribute__((__no_stack_protector__)) __attribute__((always_inline)) g() {
  return 0;
}

int f() {
  int a[1];
  h(a);
  return g();
}

I think that's fine. Such code would be unusual since no_stack_protector is normally applied to a program entry point which sets up the stack canary. And even if such code exists, inlining doesn't change the semantics: there is still no stack cookie setup/check around entry/exit of the g() code region, but there may be in the surrounding context, as there was before inlining. This also matches GCC.

See also the discussion at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722

Please let me know what you think.

Diff Detail

Event Timeline

hans created this revision.Jan 4 2022, 3:12 AM
hans requested review of this revision.Jan 4 2022, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2022, 3:12 AM

I am fine with this change but Nick should review.

Please add a link to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722 in the commit description.

llvm/test/ThinLTO/X86/nossp.ll
26–27

is local_unnamed_addr necessary for these tests?

35–36

Please put the attribute inline; it's much more readable for this single attribute.

llvm/test/Transforms/Inline/inline_ssp.ll
164

nounwind uwtable adds a lot of noise to this test. I wouldn't mind if you pre-committed a change removing them. ;)

Is there a new test case that demonstrates behavior of an always_inline callee?

hans updated this revision to Diff 397341.Jan 4 2022, 10:57 AM
hans marked 2 inline comments as done.
hans edited the summary of this revision. (Show Details)

Addressing comments.

hans added a comment.Jan 4 2022, 10:57 AM

Is there a new test case that demonstrates behavior of an always_inline callee?

Added one to llvm/test/Transforms/Inline/inline_ssp.ll now.

llvm/test/ThinLTO/X86/nossp.ll
26–27

No, I just wanted to check the whole line up until the { to make sure there's no ssp attribute.
I'll do it in a different way.

35–36

llvm-dis doesn't print it inline though. I could use a regex and give it a symbolic name, but I'm not sure it's worth it here?

llvm/test/Transforms/Inline/inline_ssp.ll
164

I'll update the patch to do this for now. If you prefer to have it pre-submitted I can do that before landing.

MaskRay accepted this revision.EditedJan 4 2022, 2:17 PM

The refined behavior (__attribute__((no_stack_protector)) caller with an always_inline callee) looks good to me. I think GCC folks focused on the optimization part (whether inlining is suppressed) but ssp conveys some security hardening semantics (inlining can lose protection) so is unusual.

llvm/docs/LangRef.rst
1998

I think the comments are still useful. It is less about semantic requirement, but about the behavior which users may want to know.

Upgrading ssp/sspstrong/sspreq level is like an implementation compromise in the absence of instruction-level (fine-grained) ssp as we only have function attributes.

llvm/test/ThinLTO/X86/nossp.ll
26–27

CHECK-LABEL is useful. The diagnostic is better if the CHECK-NOT fails.

This revision is now accepted and ready to land.Jan 4 2022, 2:17 PM
nickdesaulniers accepted this revision.Jan 4 2022, 2:58 PM
nickdesaulniers added inline comments.
llvm/test/Transforms/Inline/inline_ssp.ll
2

I think there's a pass called always-inline, too. Should we add that as a run line?

51

converting all of these attribute checks to regexes can also probably be pre-committed.

hans updated this revision to Diff 397581.Jan 5 2022, 7:34 AM
hans marked 2 inline comments as done.

Putting the CHECK-LABELs back.

hans added a comment.Jan 5 2022, 7:40 AM

The refined behavior (__attribute__((no_stack_protector)) caller with an always_inline callee) looks good to me

Note that the patch doesn't differentiate between always_inline and regular inline.

llvm/docs/LangRef.rst
1998

But the old comment is no longer true: an ssp function *can* now be inlined into a non-ssp function, just like any other function, and alwaysinline doesn't make any difference.

Or am I missing something?

llvm/test/Transforms/Inline/inline_ssp.ll
2

I don't think we'd get any new coverage from that really. The interesting thing to test is the inline case. It's good to have a test case with autoinline too, to make sure they don't diverge in the future, but the inline pass will handle that too.

2

s/autoinline/alwaysinline/ obviously :-]

51

Okay, will do.

hans added a comment.Jan 10 2022, 8:16 AM

Ping? Especially the question about the LangRef change.

MaskRay accepted this revision.Jan 12 2022, 10:49 AM
This revision was landed with ongoing or failed builds.Jan 13 2022, 3:05 AM
This revision was automatically updated to reflect the committed changes.