-fsanitize=function: fix alignment fault on Arm targets.

[simon_tatham]

Summary

Function pointers are checked by loading a prefix structure from just
before the function's entry point. However, on Arm, the function
pointer is not always exactly equal to the address of the entry point,
because Thumb function pointers have the low bit set to tell the BX
instruction to enter them in Thumb state. So the generated code loads
from an odd address and suffers an alignment fault.

Fixed by clearing the low bit of the function pointer before
subtracting 8.

[simon_tatham]

I think this began going wrong as a result of D148573, which enabled -fsanitize=function on all targets, where previously it hadn't been running on Arm at all. But I'd rather make it work than turn it off again!

[simon_tatham]

(oops, forgot to clang-format)

[simon_tatham]

How embarrassing. _Really_ upload the clang-formatted version this time. Sorry for the noise.

[peter.smith]

This looks good to me. Will be worth waiting for a day to give the US time zone time to leave any comments.

I note that this is also broken in -fsanitize=kcfi [*] (https://reviews.llvm.org/D135411) although fixing that is a separate patch. Would you be able to raise a github issue to cover that?

As an end-to-end example for:

typedef int Fptr(void);

// pf could be Arm (bit 0 clear) or Thumb (bit 0 set)
int f(Fptr* pf) {
  return pf();
}

This generates:

f:
        .fnstart
@ %bb.0:                                @ %entry
        push    {r4, lr}
        mov     r3, r0
        bic     r0, r0, #1
        movw    r2, #51966
        ldr     r1, [r0, #-8]
        movt    r2, #49413
        cmp     r1, r2
        bne     .LBB0_2
@ %bb.1:                                @ %typecheck
        ldr     r0, [r0, #-4]
        movw    r1, #50598
        movt    r1, #14001
        cmp     r0, r1
        bne     .LBB0_3
.LBB0_2:                                @ %cont1
        pop.w   {r4, lr}
        bx      r3

Which gets the address of the signature and type correct, while preserving the thumb bit on the register used for the indirect branch.

-fsanitize=kcfi output is not correct for a Thumb destination:

f:
        .fnstart
        // r0 will have thumb bit set if destination thumb
        ldr     r1, [r0, #-4]
        movw    r2, #50598
        movt    r2, #14001
        cmp     r1, r2
        bne     .LBB0_2
        bx      r0
.LBB0_2:
        .inst   0xe7ffdefe

[michaelplatings]

clang/lib/CodeGen/CGExpr.cpp

5380–5381

I think this line could be more readable. I suggest defining Mask separately and using ~1 instead of -2

[simon_tatham]

Clarify mask construction as @michaelplatings suggested.

[michaelplatings]

Thanks!

[MaskRay]

In D151308#4367704, @peter.smith wrote:

This looks good to me. Will be worth waiting for a day to give the US time zone time to leave any comments.

Thanks!

I note that this is also broken in -fsanitize=kcfi [*] (https://reviews.llvm.org/D135411) although fixing that is a separate patch. Would you be able to raise a github issue to cover that?

-fsanitize=kcfi only supports aarch64 and x86-64 now. riscv64 is on the plan.

% fclang -fsanitize=kcfi --traget=armv7-linux-gnueabi -c a.c
clang: error: unsupported option '--traget=armv7-linux-gnueabi'

clang/lib/CodeGen/CGExpr.cpp

5381

For chained operations, it may be cleaner to remove some used-once variables.

[MaskRay]

LGTM.

clang/test/CodeGen/ubsan-function.cpp

2

Consider using --check-prefixes=CHECK,64 for 64-bit targets and --check-prefixes=CHECK,ARM (or ,32) for the // CHECK: call void @__ubsan_handle_function_type_mismatch_abort(ptr @[[#]], {{i64|i32}} %[[#]]) #[[#]], !nosanitize line below.

[simon_tatham]

-fsanitize=kcfi only supports aarch64 and x86-64 now. riscv64 is on the plan.

% fclang -fsanitize=kcfi --traget=armv7-linux-gnueabi -c a.c
clang: error: unsupported option '--traget=armv7-linux-gnueabi'

Sorry to contradict, but that error message only indicates that you misspelled --target! With Peter's test source file, these two commands generate different object files, and as Peter says, the -fsanitize=kcfi one includes a load from offset −4 relative to a potentially odd-valued function pointer:

clang -O1                 --target=armv7-linux-gnueabi -c a.c   # generates a bare BX r0
clang -O1 -fsanitize=kcfi --target=armv7-linux-gnueabi -c a.c   # generates the code shown in Peter's example above

[peter.smith]

In D151308#4369828, @MaskRay wrote:

In D151308#4367704, @peter.smith wrote:

This looks good to me. Will be worth waiting for a day to give the US time zone time to leave any comments.

Thanks!

I note that this is also broken in -fsanitize=kcfi [*] (https://reviews.llvm.org/D135411) although fixing that is a separate patch. Would you be able to raise a github issue to cover that?

-fsanitize=kcfi only supports aarch64 and x86-64 now. riscv64 is on the plan.

% fclang -fsanitize=kcfi --traget=armv7-linux-gnueabi -c a.c
clang: error: unsupported option '--traget=armv7-linux-gnueabi'

IIUC initially kcfi was x86_64 and AArch64 only. In D135411 "generic" support was added for all targets, quoting from the description.

The KCFI sanitizer emits "kcfi" operand bundles to indirect
call instructions, which the LLVM back-end lowers into an
architecture-specific type check with a known machine instruction
sequence. Currently, KCFI operand bundle lowering is supported only
on 64-bit X86 and AArch64 architectures.

As a lightweight forward-edge CFI implementation that doesn't
require LTO is also useful for non-Linux low-level targets on
other machine architectures, add a generic KCFI operand bundle
lowering pass that's only used when back-end lowering support is not
available and allows -fsanitize=kcfi to be enabled in Clang on all
architectures.

[simon_tatham]

In D151308#4367704, @peter.smith wrote:

I note that this is also broken in -fsanitize=kcfi [*] (https://reviews.llvm.org/D135411) although fixing that is a separate patch. Would you be able to raise a github issue to cover that?

OK, raised https://github.com/llvm/llvm-project/issues/62936 .

clang/lib/CodeGen/CGExpr.cpp

if (llvm::Constant *PrefixSig =
       auto *TypeHash = getUBSanFunctionTypeHash(PointeeType);
 
       llvm::Type *PrefixSigType = PrefixSig->getType();
       llvm::StructType *PrefixStructTy = llvm::StructType::get(
           CGM.getLLVMContext(), {PrefixSigType, Int32Ty}, /*isPacked=*/true);
 
       llvm::Value *CalleePtr = Callee.getFunctionPointer();
 
+      // On 32-bit Arm, the low bit of a function pointer indicates whether
+      // it's using the Arm or Thumb instruction set. The actual first
+      // instruction lives at the same address either way, so we must clear
+      // that low bit before using the function address to find the prefix
+      // structure.
+      //
+      // This applies to both Arm and Thumb target triples, because
+      // either one could be used in an interworking context where it
+      // might be passed function pointers of both types.
+      llvm::Value *AlignedCalleePtr;
+      if (CGM.getTriple().isARM() || CGM.getTriple().isThumb()) {
+        llvm::Value *CalleeAddress =
+            Builder.CreatePtrToInt(CalleePtr, IntPtrTy);
+        llvm::Value *Mask = llvm::ConstantInt::get(IntPtrTy, ~1);
+        llvm::Value *AlignedCalleeAddress =
+            Builder.CreateAnd(CalleeAddress, Mask);
+        AlignedCalleePtr =
+            Builder.CreateIntToPtr(AlignedCalleeAddress, CalleePtr->getType());
+      } else {
+        AlignedCalleePtr = CalleePtr;
+      }
+
       llvm::Value *CalleePrefixStruct = Builder.CreateBitCast(
-          CalleePtr, llvm::PointerType::getUnqual(PrefixStructTy));
+          AlignedCalleePtr, llvm::PointerType::getUnqual(PrefixStructTy));
       llvm::Value *CalleeSigPtr =
           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");

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
-// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s
-// RUN: %clang_cc1 -triple aarch64_be-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,64
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,64
+// RUN: %clang_cc1 -triple aarch64_be-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,64
+// RUN: %clang_cc1 -triple arm-none-eabi -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,ARM,32
 
 // CHECK: define{{.*}} void @_Z3funv() #0 !func_sanitize ![[FUNCSAN:.*]] {
 void fun() {}
 
 // CHECK-LABEL: define{{.*}} void @_Z6callerPFvvE(ptr noundef %f)
+// ARM:   ptrtoint ptr {{.*}} to i32, !nosanitize !5
+// ARM:   and i32 {{.*}}, -2, !nosanitize !5
+// ARM:   inttoptr i32 {{.*}} to ptr, !nosanitize !5
 // CHECK: getelementptr <{ i32, i32 }>, ptr {{.*}}, i32 -1, i32 0, !nosanitize
 // CHECK: load i32, ptr {{.*}}, align {{.*}}, !nosanitize
 // CHECK: icmp eq i32 {{.*}}, -1056584962, !nosanitize
 // CHECK: br i1 {{.*}}, label %[[LABEL1:.*]], label %[[LABEL4:.*]], !nosanitize
 // CHECK: [[LABEL1]]:
 // CHECK: getelementptr <{ i32, i32 }>, ptr {{.*}}, i32 -1, i32 1, !nosanitize
 // CHECK: load i32, ptr {{.*}}, align {{.*}}, !nosanitize
 // CHECK: icmp eq i32 {{.*}}, -1522505972, !nosanitize
 // CHECK: br i1 {{.*}}, label %[[LABEL3:.*]], label %[[LABEL2:[^,]*]], {{.*}}!nosanitize
 // CHECK: [[LABEL2]]:
-// CHECK: call void @__ubsan_handle_function_type_mismatch_abort(ptr @[[#]], i64 %[[#]]) #[[#]], !nosanitize
+// 64:    call void @__ubsan_handle_function_type_mismatch_abort(ptr @[[#]], i64 %[[#]]) #[[#]], !nosanitize
+// 32:    call void @__ubsan_handle_function_type_mismatch_abort(ptr @[[#]], i32 %[[#]]) #[[#]], !nosanitize
 // CHECK-NEXT: unreachable, !nosanitize
 // CHECK-EMPTY:
 // CHECK-NEXT: [[LABEL3]]:
 // CHECK: br label %[[LABEL4]], !nosanitize
 // CHECK-EMPTY:
 // CHECK-NEXT: [[LABEL4]]:
 // CHECK-NEXT:   call void
 // CHECK-NEXT:   ret void
 void caller(void (*f)()) { f(); }
 
 // CHECK: ![[FUNCSAN]] = !{i32 -1056584962, i32 -1522505972}