[ubsan] Function Sanitizer: Don't require writable text segments

[vsk]

Summary

This change will make it possible to use -fsanitize=function on Darwin and
possibly on other platforms. It fixes an issue with the way RTTI is stored into
function prologue data.

On Darwin, addresses stored in prologue data can't require run-time fixups and
must be PC-relative. Run-time fixups are undesirable because they necessitate
writable text segments, which can lead to security issues. And absolute
addresses are undesirable because they break PIE mode.

The fix is to create a private global which points to the RTTI, and then to
encode a PC-relative reference to the global into prologue data.

[pcc]

Thanks. Once we apply this fix to other platforms this would seem to fix PR17633.

We may also want to change the ubsan function signature in order to avoid bad pointer reads in case of version mismatches.

lib/CodeGen/CodeGenFunction.cpp

434

I think you can just do this unconditionally. As far as I know, all three object formats should support 32-bit relative relocations on x86 and x86_64, which are the only two architectures which currently support -fsanitize=function.

445

This can be constant I think.

463

Maybe use Int32Ty (here and below). That should be sufficient under the small code model.

[pcc]

lib/CodeGen/CodeGenFunction.cpp

463

Sorry, I meant that the difference could be truncated to Int32Ty, and stored as an integer, not a pointer.

[vsk]

Thanks for your comments!

lib/CodeGen/CodeGenFunction.cpp

434

Ok! I'll make this change in the next version of this patch (probably tomorrow).

445

Can the value of the global change if a dynamic library is unloaded? I'm unsure about this case: A.dylib and B.dylib both have linkonce_odr definitions of X, dyld picks the definition from A, and A is unloaded.

463

I tried this out but it resulted in a mysterious dyld crash after the indirect callee returns, which I've yet to understand. I'll have to try using a Debug build of dyld to see what's happening.

[vsk]

Address review feedback:

• Enable this change on all platforms.
• Bump the function signature version.
• Store the indirect RTTI pointer as an IntPtrTy. This saves a "ptrtoint" instruction. Truncating to Int32Ty should be possible, but causes dyld to crash, and also requires an extra sign-extension.

[vsk]

lib/CodeGen/CodeGenFunction.cpp

463

@pcc It turned out that the jump we encode in getUBSanFunctionSignature not correct if we truncate a 64-bit address to a 32-bit address. If we jump to +8 instead of +12, we can make the truncation work :). There is no dyld bug.

That said, I think we should store the pc-rel address as an IntPtrTy to keep things a bit simpler. We could avoid logic to truncate/sign-extend conditionally, and not emit those instructions at all. Wdyt?

[pcc]

lib/CodeGen/CodeGenFunction.cpp

445

I don't think it is possible to get into a situation where a global's address can change if a library is unloaded. In your scenario, suppose that B takes the address of X and then A is dlclosed. In that scenario the program would rightly expect the address to continue to be valid; to do otherwise would break the C++ standard's guarantee that each global has a unique address. That I believe is why POSIX does not provide a mechanism to require that a DSO is unloaded, only to request it to be unloaded; see the specification of dlclose [0].

The Linux manpage for dlclose is more specific: "If the reference count drops to zero and no other loaded libraries use symbols in it, then the dynamic library is unloaded." I believe that any references to X in B would count as a use of X in A.

[0] http://pubs.opengroup.org/onlinepubs/009695399/functions/dlclose.html

463

The problem is that COFF does not have a relocation that can express a 64-bit relative reference. If we want something that will work with all object formats, 32-bit is probably best.

[vsk]

Address review feedback.

lib/CodeGen/CodeGenFunction.cpp

445

I see, thanks for explaining!

463

Got it.

[pcc]

LGTM, thanks!

lib/CodeGen/CodeGenFunction.cpp

460

Is this conditional necessary? I believe that IRBuilder will return the same value back if you cast to the same type.

[vsk]

lib/CodeGen/CodeGenFunction.cpp

460

Thanks, it's not needed. I've gotten rid of it.

lib/CodeGen/CGExpr.cpp

if (getLangOpts().CPlusPlus && SanOpts.has(SanitizerKind::Function) &&
       (!TargetDecl || !isa<FunctionDecl>(TargetDecl))) {
     if (llvm::Constant *PrefixSig =
             CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM)) {
       SanitizerScope SanScope(this);
       llvm::Constant *FTRTTIConst =
           CGM.GetAddrOfRTTIDescriptor(QualType(FnType, 0), /*ForEH=*/true);
       llvm::Type *PrefixStructTyElems[] = {
         PrefixSig->getType(),
-        FTRTTIConst->getType()
+        (CGM.getTriple().isOSDarwin() ? Int8PtrPtrTy : Int8PtrTy)
       };
       llvm::StructType *PrefixStructTy = llvm::StructType::get(
           CGM.getLLVMContext(), PrefixStructTyElems, /*isPacked=*/true);
 
       llvm::Value *CalleePtr = Callee.getFunctionPointer();
 
       llvm::Value *CalleePrefixStruct = Builder.CreateBitCast(
           CalleePtr, llvm::PointerType::getUnqual(PrefixStructTy));
       llvm::Value *CalleeSigPtr =
           Builder.CreateConstGEP2_32(PrefixStructTy, CalleePrefixStruct, 0, 0);
       llvm::Value *CalleeSig =
           Builder.CreateAlignedLoad(CalleeSigPtr, getIntAlign());
       llvm::Value *CalleeSigMatch = Builder.CreateICmpEQ(CalleeSig, PrefixSig);
 
       llvm::BasicBlock *Cont = createBasicBlock("cont");
       llvm::BasicBlock *TypeCheck = createBasicBlock("typecheck");
       Builder.CreateCondBr(CalleeSigMatch, TypeCheck, Cont);
 
       EmitBlock(TypeCheck);
       llvm::Value *CalleeRTTIPtr =
           Builder.CreateConstGEP2_32(PrefixStructTy, CalleePrefixStruct, 0, 1);
-      llvm::Value *CalleeRTTI =
+      llvm::Value *CalleeRTTIEncoded =
           Builder.CreateAlignedLoad(CalleeRTTIPtr, getPointerAlign());
+      llvm::Value *CalleeRTTI =
+          DecodeAddrUsedInPrologue(CalleePtr, CalleeRTTIEncoded);
       llvm::Value *CalleeRTTIMatch =
           Builder.CreateICmpEQ(CalleeRTTI, FTRTTIConst);
       llvm::Constant *StaticData[] = {
         EmitCheckSourceLocation(E->getLocStart()),
         EmitCheckTypeDescriptor(CalleeType)
       };
       EmitCheck(std::make_pair(CalleeRTTIMatch, SanitizerKind::Function),
                 SanitizerHandler::FunctionTypeMismatch, StaticData, CalleePtr);

lib/CodeGen/CodeGenFunction.h

public:
   /// EmitFunctionInstrumentation - Emit LLVM code to call the specified
   /// instrumentation function with the current function and the call site, if
   /// function instrumentation is enabled.
   void EmitFunctionInstrumentation(const char *Fn);
 
   /// EmitMCountInstrumentation - Emit call to .mcount.
   void EmitMCountInstrumentation();
 
+  /// Encode an address into a form suitable for use in a function prologue.
+  llvm::Constant *EncodeAddrForUseInPrologue(llvm::Function *F,
+                                             llvm::Constant *Addr);
+
+  /// Decode an address used in a function prologue, encoded by \c
+  /// EncodeAddrForUseInPrologue.
+  llvm::Value *DecodeAddrUsedInPrologue(llvm::Value *F,
+                                        llvm::Value *EncodedAddr);
+
   /// EmitFunctionProlog - Emit the target specific LLVM code to load the
   /// arguments for the given function. This is also responsible for naming the
   /// LLVM function arguments.
   void EmitFunctionProlog(const CGFunctionInfo &FI,
                           llvm::Function *Fn,
                           const FunctionArgList &Args);
 
   /// EmitFunctionEpilog - Emit the target specific LLVM code to return the

lib/CodeGen/CodeGenFunction.cpp

 }
 
 /// ShouldXRayInstrument - Return true if the current function should be
 /// instrumented with XRay nop sleds.
 bool CodeGenFunction::ShouldXRayInstrumentFunction() const {
   return CGM.getCodeGenOpts().XRayInstrumentFunctions;
 }
 
+llvm::Constant *
+CodeGenFunction::EncodeAddrForUseInPrologue(llvm::Function *F,
+                                            llvm::Constant *Addr) {
+  if (!CGM.getTriple().isOSDarwin())
+    return Addr;
+
+  // Addresses stored in prologue data can't require run-time fixups and must
+  // be PC-relative. Run-time fixups are undesirable because they necessitate
+  // writable text segments, which is unsafe. And absolute addresses are
+  // undesirable because they break PIE mode.
+
+  // Add a layer of indirection through a private global. Taking its address
+  // won't result in a run-time fixup, even if Addr has linkonce_odr linkage.
+  auto *GV = new llvm::GlobalVariable(CGM.getModule(), Addr->getType(),
+                                      /*isConstant=*/false,
+                                      llvm::GlobalValue::PrivateLinkage, Addr);
+
+  // Create a PC-relative address.
+  auto *GOTAsInt = llvm::ConstantExpr::getPtrToInt(GV, IntPtrTy);
+  auto *FuncAsInt = llvm::ConstantExpr::getPtrToInt(F, IntPtrTy);
+  auto *PCRelAsInt = llvm::ConstantExpr::getSub(GOTAsInt, FuncAsInt);
+  return llvm::ConstantExpr::getIntToPtr(PCRelAsInt, Int8PtrPtrTy);
+}
+
+llvm::Value *
+CodeGenFunction::DecodeAddrUsedInPrologue(llvm::Value *F,
+                                          llvm::Value *EncodedAddr) {
+  if (!CGM.getTriple().isOSDarwin())
+    return EncodedAddr;
+
+  // Reconstruct the address of the global.
+  auto *PCRelAsInt =
+      Builder.CreatePtrToInt(EncodedAddr, IntPtrTy, "encoded_addr.int");
+  auto *FuncAsInt = Builder.CreatePtrToInt(F, IntPtrTy, "func_addr.int");
+  auto *GOTAsInt = Builder.CreateAdd(PCRelAsInt, FuncAsInt, "global_addr.int");
+  auto *GOTAddr = Builder.CreateIntToPtr(GOTAsInt, Int8PtrPtrTy, "global_addr");
+
+  // Load the original pointer through the global.
+  return Builder.CreateLoad(Address(GOTAddr, getPointerAlign()),
+                            "decoded_addr");
+}
+
 /// EmitFunctionInstrumentation - Emit LLVM code to call the specified
 /// instrumentation function with the current function and the call site, if
 /// function instrumentation is enabled.
 void CodeGenFunction::EmitFunctionInstrumentation(const char *Fn) {
   auto NL = ApplyDebugLocation::CreateArtificial(*this);
   // void __cyg_profile_func_{enter,exit} (void *this_fn, void *call_site);
   llvm::PointerType *PointerTy = Int8PtrTy;
   llvm::Type *ProfileFuncArgs[] = { PointerTy, PointerTy };
void CodeGenFunction::StartFunction(GlobalDecl GD,
   // If we are checking function types, emit a function type signature as
   // prologue data.
   if (getLangOpts().CPlusPlus && SanOpts.has(SanitizerKind::Function)) {
     if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {
       if (llvm::Constant *PrologueSig =
               CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM)) {
         llvm::Constant *FTRTTIConst =
             CGM.GetAddrOfRTTIDescriptor(FD->getType(), /*ForEH=*/true);
-        llvm::Constant *PrologueStructElems[] = { PrologueSig, FTRTTIConst };
+        llvm::Constant *FTRTTIConstEncoded =
+            EncodeAddrForUseInPrologue(Fn, FTRTTIConst);
+        llvm::Constant *PrologueStructElems[] = {PrologueSig,
+                                                 FTRTTIConstEncoded};
         llvm::Constant *PrologueStructConst =
             llvm::ConstantStruct::getAnon(PrologueStructElems, /*Packed=*/true);
         Fn->setPrologueData(PrologueStructConst);
       }
     }
   }
 
   // If we're checking nullability, we need to know whether we can check the

test/CodeGenCXX/catch-undef-behavior.cpp

 // RUN: %clang_cc1 -std=c++11 -fsanitize=signed-integer-overflow,integer-divide-by-zero,float-divide-by-zero,shift-base,shift-exponent,unreachable,return,vla-bound,alignment,null,vptr,object-size,float-cast-overflow,bool,enum,array-bounds,function -fsanitize-recover=signed-integer-overflow,integer-divide-by-zero,float-divide-by-zero,shift-base,shift-exponent,vla-bound,alignment,null,vptr,object-size,float-cast-overflow,bool,enum,array-bounds,function -emit-llvm %s -o - -triple x86_64-linux-gnu | opt -instnamer -S | FileCheck %s
 // RUN: %clang_cc1 -std=c++11 -fsanitize=vptr,address -fsanitize-recover=vptr,address -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK-ASAN
 // RUN: %clang_cc1 -std=c++11 -fsanitize=vptr -fsanitize-recover=vptr -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=DOWNCAST-NULL
 // RUN: %clang_cc1 -std=c++11 -fsanitize=function -emit-llvm %s -o - -triple x86_64-linux-gnux32 | FileCheck %s --check-prefix=CHECK-X32
 // RUN: %clang_cc1 -std=c++11 -fsanitize=function -emit-llvm %s -o - -triple i386-linux-gnu | FileCheck %s --check-prefix=CHECK-X86
+// RUN: %clang_cc1 -std=c++11 -fsanitize=function -emit-llvm %s -o - -triple x86_64-apple-darwin | FileCheck %s --check-prefix=FSAN-DARWIN
 
 struct S {
   double d;
   int a, b;
   virtual int f();
 };
 
 // Check that type descriptor global is not modified by ASan.
 // CHECK-ASAN: [[TYPE_DESCR:@[0-9]+]] = private unnamed_addr constant { i16, i16, [4 x i8] } { i16 -1, i16 0, [4 x i8] c"'S'\00" }
 
 // Check that type mismatch handler is not modified by ASan.
 // CHECK-ASAN: private unnamed_addr global { { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }*, i8*, i8 } { {{.*}}, { i16, i16, [4 x i8] }* [[TYPE_DESCR]], {{.*}} }
 
+// FSAN-DARWIN: [[IndirectRTTI_ZTIFvPFviEE:@.+]] = private global i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*)
+
 struct T : S {};
 
 // CHECK-LABEL: @_Z17reference_binding
 void reference_binding(int *p, S *q) {
   // C++ core issue 453: If an lvalue to which a reference is directly bound
   // designates neither an existing object or function of an appropriate type,
   // nor a region of storage of suitable size and alignment to contain an object
   // of the reference's type, the behavior is undefined.
void downcast_reference(B &b) {
   // CHECK:      [[C_INT:%.+]] = ptrtoint %class.C* [[C]] to i64
   // CHECK-NEXT: [[MASKED:%.+]] = and i64 [[C_INT]], 15
   // CHECK-NEXT: [[TEST:%.+]] = icmp eq i64 [[MASKED]], 0
   // AND the alignment test with the objectsize test.
   // CHECK:      [[AND:%.+]] = and i1 {{.*}}, [[TEST]]
   // CHECK-NEXT: br i1 [[AND]]
 }
 
+//
+// FSAN-DARWIN: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i8** }> <{ i32 1413876459, i8** inttoptr (i64 sub (i64 ptrtoint (i8** [[IndirectRTTI_ZTIFvPFviEE]] to i64), i64 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i64)) to i8**) }> 
 // CHECK-LABEL: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i8* }> <{ i32 1413876459, i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*) }>
 // CHECK-X32: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i8* }> <{ i32 1413875435, i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*) }>
 // CHECK-X86: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i8* }> <{ i32 1413875435, i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*) }>
 void indirect_function_call(void (*p)(int)) {
   // CHECK: [[PTR:%.+]] = bitcast void (i32)* {{.*}} to <{ i32, i8* }>*
 
   // Signature check
   // CHECK-NEXT: [[SIGPTR:%.+]] = getelementptr <{ i32, i8* }>, <{ i32, i8* }>* [[PTR]], i32 0, i32 0
   // CHECK-NEXT: [[SIG:%.+]] = load i32, i32* [[SIGPTR]]
   // CHECK-NEXT: [[SIGCMP:%.+]] = icmp eq i32 [[SIG]], 1413876459
   // CHECK-NEXT: br i1 [[SIGCMP]]
 
   // RTTI pointer check
   // CHECK: [[RTTIPTR:%.+]] = getelementptr <{ i32, i8* }>, <{ i32, i8* }>* [[PTR]], i32 0, i32 1
   // CHECK-NEXT: [[RTTI:%.+]] = load i8*, i8** [[RTTIPTR]]
   // CHECK-NEXT: [[RTTICMP:%.+]] = icmp eq i8* [[RTTI]], bitcast ({ i8*, i8* }* @_ZTIFviE to i8*)
   // CHECK-NEXT: br i1 [[RTTICMP]]
+
+  // FSAN-DARWIN: [[PTR:%.+]] = bitcast void (i32)* {{.*}} to <{ i32, i8** }>*
+
+  // Signature check
+  // FSAN-DARWIN-NEXT: [[SIGPTR:%.+]] = getelementptr <{ i32, i8** }>, <{ i32, i8** }>* [[PTR]], i32 0, i32 0
+  // FSAN-DARWIN-NEXT: [[SIG:%.+]] = load i32, i32* [[SIGPTR]]
+  // FSAN-DARWIN-NEXT: [[SIGCMP:%.+]] = icmp eq i32 [[SIG]], 1413876459
+  // FSAN-DARWIN-NEXT: br i1 [[SIGCMP]]
+
+  // RTTI pointer check
+  // FSAN-DARWIN: [[RTTIPTR:%.+]] = getelementptr <{ i32, i8** }>, <{ i32, i8** }>* [[PTR]], i32 0, i32 1
+  // FSAN-DARWIN-NEXT: [[RTTIEnc:%.+]] = load i8**, i8*** [[RTTIPTR]]
+  // FSAN-DARWIN-NEXT: [[RTTIEncInt:%.+]] = ptrtoint i8** [[RTTIEnc]] to i64
+  // FSAN-DARWIN-NEXT: [[FuncAddrInt:%.+]] = ptrtoint void (i32)* {{.*}} to i64
+  // FSAN-DARWIN-NEXT: [[IndirectGVInt:%.+]] = add i64 [[RTTIEncInt]], [[FuncAddrInt]]
+  // FSAN-DARWIN-NEXT: [[IndirectGV:%.+]] = inttoptr i64 [[IndirectGVInt]] to i8**
+  // FSAN-DARWIN-NEXT: [[RTTI:%.+]] = load i8*, i8** [[IndirectGV]], align 8
+  // FSAN-DARWIN-NEXT: [[RTTICMP:%.+]] = icmp eq i8* [[RTTI]], bitcast ({ i8*, i8* }* @_ZTIFviE to i8*)
+  // FSAN-DARWIN-NEXT: br i1 [[RTTICMP]]
+
   p(42);
 }
 
 namespace UpcastPointerTest {
 struct S {};
 struct T : S { double d; };
 struct V : virtual S {};