Change -fsanitize=function to place two words before the function entry

[MaskRay]

Summary

The current implementation of -fsanitize=function places two words (the prolog
signature and the RTTI proxy) at the function entry, which makes the feature
incompatible with Intel Indirect Branch Tracking (IBT) that needs an ENDBR instruction
at the function entry. To allow the combination, move the two words before the
function entry, similar to -fsanitize=kcfi.

Armv8.5 Branch Target Identification (BTI) has a similar requirement.

Note: for IBT and BTI, whether a function gets a marker instruction at the entry
generally cannot be assumed (it can be disabled by a function attribute or
stronger LTO optimizations).

It is extremely unlikely for two words preceding a function entry to be
inaccessible. One way to achieve this is by ensuring that a function is
aligned at a page boundary and making the preceding page unmapped or
unreadable. This is not reasonable for application or library code.
(Think: the first text section has crt* code not instrumented by
-fsanitize=function.)

We use 0xc105cafe for all targets. .long 0xc105cafe disassembles to invalid
instructions on all architectures I have tested, except Power where it is
lfs 8, -13570(5) (Load Floating-Point with a weird offset, unlikely to be used in real code).

---

For the removed function in AsmPrinter.cpp, remove an assert: mdconst::extract
already asserts non-nullness.

For compiler-rt/test/ubsan/TestCases/TypeCheck/Function/function.cpp,
when the function doesn't have prolog/epilog (-O1 and above), after moving the two words,
the address of the function equals the address of ret instruction,
so symbolizing the function will additionally get a non-zero column number.
Adjust the test to allow an optional column number.

  .long   3238382334
  .long   .L__llvm_rtti_proxy-_Z1fv
_Z1fv:   // symbolizing here retrieves the line table entry from the second .loc
  .file   0 ...
  .loc    0 1 0
  .cfi_startproc
  .loc    0 2 1 prologue_end
  retq

[MaskRay]

fix clang/test/CodeGen

[MaskRay]

remove an assert to make this feature available to all targets

[MaskRay]

update test

[MaskRay]

ping :)

[MaskRay]

Ping:)

[peter.smith]

My apologies for not responding. If I've got this right there are 4 related patches:
D148573 (approved)
D148785 Use type hashes rather than RTTI
D148827 support C
D148665 (this one)

My initial impressions is that this makes -fsanitize=function look more like -fsanitize=kcfi which makes it accessible from C and available to more targets. Is there anything that we lose in the process of making these changes? For example I would expect RTTI to have more information available than a type hash, although this might not make any functional difference.

I'll try and look over the next few days and leave some comments, apologies a bit busy at work at the moment so I can't promise anything speedy.

[MaskRay]

In D148665#4316310, @peter.smith wrote:

My apologies for not responding. If I've got this right there are 4 related patches:
D148573 (approved)
D148785 Use type hashes rather than RTTI
D148827 support C
D148665 (this one)

My initial impressions is that this makes -fsanitize=function look more like -fsanitize=kcfi which makes it accessible from C and available to more targets. Is there anything that we lose in the process of making these changes? For example I would expect RTTI to have more information available than a type hash, although this might not make any functional difference.

I'll try and look over the next few days and leave some comments, apologies a bit busy at work at the moment so I can't promise anything speedy.

Thanks! -fsanitize=function will indeed become more like -fsanitize=kcfi.

There is a big difference that -fsanitize=function instrumented code has a signature check for compatibility with object files not compiled with -fsanitize=function (and old implementations of -fsanitize=function with a difference location to place the signature).
-fsanitize=kcfi doesn't have the compatibility guarantee.

[MaskRay]

Ping:)

[peter.smith]

Apologies for the delay LGTM. I think there is a case for setting up the signature to be target specific, but that could in theory be done on demand when a target adds a clashing instruction.

clang/lib/CodeGen/TargetInfo.h

205

Is it worth making this target specific? Defaulting to 0xc105cafe for all targets, if that gets allocated in the future on any one target we can change it without having to test against all other targets.

For example on AArch64 this is is in the decoding space of SME instructions op0 = 0b1 op1 = 0b0000. This may get allocated in the future. Although admittedly it is likely to be very rare to find a use of a SME instruction at the end of a function to cause it to get misidentified.

I suspect most targets have an explicit undefined instruction, such as UDF in AArch64 which is 0x0000???? where ? can be any value. On Arm there two separate 4-byte encodings for Arm, Thumb of UDF.

[MaskRay]

clang/lib/CodeGen/TargetInfo.h

205

Thanks for the review! This is a virtual function, so a target can override as appropriate.

Thanks for informing that this is an encoding that AArch64 may allocate in the future. Since the signature is placed before the function label and is used to protect uninstrumented object files, the signature doesn't need to be out of all encoding space. As long as it is unlikely to be the last 2 instructions of a previous function, this is going to have a good defensive effect. So I expect that A32/A64 may not want to change this as well.

I assume that T32 unlikely uses 0xca 0xca (ldm r2, {r1-r7}) as one of the last two instructions of the previous function, so this seems fine as well, but no objection if T32 wants to change to a different signature :)

clang/lib/CodeGen/CGExpr.cpp

if (llvm::Constant *PrefixSig =
       llvm::StructType *PrefixStructTy = llvm::StructType::get(
           CGM.getLLVMContext(), {PrefixSigType, Int32Ty}, /*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);
+          Builder.CreateConstGEP2_32(PrefixStructTy, CalleePrefixStruct, -1, 0);
       llvm::Value *CalleeSig =
           Builder.CreateAlignedLoad(PrefixSigType, 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);
+          Builder.CreateConstGEP2_32(PrefixStructTy, CalleePrefixStruct, -1, 1);
       llvm::Value *CalleeRTTIEncoded =
           Builder.CreateAlignedLoad(Int32Ty, CalleeRTTIPtr, getPointerAlign());
       llvm::Value *CalleeRTTI =
           DecodeAddrUsedInPrologue(CalleePtr, CalleeRTTIEncoded);
       llvm::Value *CalleeRTTIMatch =
           Builder.CreateICmpEQ(CalleeRTTI, FTRTTIConst);
       llvm::Constant *StaticData[] = {EmitCheckSourceLocation(E->getBeginLoc()),
                                       EmitCheckTypeDescriptor(CalleeType)};

clang/lib/CodeGen/TargetInfo.h

public:
   }
 
   /// Determine whether a call to objc_retainAutoreleasedReturnValue or
   /// objc_unsafeClaimAutoreleasedReturnValue should be marked as 'notail'.
   virtual bool markARCOptimizedReturnCallsAsNoTail() const { return false; }
 
   /// Return a constant used by UBSan as a signature to identify functions
   /// possessing type information, or 0 if the platform is unsupported.
+  /// This magic number is invalid instruction encoding in many targets.
   virtual llvm::Constant *
   getUBSanFunctionSignature(CodeGen::CodeGenModule &CGM) const {
-    return nullptr;
+    return llvm::ConstantInt::get(CGM.Int32Ty, 0xc105cafe);
   }
 
   /// Determine whether a call to an unprototyped functions under
   /// the given calling convention should use the variadic
   /// convention or the non-variadic convention.
   ///
   /// There's a good reason to make a platform's variadic calling
   /// convention be different from its non-variadic calling

clang/lib/CodeGen/TargetInfo.cpp

public:
   void addReturnRegisterOutputs(CodeGenFunction &CGF, LValue ReturnValue,
                                 std::string &Constraints,
                                 std::vector<llvm::Type *> &ResultRegTypes,
                                 std::vector<llvm::Type *> &ResultTruncRegTypes,
                                 std::vector<LValue> &ResultRegDests,
                                 std::string &AsmString,
                                 unsigned NumOutputs) const override;
 
-  llvm::Constant *
-  getUBSanFunctionSignature(CodeGen::CodeGenModule &CGM) const override {
-    unsigned Sig = (0xeb << 0) |  // jmp rel8
-                   (0x06 << 8) |  //           .+0x08
-                   ('v' << 16) |
-                   ('2' << 24);
-    return llvm::ConstantInt::get(CGM.Int32Ty, Sig);
-  }
-
   StringRef getARCRetainAutoreleasedReturnValueMarker() const override {
     return "movl\t%ebp, %ebp"
            "\t\t// marker for objc_retainAutoreleaseReturnValue";
   }
 };
 
 }
 
if (fnType->getCallConv() == CC_C) {
 
       if (!HasAVXType)
         return true;
     }
 
     return TargetCodeGenInfo::isNoProtoCallVariadic(args, fnType);
   }
 
-  llvm::Constant *
-  getUBSanFunctionSignature(CodeGen::CodeGenModule &CGM) const override {
-    unsigned Sig = (0xeb << 0) | // jmp rel8
-                   (0x06 << 8) | //           .+0x08
-                   ('v' << 16) |
-                   ('2' << 24);
-    return llvm::ConstantInt::get(CGM.Int32Ty, Sig);
-  }
-
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
                            CodeGen::CodeGenModule &CGM) const override {
     if (GV->isDeclaration())
       return;
     if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {
       if (FD->hasAttr<X86ForceAlignArgPointerAttr>()) {
         llvm::Function *Fn = cast<llvm::Function>(GV);
         Fn->addFnAttr("stackrealign");

clang/test/CodeGen/ubsan-function.cpp

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s
 
 // CHECK: @[[PROXY:.*]] = private unnamed_addr constant ptr @_ZTIFvvE
 // CHECK: define{{.*}} void @_Z3funv() #0 !func_sanitize ![[FUNCSAN:.*]] {
 void fun() {}
 
 // CHECK-LABEL: define{{.*}} void @_Z6callerPFvvE(ptr noundef %f)
-// CHECK: getelementptr <{ i32, i32 }>, ptr {{.*}}, i32 0, i32 0, !nosanitize
+// CHECK: getelementptr <{ i32, i32 }>, ptr {{.*}}, i32 -1, i32 0, !nosanitize
 // CHECK: load i32, ptr {{.*}}, align {{.*}}, !nosanitize
-// CHECK: icmp eq i32 {{.*}}, 846595819, !nosanitize
+// CHECK: icmp eq i32 {{.*}}, -1056584962, !nosanitize
 // CHECK: br i1 {{.*}}, label %[[LABEL1:.*]], label %[[LABEL4:.*]], !nosanitize
 // CHECK: [[LABEL1]]:
-// CHECK: getelementptr <{ i32, i32 }>, ptr {{.*}}, i32 0, i32 1, !nosanitize
+// CHECK: getelementptr <{ i32, i32 }>, ptr {{.*}}, i32 -1, i32 1, !nosanitize
 // CHECK: load i32, ptr {{.*}}, align {{.*}}, !nosanitize
 // CHECK: icmp eq ptr {{.*}}, @_ZTIFvvE, !nosanitize
 // CHECK: br i1 {{.*}}, label %[[LABEL3:.*]], label %[[LABEL2:[^,]*]], {{.*}}!nosanitize
 // CHECK: [[LABEL2]]:
 // CHECK: call void @__ubsan_handle_function_type_mismatch_v1_abort(ptr {{.*}}, i64 {{.*}}, i64 {{.*}}, i64 {{.*}}) #{{.*}}, !nosanitize
 // CHECK-NOT: unreachable
 // CHECK: br label %[[LABEL3]], !nosanitize
 // CHECK: [[LABEL3]]:
 // CHECK: br label %[[LABEL4]], !nosanitize
 void caller(void (*f)()) { f(); }
 
-// CHECK: ![[FUNCSAN]] = !{i32 846595819, ptr @[[PROXY]]}
+// CHECK: ![[FUNCSAN]] = !{i32 -1056584962, ptr @[[PROXY]]}

compiler-rt/test/ubsan/TestCases/TypeCheck/Function/function.cpp

 
 void make_valid_call() {
   // CHECK-NOT: runtime error: call to function g
   reinterpret_cast<void (*)(int)>(reinterpret_cast<uintptr_t>(g))(42);
 }
 
 void make_invalid_call() {
   // CHECK: function.cpp:[[@LINE+4]]:3: runtime error: call to function f() through pointer to incorrect function type 'void (*)(int)'
-  // CHECK-NEXT: function.cpp:[[@LINE-11]]: note: f() defined here
+  // CHECK-NEXT: function.cpp:[[@LINE-11]]:{{(11:)?}} note: f() defined here
   // NOSYM: function.cpp:[[@LINE+2]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)(int)'
   // NOSYM-NEXT: ({{.*}}+0x{{.*}}): note: (unknown) defined here
   reinterpret_cast<void (*)(int)>(reinterpret_cast<uintptr_t>(f))(42);
 }
 
 void f1(int) {}
 void f2(unsigned int) {}
 void f3(int) noexcept {}

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (PatchableFunctionPrefix) {
     OutStreamer->emitLabel(CurrentPatchableFunctionEntrySym);
     emitNops(PatchableFunctionPrefix);
   } else if (PatchableFunctionEntry) {
     // May be reassigned when emitting the body, to reference the label after
     // the initial BTI (AArch64) or endbr32/endbr64 (x86).
     CurrentPatchableFunctionEntrySym = CurrentFnBegin;
   }
 
+  // Emit the function prologue data for the indirect call sanitizer.
+  if (const MDNode *MD = F.getMetadata(LLVMContext::MD_func_sanitize)) {
+    assert(MD->getNumOperands() == 2);
+
+    auto *PrologueSig = mdconst::extract<Constant>(MD->getOperand(0));
+    auto *FTRTTIProxy = mdconst::extract<Constant>(MD->getOperand(1));
+    emitGlobalConstant(F.getParent()->getDataLayout(), PrologueSig);
+
+    const MCExpr *Proxy = lowerConstant(FTRTTIProxy);
+    const MCExpr *FnExp = MCSymbolRefExpr::create(CurrentFnSym, OutContext);
+    const MCExpr *PCRel = MCBinaryExpr::createSub(Proxy, FnExp, OutContext);
+    // Use 32 bit since only small code model is supported.
+    OutStreamer->emitValue(PCRel, 4u);
+  }
+
   if (isVerbose()) {
     F.printAsOperand(OutStreamer->getCommentOS(),
                      /*PrintType=*/false, F.getParent());
     emitFunctionHeaderComment();
     OutStreamer->getCommentOS() << '\n';
   }
 
   // Emit the function descriptor. This is a virtual function to allow targets
for (const HandlerInfo &HI : Handlers) {
     NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
                        HI.TimerGroupDescription, TimePassesIsEnabled);
     HI.Handler->beginBasicBlockSection(MF->front());
   }
 
   // Emit the prologue data.
   if (F.hasPrologueData())
     emitGlobalConstant(F.getParent()->getDataLayout(), F.getPrologueData());
-
-  // Emit the function prologue data for the indirect call sanitizer.
-  if (const MDNode *MD = F.getMetadata(LLVMContext::MD_func_sanitize)) {
-    assert(TM.getTargetTriple().getArch() == Triple::x86 ||
-           TM.getTargetTriple().getArch() == Triple::x86_64);
-    assert(MD->getNumOperands() == 2);
-
-    auto *PrologueSig = mdconst::extract<Constant>(MD->getOperand(0));
-    auto *FTRTTIProxy = mdconst::extract<Constant>(MD->getOperand(1));
-    assert(PrologueSig && FTRTTIProxy);
-    emitGlobalConstant(F.getParent()->getDataLayout(), PrologueSig);
-
-    const MCExpr *Proxy = lowerConstant(FTRTTIProxy);
-    const MCExpr *FnExp = MCSymbolRefExpr::create(CurrentFnSym, OutContext);
-    const MCExpr *PCRel = MCBinaryExpr::createSub(Proxy, FnExp, OutContext);
-    // Use 32 bit since only small code model is supported.
-    OutStreamer->emitValue(PCRel, 4u);
-  }
 }
 
 /// EmitFunctionEntryLabel - Emit the label that is the entrypoint for the
 /// function.  This can be overridden by targets as required to do custom stuff.
 void AsmPrinter::emitFunctionEntryLabel() {
   CurrentFnSym->redefineIfPossible();
 
   // The function label could have already been emitted if two symbols end up

llvm/test/CodeGen/X86/func-sanitizer.ll

 ; RUN: llc -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
 
-; CHECK: _Z3funv:
-; CHECK:         .cfi_startproc
-; CHECK:         .long   846595819
-; CHECK:         .long   .L__llvm_rtti_proxy-_Z3funv
-; CHECK: .L__llvm_rtti_proxy:
-; CHECK:         .quad   i
-; CHECK:         .size   .L__llvm_rtti_proxy, 8
+; CHECK:      .type _Z3funv,@function
+; CHECK-NEXT:   .long   3238382334  # 0xc105cafe
+; CHECK-NEXT:   .long   .L__llvm_rtti_proxy-_Z3funv
+; CHECK-NEXT: _Z3funv:
+; CHECK-NEXT:   .cfi_startproc
+; CHECK-NEXT:   # %bb.0:
+; CHECK-NEXT:   retq
 
 @i = linkonce_odr constant i32 1
 @__llvm_rtti_proxy = private unnamed_addr constant ptr @i
 
 define dso_local void @_Z3funv() !func_sanitize !0 {
   ret void
 }
 
-!0 = !{i32 846595819, ptr @__llvm_rtti_proxy}
+!0 = !{i32 3238382334, ptr @__llvm_rtti_proxy}

llvm/test/CodeGen/X86/patchable-function-entry-ibt.ll

 ; RUN: llc -mtriple=i686 %s -o - | FileCheck --check-prefixes=CHECK,32 %s
 ; RUN: llc -mtriple=x86_64 %s -o - | FileCheck --check-prefixes=CHECK,64 %s
 
+@_ZTIFvvE = linkonce_odr constant i32 1
+@__llvm_rtti_proxy = private unnamed_addr constant ptr @_ZTIFvvE
+
 ;; -fpatchable-function-entry=0 -fcf-protection=branch
 define void @f0() "patchable-function-entry"="0" {
 ; CHECK-LABEL: f0:
 ; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK-NEXT: .cfi_startproc
 ; CHECK-NEXT: # %bb.0:
 ; 32-NEXT:     endbr32
 ; 64-NEXT:     endbr64

 ; 32-NEXT:    .long .Lfunc_begin3
 ; 64-NEXT:    .p2align 3
 ; 64-NEXT:    .quad .Lfunc_begin3
 entry:
   tail call i32 @llvm.eh.sjlj.setjmp(ptr @buf)
   ret void
 }
 
+;; Test the interaction with -fsanitize=function.
+; CHECK:      .type sanitize_function,@function
+; CHECK-NEXT: .Ltmp{{.*}}:
+; CHECK-NEXT:   nop
+; CHECK-NEXT:   .long   3238382334
+; CHECK-NEXT:   .long   .L__llvm_rtti_proxy-sanitize_function
+; CHECK-NEXT: sanitize_function:
+; CHECK-NEXT: .Lfunc_begin{{.*}}:
+; CHECK-NEXT:   .cfi_startproc
+; CHECK-NEXT:   # %bb.0:
+; 32-NEXT:      endbr32
+; 64-NEXT:      endbr64
+; CHECK-NEXT:   nop
+; CHECK-NEXT:   ret
+define void @sanitize_function(ptr noundef %x) "patchable-function-prefix"="1" "patchable-function-entry"="1" !func_sanitize !1 {
+  ret void
+}
+
 !llvm.module.flags = !{!0}
 
 !0 = !{i32 8, !"cf-protection-branch", i32 1}
+!1 = !{i32 3238382334, ptr @__llvm_rtti_proxy}