From 2e1beed27092ed92bd6ac7084279f76e69485cee Mon Sep 17 00:00:00 2001 From: Julian Lettner Date: Thu, 24 Jan 2019 01:06:19 +0000 Subject: [PATCH] [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls Summary: UBSan wants to detect when unreachable code is actually reached, so it adds instrumentation before every `unreachable` instruction. However, the optimizer will remove code after calls to functions marked with `noreturn`. To avoid this UBSan removes `noreturn` from both the call instruction as well as from the function itself. Unfortunately, ASan relies on this annotation to unpoison the stack by inserting calls to `_asan_handle_no_return` before `noreturn` functions. This is important for functions that do not return but access the the stack memory, e.g., unwinder functions *like* `longjmp` (`longjmp` itself is actually "double-proofed" via its interceptor). The result is that when ASan and UBSan are combined, the `noreturn` attributes are missing and ASan cannot unpoison the stack, so it has false positives when stack unwinding is used. Changes: # UBSan now adds the `expect_noreturn` attribute whenever it removes the `noreturn` attribute from a function # ASan additionally checks for the presence of this attribute Generated code: ``` call void @__asan_handle_no_return // Additionally inserted to avoid false positives call void @longjmp call void @__asan_handle_no_return call void @__ubsan_handle_builtin_unreachable unreachable ``` The second call to `__asan_handle_no_return` is redundant. This will be cleaned up in a follow-up patch. rdar://problem/40723397 Reviewers: delcypher, eugenis Tags: #sanitizers Differential Revision: https://reviews.llvm.org/D56624 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@352003 91177308-0d34-0410-b5e6-96231b3b80d8 --- docs/LangRef.rst | 4 ++ include/llvm/Bitcode/LLVMBitCodes.h | 1 + include/llvm/IR/Attributes.td | 4 ++ lib/AsmParser/LLLexer.cpp | 1 + lib/AsmParser/LLParser.cpp | 4 ++ lib/AsmParser/LLToken.h | 1 + lib/Bitcode/Reader/BitcodeReader.cpp | 6 ++- lib/Bitcode/Writer/BitcodeWriter.cpp | 2 + lib/IR/Attributes.cpp | 2 + lib/IR/Verifier.cpp | 1 + lib/Transforms/IPO/ForceFunctionAttrs.cpp | 1 + .../Instrumentation/AddressSanitizer.cpp | 3 +- lib/Transforms/Utils/CodeExtractor.cpp | 1 + test/Bitcode/attributes.ll | 11 ++++- .../AddressSanitizer/instrument-no-return.ll | 48 +++++++++++-------- 15 files changed, 65 insertions(+), 25 deletions(-) diff --git a/docs/LangRef.rst b/docs/LangRef.rst index 65c8c75a22d..32c4d458e4e 100644 --- a/docs/LangRef.rst +++ b/docs/LangRef.rst @@ -1458,6 +1458,10 @@ example: This function attribute indicates that the function never returns normally. This produces undefined behavior at runtime if the function ever does dynamically return. +``expect_noreturn`` + This function attribute indicates that the function is unlikely to return + normally, but that it still allowed to do so. This is useful in cases where + ``noreturn`` is too strong a guarantee. ``norecurse`` This function attribute indicates that the function does not call itself either directly or indirectly down any possible call path. This produces diff --git a/include/llvm/Bitcode/LLVMBitCodes.h b/include/llvm/Bitcode/LLVMBitCodes.h index ce853cd3998..0015668e383 100644 --- a/include/llvm/Bitcode/LLVMBitCodes.h +++ b/include/llvm/Bitcode/LLVMBitCodes.h @@ -603,6 +603,7 @@ enum AttributeKindCodes { ATTR_KIND_OPT_FOR_FUZZING = 57, ATTR_KIND_SHADOWCALLSTACK = 58, ATTR_KIND_SPECULATIVE_LOAD_HARDENING = 59, + ATTR_KIND_EXPECT_NO_RETURN = 60, }; enum ComdatSelectionKindCodes { diff --git a/include/llvm/IR/Attributes.td b/include/llvm/IR/Attributes.td index e786d85d05a..dc972cf5a09 100644 --- a/include/llvm/IR/Attributes.td +++ b/include/llvm/IR/Attributes.td @@ -106,6 +106,10 @@ def NoRedZone : EnumAttr<"noredzone">; /// Mark the function as not returning. def NoReturn : EnumAttr<"noreturn">; +/// Mark the function as unlikely to return. This is useful in cases where +/// `noreturn` is too strong a guarantee. +def ExpectNoReturn : EnumAttr<"expect_noreturn">; + /// Disable Indirect Branch Tracking. def NoCfCheck : EnumAttr<"nocf_check">; diff --git a/lib/AsmParser/LLLexer.cpp b/lib/AsmParser/LLLexer.cpp index b543115a88e..a60639bef0c 100644 --- a/lib/AsmParser/LLLexer.cpp +++ b/lib/AsmParser/LLLexer.cpp @@ -656,6 +656,7 @@ lltok::Kind LLLexer::LexIdentifier() { KEYWORD(nonnull); KEYWORD(noredzone); KEYWORD(noreturn); + KEYWORD(expect_noreturn); KEYWORD(nocf_check); KEYWORD(nounwind); KEYWORD(optforfuzzing); diff --git a/lib/AsmParser/LLParser.cpp b/lib/AsmParser/LLParser.cpp index 855c5d26500..9167694607f 100644 --- a/lib/AsmParser/LLParser.cpp +++ b/lib/AsmParser/LLParser.cpp @@ -1248,6 +1248,8 @@ bool LLParser::ParseFnAttributeValuePairs(AttrBuilder &B, case lltok::kw_nonlazybind: B.addAttribute(Attribute::NonLazyBind); break; case lltok::kw_noredzone: B.addAttribute(Attribute::NoRedZone); break; case lltok::kw_noreturn: B.addAttribute(Attribute::NoReturn); break; + case lltok::kw_expect_noreturn: + B.addAttribute(Attribute::ExpectNoReturn); break; case lltok::kw_nocf_check: B.addAttribute(Attribute::NoCfCheck); break; case lltok::kw_norecurse: B.addAttribute(Attribute::NoRecurse); break; case lltok::kw_nounwind: B.addAttribute(Attribute::NoUnwind); break; @@ -1611,6 +1613,7 @@ bool LLParser::ParseOptionalParamAttrs(AttrBuilder &B) { case lltok::kw_nonlazybind: case lltok::kw_noredzone: case lltok::kw_noreturn: + case lltok::kw_expect_noreturn: case lltok::kw_nocf_check: case lltok::kw_nounwind: case lltok::kw_optforfuzzing: @@ -1708,6 +1711,7 @@ bool LLParser::ParseOptionalReturnAttrs(AttrBuilder &B) { case lltok::kw_nonlazybind: case lltok::kw_noredzone: case lltok::kw_noreturn: + case lltok::kw_expect_noreturn: case lltok::kw_nocf_check: case lltok::kw_nounwind: case lltok::kw_optforfuzzing: diff --git a/lib/AsmParser/LLToken.h b/lib/AsmParser/LLToken.h index 41899b29ce5..b90394f4c1f 100644 --- a/lib/AsmParser/LLToken.h +++ b/lib/AsmParser/LLToken.h @@ -200,6 +200,7 @@ enum Kind { kw_nonnull, kw_noredzone, kw_noreturn, + kw_expect_noreturn, kw_nocf_check, kw_nounwind, kw_optforfuzzing, diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp index 0b93a61dc40..d377b6ae515 100644 --- a/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/lib/Bitcode/Reader/BitcodeReader.cpp @@ -1186,8 +1186,8 @@ static uint64_t getRawAttributeMask(Attribute::AttrKind Val) { case Attribute::NoCfCheck: return 1ULL << 57; case Attribute::OptForFuzzing: return 1ULL << 58; case Attribute::ShadowCallStack: return 1ULL << 59; - case Attribute::SpeculativeLoadHardening: - return 1ULL << 60; + case Attribute::SpeculativeLoadHardening: return 1ULL << 60; + case Attribute::ExpectNoReturn: return 1ULL << 61; case Attribute::Dereferenceable: llvm_unreachable("dereferenceable attribute not supported in raw format"); break; @@ -1366,6 +1366,8 @@ static Attribute::AttrKind getAttrFromCode(uint64_t Code) { return Attribute::NoRedZone; case bitc::ATTR_KIND_NO_RETURN: return Attribute::NoReturn; + case bitc::ATTR_KIND_EXPECT_NO_RETURN: + return Attribute::ExpectNoReturn; case bitc::ATTR_KIND_NOCF_CHECK: return Attribute::NoCfCheck; case bitc::ATTR_KIND_NO_UNWIND: diff --git a/lib/Bitcode/Writer/BitcodeWriter.cpp b/lib/Bitcode/Writer/BitcodeWriter.cpp index f4a539e51f7..f16cdacaae3 100644 --- a/lib/Bitcode/Writer/BitcodeWriter.cpp +++ b/lib/Bitcode/Writer/BitcodeWriter.cpp @@ -654,6 +654,8 @@ static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) { return bitc::ATTR_KIND_NO_RED_ZONE; case Attribute::NoReturn: return bitc::ATTR_KIND_NO_RETURN; + case Attribute::ExpectNoReturn: + return bitc::ATTR_KIND_EXPECT_NO_RETURN; case Attribute::NoCfCheck: return bitc::ATTR_KIND_NOCF_CHECK; case Attribute::NoUnwind: diff --git a/lib/IR/Attributes.cpp b/lib/IR/Attributes.cpp index a474d5740fe..ab49adba81c 100644 --- a/lib/IR/Attributes.cpp +++ b/lib/IR/Attributes.cpp @@ -298,6 +298,8 @@ std::string Attribute::getAsString(bool InAttrGrp) const { return "noredzone"; if (hasAttribute(Attribute::NoReturn)) return "noreturn"; + if (hasAttribute(Attribute::ExpectNoReturn)) + return "expect_noreturn"; if (hasAttribute(Attribute::NoCfCheck)) return "nocf_check"; if (hasAttribute(Attribute::NoRecurse)) diff --git a/lib/IR/Verifier.cpp b/lib/IR/Verifier.cpp index 1000e210533..3b3e3158059 100644 --- a/lib/IR/Verifier.cpp +++ b/lib/IR/Verifier.cpp @@ -1477,6 +1477,7 @@ void Verifier::visitModuleFlagCGProfileEntry(const MDOperand &MDO) { static bool isFuncOnlyAttr(Attribute::AttrKind Kind) { switch (Kind) { case Attribute::NoReturn: + case Attribute::ExpectNoReturn: case Attribute::NoCfCheck: case Attribute::NoUnwind: case Attribute::NoInline: diff --git a/lib/Transforms/IPO/ForceFunctionAttrs.cpp b/lib/Transforms/IPO/ForceFunctionAttrs.cpp index cd1fc379820..7300ca06ce7 100644 --- a/lib/Transforms/IPO/ForceFunctionAttrs.cpp +++ b/lib/Transforms/IPO/ForceFunctionAttrs.cpp @@ -41,6 +41,7 @@ static Attribute::AttrKind parseAttrKind(StringRef Kind) { .Case("nonlazybind", Attribute::NonLazyBind) .Case("noredzone", Attribute::NoRedZone) .Case("noreturn", Attribute::NoReturn) + .Case("expect_noreturn", Attribute::ExpectNoReturn) .Case("nocf_check", Attribute::NoCfCheck) .Case("norecurse", Attribute::NoRecurse) .Case("nounwind", Attribute::NoUnwind) diff --git a/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/lib/Transforms/Instrumentation/AddressSanitizer.cpp index 1a007b4258f..8d27ee9d1e1 100644 --- a/lib/Transforms/Instrumentation/AddressSanitizer.cpp +++ b/lib/Transforms/Instrumentation/AddressSanitizer.cpp @@ -2568,7 +2568,8 @@ bool AddressSanitizer::runOnFunction(Function &F) { if (CS) { // A call inside BB. TempsToInstrument.clear(); - if (CS.doesNotReturn()) NoReturnCalls.push_back(CS.getInstruction()); + if (CS.doesNotReturn() || CS.hasFnAttr(Attribute::ExpectNoReturn)) + NoReturnCalls.push_back(CS.getInstruction()); } if (CallInst *CI = dyn_cast(&Inst)) maybeMarkSanitizerLibraryCallNoBuiltin(CI, TLI); diff --git a/lib/Transforms/Utils/CodeExtractor.cpp b/lib/Transforms/Utils/CodeExtractor.cpp index 03e2b9db078..3cf26b5e0c3 100644 --- a/lib/Transforms/Utils/CodeExtractor.cpp +++ b/lib/Transforms/Utils/CodeExtractor.cpp @@ -779,6 +779,7 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs, case Attribute::NoBuiltin: case Attribute::NoCapture: case Attribute::NoReturn: + case Attribute::ExpectNoReturn: case Attribute::None: case Attribute::NonNull: case Attribute::ReadNone: diff --git a/test/Bitcode/attributes.ll b/test/Bitcode/attributes.ll index de3cf8dd4d7..e96007e5629 100644 --- a/test/Bitcode/attributes.ll +++ b/test/Bitcode/attributes.ll @@ -204,7 +204,7 @@ define void @f34() ; CHECK: define void @f34() { call void @nobuiltin() nobuiltin -; CHECK: call void @nobuiltin() #36 +; CHECK: call void @nobuiltin() #37 ret void; } @@ -351,6 +351,12 @@ define void @f59() shadowcallstack ret void } +; CHECK: define void @f60() #36 +define void @f60() expect_noreturn +{ + ret void +} + ; CHECK: attributes #0 = { noreturn } ; CHECK: attributes #1 = { nounwind } ; CHECK: attributes #2 = { readnone } @@ -387,4 +393,5 @@ define void @f59() shadowcallstack ; CHECK: attributes #33 = { speculatable } ; CHECK: attributes #34 = { sanitize_hwaddress } ; CHECK: attributes #35 = { shadowcallstack } -; CHECK: attributes #36 = { nobuiltin } +; CHECK: attributes #36 = { expect_noreturn } +; CHECK: attributes #37 = { nobuiltin } diff --git a/test/Instrumentation/AddressSanitizer/instrument-no-return.ll b/test/Instrumentation/AddressSanitizer/instrument-no-return.ll index 2e90bfc64b2..b8e6aef9848 100644 --- a/test/Instrumentation/AddressSanitizer/instrument-no-return.ll +++ b/test/Instrumentation/AddressSanitizer/instrument-no-return.ll @@ -1,37 +1,45 @@ -; RUN: opt < %s -asan -asan-module -S | FileCheck %s +; RUN: opt < %s -asan -S | FileCheck %s ; AddressSanitizer must insert __asan_handle_no_return ; before every noreturn call or invoke. target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64" target triple = "x86_64-unknown-linux-gnu" -declare void @MyNoReturnFunc(i32) noreturn +declare void @NormalFunc() +declare void @NoReturnFunc() noreturn -define i32 @Call1(i8* nocapture %arg) uwtable sanitize_address { -entry: - call void @MyNoReturnFunc(i32 1) noreturn ; The call insn has noreturn attr. -; CHECK: @Call1 -; CHECK: call void @__asan_handle_no_return -; CHECK-NEXT: call void @MyNoReturnFunc -; CHECK-NEXT: unreachable +; Instrument calls to noreturn functions (regardless of callsite) +define i32 @Call1() sanitize_address { + call void @NoReturnFunc() unreachable } - -define i32 @Call2(i8* nocapture %arg) uwtable sanitize_address { -entry: - call void @MyNoReturnFunc(i32 1) ; No noreturn attribure on the call. -; CHECK: @Call2 +; CHECK-LABEL: @Call1 ; CHECK: call void @__asan_handle_no_return -; CHECK-NEXT: call void @MyNoReturnFunc -; CHECK-NEXT: unreachable +; CHECK-NEXT: call void @NoReturnFunc + +; Instrument noreturn call sites (regardless of function) +define i32 @Call2() sanitize_address { + call void @NormalFunc() noreturn unreachable } +; CHECK-LABEL: @Call2 +; CHECK: call void @__asan_handle_no_return +; CHECK-NEXT: call void @NormalFunc + +; Also instrument expect_noreturn call sites +define i32 @Call3() sanitize_address { + call void @NormalFunc() expect_noreturn + ret i32 0 +} +; CHECK-LABEL: @Call3 +; CHECK: call void @__asan_handle_no_return +; CHECK-NEXT: call void @NormalFunc declare i32 @__gxx_personality_v0(...) -define i64 @Invoke1(i8** %esc) nounwind uwtable ssp sanitize_address personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) { +define i64 @Invoke1(i8** %esc) sanitize_address personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) { entry: - invoke void @MyNoReturnFunc(i32 1) + invoke void @NoReturnFunc() to label %invoke.cont unwind label %lpad invoke.cont: @@ -42,8 +50,8 @@ lpad: filter [0 x i8*] zeroinitializer ret i64 1 } -; CHECK: @Invoke1 +; CHECK-LABEL: @Invoke1 ; CHECK: call void @__asan_handle_no_return -; CHECK-NEXT: invoke void @MyNoReturnFunc +; CHECK-NEXT: invoke void @NoReturnFunc ; CHECK: ret i64 0 ; CHECK: ret i64 1 -- 2.50.1