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

[hans]

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.

[manojgupta]

I am fine with this change but Nick should review.

[nickdesaulniers]

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

is local_unnamed_addr necessary for these tests?

34

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

llvm/test/Transforms/Inline/inline_ssp.ll

150

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

[nickdesaulniers]

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

[hans]

Addressing comments.

[hans]

In D116589#3220044, @nickdesaulniers wrote:

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

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.

34

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

150

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]

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

1994

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

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

[nickdesaulniers]

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?

44

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

[hans]

Putting the CHECK-LABELs back.

[hans]

In D116589#3220829, @MaskRay wrote:

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

1994

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 :-]

44

Okay, will do.

[hans]

Ping? Especially the question about the LangRef change.

llvm/docs/LangRef.rst

``ssp``
     - Character arrays larger than ``ssp-buffer-size`` (default 8).
     - Aggregates containing character arrays larger than ``ssp-buffer-size``.
     - Calls to alloca() with variable sizes or constant sizes greater than
       ``ssp-buffer-size``.
 
     Variables that are identified as requiring a protector will be arranged
     on the stack such that they are adjacent to the stack protector guard.
 
-    A function with the ``ssp`` attribute but without the ``alwaysinline``
-    attribute cannot be inlined into a function without a
-    ``ssp/sspreq/sspstrong`` attribute. If inlined, the caller will get the
-    ``ssp`` attribute. ``call``, ``invoke``, and ``callbr`` instructions with
-    the ``alwaysinline`` attribute force inlining.
+    If a function with an ``ssp`` attribute is inlined into a calling function,
+    the attribute is not carried over to the calling function.
+
 ``sspstrong``
     This attribute indicates that the function should emit a stack smashing
     protector. This attribute causes a strong heuristic to be used when
     determining if a function needs stack protectors. The strong heuristic
     will enable protectors for functions with:
 
     - Arrays of any size and type
     - Aggregates containing an array of any size and type.
     - Calls to alloca().
     - Local variables that have had their address taken.
 
     Variables that are identified as requiring a protector will be arranged
     on the stack such that they are adjacent to the stack protector guard.
     The specific layout rules are:
 
     #. Large arrays and structures containing large arrays
        (``>= ssp-buffer-size``) are closest to the stack protector.
     #. Small arrays and structures containing small arrays
        (``< ssp-buffer-size``) are 2nd closest to the protector.
     #. Variables that have had their address taken are 3rd closest to the
        protector.
 
     This overrides the ``ssp`` function attribute.
 
-    A function with the ``sspstrong`` attribute but without the
-    ``alwaysinline`` attribute cannot be inlined into a function without a
-    ``ssp/sspstrong/sspreq`` attribute. If inlined, the caller will get the
-    ``sspstrong`` attribute unless the ``sspreq`` attribute exists.  ``call``,
-    ``invoke``, and ``callbr`` instructions with the ``alwaysinline`` attribute
-    force inlining.
+    If a function with an ``sspstrong`` attribute is inlined into a calling
+    function which has an ``ssp`` attribute, the calling function's attribute
+    will be upgraded to ``sspstrong``.
+
 ``sspreq``
     This attribute indicates that the function should *always* emit a stack
     smashing protector. This overrides the ``ssp`` and ``sspstrong`` function
     attributes.
 
     Variables that are identified as requiring a protector will be arranged
     on the stack such that they are adjacent to the stack protector guard.
     The specific layout rules are:
 
     #. Large arrays and structures containing large arrays
        (``>= ssp-buffer-size``) are closest to the stack protector.
     #. Small arrays and structures containing small arrays
        (``< ssp-buffer-size``) are 2nd closest to the protector.
     #. Variables that have had their address taken are 3rd closest to the
        protector.
 
-    A function with the ``sspreq`` attribute but without the ``alwaysinline``
-    attribute cannot be inlined into a function without a
-    ``ssp/sspstrong/sspreq`` attribute. If inlined, the caller will get the
-    ``sspreq`` attribute.  ``call``, ``invoke``, and ``callbr`` instructions
-    with the ``alwaysinline`` attribute force inlining.
+    If a function with an ``sspreq`` attribute is inlined into a calling
+    function which has an ``ssp`` or ``sspstrong`` attribute, the calling
+    function's attribute will be upgraded to ``sspreq``.
 
 ``strictfp``
     This attribute indicates that the function was called from a scope that
     requires strict floating-point semantics.  LLVM will not attempt any
     optimizations that require assumptions about the floating-point rounding
     mode or that might alter the state of floating-point status flags that
     might otherwise be set or cleared by calling this function. LLVM will
     not introduce any new floating-point instructions that may trap.

llvm/lib/Analysis/InlineCost.cpp

Optional<InlineResult> llvm::getAttributeBasedInliningDecision(
   // Don't inline functions marked noinline.
   if (Callee->hasFnAttribute(Attribute::NoInline))
     return InlineResult::failure("noinline function attribute");
 
   // Don't inline call sites marked noinline.
   if (Call.isNoInline())
     return InlineResult::failure("noinline call site attribute");
 
-  // Don't inline functions if one does not have any stack protector attribute
-  // but the other does.
-  if (Caller->hasStackProtectorFnAttr() && !Callee->hasStackProtectorFnAttr())
-    return InlineResult::failure(
-        "stack protected caller but callee requested no stack protector");
-  if (Callee->hasStackProtectorFnAttr() && !Caller->hasStackProtectorFnAttr())
-    return InlineResult::failure(
-        "stack protected callee but caller requested no stack protector");
-
   return None;
 }
 
 InlineCost llvm::getInlineCost(
     CallBase &Call, Function *Callee, const InlineParams &Params,
     TargetTransformInfo &CalleeTTI,
     function_ref<AssumptionCache &(Function &)> GetAssumptionCache,
     function_ref<const TargetLibraryInfo &(Function &)> GetTLI,

llvm/lib/IR/Attributes.cpp

static void setOR(Function &Caller, const Function &Callee) {
   if (!AttrClass::isSet(Caller, AttrClass::getKind()) &&
       AttrClass::isSet(Callee, AttrClass::getKind()))
     AttrClass::set(Caller, AttrClass::getKind(), true);
 }
 
 /// If the inlined function had a higher stack protection level than the
 /// calling function, then bump up the caller's stack protection level.
 static void adjustCallerSSPLevel(Function &Caller, const Function &Callee) {
+  // If the calling function has *no* stack protection level (e.g. it was built
+  // with Clang's -fno-stack-protector or no_stack_protector attribute), don't
+  // change it as that could change the program's semantics.
+  if (!Caller.hasStackProtectorFnAttr())
+    return;
+
   // If upgrading the SSP attribute, clear out the old SSP Attributes first.
   // Having multiple SSP attributes doesn't actually hurt, but it adds useless
   // clutter to the IR.
   AttrBuilder OldSSPAttr;
   OldSSPAttr.addAttribute(Attribute::StackProtect)
       .addAttribute(Attribute::StackProtectStrong)
       .addAttribute(Attribute::StackProtectReq);
 

llvm/test/ThinLTO/X86/nossp.ll

 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 declare void @nossp_callee()
 declare void @ssp_callee() ssp
 
 ; nossp caller should be able to inline nossp callee.
 define void @nossp_caller() {
-; CHECK-LABEL: @nossp_caller
+; CHECK: define void @nossp_caller() local_unnamed_addr {
 ; CHECK-NEXT: tail call void @foo
   tail call void @nossp_callee()
   ret void
 }
 
 ; ssp caller should be able to inline ssp callee.
 define void @ssp_caller() ssp {
-; CHECK-LABEL: @ssp_caller
+; CHECK: define void @ssp_caller() local_unnamed_addr #0 {
 ; CHECK-NEXT: tail call void @foo
   tail call void @ssp_callee()
   ret void
 }
 
-; nossp caller should *NOT* be able to inline ssp callee.
+; nossp caller should be able to inline ssp callee.
+; the ssp attribute is not propagated.
 define void @nossp_caller2() {
-; CHECK-LABEL: @nossp_caller2
-; CHECK-NEXT: tail call void @ssp_callee
+; CHECK: define void @nossp_caller2() local_unnamed_addr {
+; CHECK-NEXT: tail call void @foo
   tail call void @ssp_callee()
   ret void
 }
 
-; ssp caller should *NOT* be able to inline nossp callee.
+; ssp caller should be able to inline nossp callee.
 define void @ssp_caller2() ssp {
-; CHECK-LABEL: @ssp_caller2
-; CHECK-NEXT: tail call void @nossp_callee
+; CHECK: define void @ssp_caller2() local_unnamed_addr #0 {
+; CHECK-NEXT: tail call void @foo
   tail call void @nossp_callee()
   ret void
 }
 
+; CHECK: attributes #0 = { ssp }
+
 ;--- b.ll
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 declare void @foo()
 
 define void @nossp_callee() {
   call void @foo()
   ret void
 }
 
 define void @ssp_callee() ssp {
   call void @foo()
   ret void
 }

llvm/test/Transforms/Inline/inline_nossp.ll

-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -passes='cgscc(inline)' %s -S -pass-remarks-missed=inline 2>&1 | FileCheck --check-prefixes=CHECK,CHECK-INLINE %s
-; RUN: opt -passes=always-inline -o - -S %s | FileCheck %s
-
-; CHECK-INLINE: 'ssp' not inlined into 'nossp_caller' because it should never be inlined (cost=never): stack protected callee but caller requested no stack protector
-; CHECK-INLINE: 'nossp' not inlined into 'ssp_caller' because it should never be inlined (cost=never): stack protected caller but callee requested no stack protector
-
-; Not interesting to test.
-define i32 @nossp() { ret i32 41 }
-define i32 @ssp() sspstrong { ret i32 42 }
-define i32 @nossp_alwaysinline() alwaysinline { ret i32 43 }
-define i32 @ssp_alwaysinline() sspstrong alwaysinline { ret i32 44 }
-
-; @ssp should not be inlined due to mismatch stack protector.
-; @ssp_alwaysinline should be inlined due to alwaysinline.
-define i32 @nossp_caller() {
-; CHECK-LABEL: @nossp_caller(
-; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @ssp()
-; CHECK-NEXT:    ret i32 44
-;
-  call i32 @ssp()
-  %2 = call i32 @ssp_alwaysinline()
-  ret i32 %2
-}
-; @nossp should not be inlined due to mismatch stack protector.
-; @nossp_alwaysinline should be inlined due to alwaysinline.
-define i32 @ssp_caller() sspstrong {
-; CHECK-LABEL: @ssp_caller(
-; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @nossp()
-; CHECK-NEXT:    ret i32 43
-;
-  call i32 @nossp()
-  %2 = call i32 @nossp_alwaysinline()
-  ret i32 %2
-}
-
-; The alwaysinline attribute can also appear on the CallBase (ie. the call
-; site), ie. when __attribute__((flatten)) is used on the caller. Treat this
-; the same as if the caller had the fn attr alwaysinline and permit inline
-; substitution, despite the mismatch between caller and callee on ssp attrs.
-;
-; Curiously, the always_inline attribute on a CallInst is only expanded by the
-; inline pass, but not always_inline pass!
-define i32 @nossp_alwaysinline_caller() {
-; CHECK-INLINE-LABEL: @nossp_alwaysinline_caller(
-; CHECK-INLINE-NEXT:    ret i32 42
-;
-  %1 = call i32 @ssp() alwaysinline
-  ret i32 %1
-}

llvm/test/Transforms/Inline/inline_ssp.ll

 ; RUN: opt -inline %s -S | FileCheck %s
 ; RUN: opt -passes='cgscc(inline)' %s -S | FileCheck %s
 ; Ensure SSP attributes are propagated correctly when inlining.
 
 @.str = private unnamed_addr constant [11 x i8] c"fun_nossp\0A\00", align 1
 @.str1 = private unnamed_addr constant [9 x i8] c"fun_ssp\0A\00", align 1
 @.str2 = private unnamed_addr constant [15 x i8] c"fun_sspstrong\0A\00", align 1
 @.str3 = private unnamed_addr constant [12 x i8] c"fun_sspreq\0A\00", align 1
 
 ; These first four functions (@fun_sspreq, @fun_sspstrong, @fun_ssp, @fun_nossp)
 ; are used by the remaining functions to ensure that the SSP attributes are
-; propagated correctly.  The caller should have its SSP attribute set as:
+; propagated correctly.  If the caller had an SSP attribute before inlining, it
+; should have its new SSP attribute set as:
 ; strictest(caller-ssp-attr, callee-ssp-attr), where strictness is ordered as:
-;  sspreq > sspstrong > ssp > [no ssp]
+;  sspreq > sspstrong > ssp
 define internal void @fun_sspreq() nounwind sspreq uwtable {
 entry:
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([12 x i8], [12 x i8]* @.str3, i32 0, i32 0))
   ret void
 }
 
 define internal void @fun_sspstrong() nounwind sspstrong uwtable {
 entry:
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([15 x i8], [15 x i8]* @.str2, i32 0, i32 0))
   ret void
 }
 
 define internal void @fun_ssp() nounwind ssp uwtable {
 entry:
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @.str1, i32 0, i32 0))
   ret void
 }
 
 define internal void @fun_nossp() nounwind uwtable {
 entry:
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([11 x i8], [11 x i8]* @.str, i32 0, i32 0))
   ret void
 }
 
-; Tests start below 
+; Tests start below.
 
 define void @inline_req_req() nounwind sspreq uwtable {
 entry:
-; CHECK: @inline_req_req() #0
+; CHECK: @inline_req_req() #[[SSPREQ:[0-9]]]
   call void @fun_sspreq()
   ret void
 }
 
 define void @inline_req_strong() nounwind sspstrong uwtable {
 entry:
-; CHECK: @inline_req_strong() #0
+; CHECK: @inline_req_strong() #[[SSPREQ]]
   call void @fun_sspreq()
   ret void
 }
 
 define void @inline_req_ssp() nounwind ssp uwtable {
 entry:
-; CHECK: @inline_req_ssp() #0
+; CHECK: @inline_req_ssp() #[[SSPREQ]]
   call void @fun_sspreq()
   ret void
 }
 
 define void @inline_req_nossp() nounwind uwtable {
 entry:
-; CHECK: @inline_req_nossp() #3
+; CHECK: @inline_req_nossp() #[[NOSSP:[0-9]]]
   call void @fun_sspreq()
   ret void
 }
 
 define void @inline_strong_req() nounwind sspreq uwtable {
 entry:
-; CHECK: @inline_strong_req() #0
+; CHECK: @inline_strong_req() #[[SSPREQ]]
   call void @fun_sspstrong()
   ret void
 }
 
 
 define void @inline_strong_strong() nounwind sspstrong uwtable {
 entry:
-; CHECK: @inline_strong_strong() #1
+; CHECK: @inline_strong_strong() #[[SSPSTRONG:[0-9]]]
   call void @fun_sspstrong()
   ret void
 }
 
 define void @inline_strong_ssp() nounwind ssp uwtable {
 entry:
-; CHECK: @inline_strong_ssp() #1
+; CHECK: @inline_strong_ssp() #[[SSPSTRONG]]
   call void @fun_sspstrong()
   ret void
 }
 
 define void @inline_strong_nossp() nounwind uwtable {
 entry:
-; CHECK: @inline_strong_nossp() #3
+; CHECK: @inline_strong_nossp() #[[NOSSP]]
   call void @fun_sspstrong()
   ret void
 }
 
 define void @inline_ssp_req() nounwind sspreq uwtable {
 entry:
-; CHECK: @inline_ssp_req() #0
+; CHECK: @inline_ssp_req() #[[SSPREQ]]
   call void @fun_ssp()
   ret void
 }
 
 
 define void @inline_ssp_strong() nounwind sspstrong uwtable {
 entry:
-; CHECK: @inline_ssp_strong() #1
+; CHECK: @inline_ssp_strong() #[[SSPSTRONG]]
   call void @fun_ssp()
   ret void
 }
 
 define void @inline_ssp_ssp() nounwind ssp uwtable {
 entry:
-; CHECK: @inline_ssp_ssp() #2
+; CHECK: @inline_ssp_ssp() #[[SSP:[0-9]]]
   call void @fun_ssp()
   ret void
 }
 
 define void @inline_ssp_nossp() nounwind uwtable {
 entry:
-; CHECK: @inline_ssp_nossp() #3
+; CHECK: @inline_ssp_nossp() #[[NOSSP]]
   call void @fun_ssp()
   ret void
 }
 
 define void @inline_nossp_req() nounwind uwtable sspreq {
 entry:
-; CHECK: @inline_nossp_req() #0
+; CHECK: @inline_nossp_req() #[[SSPREQ]]
   call void @fun_nossp()
   ret void
 }
 
 
 define void @inline_nossp_strong() nounwind sspstrong uwtable {
 entry:
-; CHECK: @inline_nossp_strong() #1
+; CHECK: @inline_nossp_strong() #[[SSPSTRONG]]
   call void @fun_nossp()
   ret void
 }
 
 define void @inline_nossp_ssp() nounwind ssp uwtable {
 entry:
-; CHECK: @inline_nossp_ssp() #2
+; CHECK: @inline_nossp_ssp() #[[SSP]]
   call void @fun_nossp()
   ret void
 }
 
 define void @inline_nossp_nossp() nounwind uwtable {
 entry:
-; CHECK: @inline_nossp_nossp() #3
+; CHECK: @inline_nossp_nossp() #[[NOSSP]]
   call void @fun_nossp()
   ret void
 }
 
 declare i32 @printf(i8*, ...)
 
-; CHECK: attributes #0 = { nounwind sspreq uwtable }
-; CHECK: attributes #1 = { nounwind sspstrong uwtable }
-; CHECK: attributes #2 = { nounwind ssp uwtable }
-; CHECK: attributes #3 = { nounwind uwtable }
+; CHECK: attributes #[[SSPREQ]] = { nounwind sspreq uwtable }
+; CHECK: attributes #[[NOSSP]] = { nounwind uwtable }
+; CHECK: attributes #[[SSPSTRONG]] = { nounwind sspstrong uwtable }
+; CHECK: attributes #[[SSP]] = { nounwind ssp uwtable }