[PAuth] Use different discriminators for __int128_t / __uint128_t / _BitInt(n)

[asl]

compared to other integer types when computing function pointer type discriminator.

These parameter types have different parameter passing ABI as compared to ordinary integer types (e.g. require 2 registers instead of 1) and therefore this parameter passing difference could potentially be exploited should function pointer is substituted.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Anton Korobeynikov (asl)

Changes

compared to other integer types when computing function pointer type discriminator.

These parameter types have different parameter passing ABI as compared to ordinary integer types (e.g. require 2 registers instead of 1) and therefore this parameter passing difference could potentially be exploited should function pointer is substituted.

---

Full diff: https://github.com/llvm/llvm-project/pull/140276.diff

2 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+10-4)
• (modified) clang/test/CodeGen/ptrauth-function-type-discriminator.c (+31)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index c58cd2c93fb60..bbaaff6bcda4e 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -3383,21 +3383,27 @@ static void encodeTypeForFunctionPointerAuth(const ASTContext &Ctx,
 
   // Don't bother discriminating based on these types.
   case Type::Pipe:
-  case Type::BitInt:
   case Type::ConstantMatrix:
     OS << "?";
     return;
 
+  case Type::BitInt: {
+    const auto *BtTy = T->castAs<BitIntType>();
+    OS << "D" << (BtTy->isUnsigned() ? "U" : "B") << BtTy->getNumBits() << "_";
+    return;
+  }
+
   case Type::Builtin: {
     const auto *BTy = T->castAs<BuiltinType>();
-    switch (BTy->getKind()) {
+    const auto Kind = BTy->getKind();
+    switch (Kind) {
 #define SIGNED_TYPE(Id, SingletonId)                                           \
   case BuiltinType::Id:                                                        \
-    OS << "i";                                                                 \
+    OS << (Kind == BuiltinType::Int128 ? "n" : "i");                           \
     return;
 #define UNSIGNED_TYPE(Id, SingletonId)                                         \
   case BuiltinType::Id:                                                        \
-    OS << "i";                                                                 \
+    OS << (Kind == BuiltinType::UInt128 ? "o" : "i");                          \
     return;
 #define PLACEHOLDER_TYPE(Id, SingletonId) case BuiltinType::Id:
 #define BUILTIN_TYPE(Id, SingletonId)
diff --git a/clang/test/CodeGen/ptrauth-function-type-discriminator.c b/clang/test/CodeGen/ptrauth-function-type-discriminator.c
index 0952c1abf6c07..9bf4a8874c3c3 100644
--- a/clang/test/CodeGen/ptrauth-function-type-discriminator.c
+++ b/clang/test/CodeGen/ptrauth-function-type-discriminator.c
@@ -65,6 +65,37 @@ void (*fptr3)(void) = __builtin_ptrauth_sign_constant(&external_function, 2, 26)
 // CHECK: @fptr4 = global ptr ptrauth (ptr @external_function, i32 2, i64 26, ptr @fptr4)
 void (*fptr4)(void) = __builtin_ptrauth_sign_constant(&external_function, 2, __builtin_ptrauth_blend_discriminator(&fptr4, 26));
 
+extern void external_function_int(int);
+extern void external_function_char(char);
+extern void external_function_i128(__int128_t);
+extern void external_function_u128(__uint128_t);
+extern void external_function_b128(_BitInt(128));
+extern void external_function_b8(_BitInt(8));
+
+// Check discriminators of functions taking integer type arguments:
+
+//  - Builtin integer types should be discriminated equally (so, pointer to
+//  function taking int argument should accept function taking char argument
+//  - _BitInt types are guaranteed distinct and therefore should be discriminated
+//  differently
+//  - __int128_t / __uint128_t are passed differently than char / int / long
+//  (require two registers instead of one) and therefore should be discriminated
+//  differently.
+
+// CHECK: @fptr5 = global ptr ptrauth (ptr @external_function_int, i32 0, i64 2712)
+// CHECK: @fptr6 = global ptr ptrauth (ptr @external_function_char, i32 0, i64 2712)
+void (*fptr5)(int) = external_function_int;
+void (*fptr6)(char) = external_function_char;
+
+// CHECK: @fptr7 = global ptr ptrauth (ptr @external_function_i128, i32 0, i64 23141)
+// CHECK: @fptr8 = global ptr ptrauth (ptr @external_function_u128, i32 0, i64 45743)
+// CHECK: @fptr9 = global ptr ptrauth (ptr @external_function_b128, i32 0, i64 17854)
+// CHECK: @fptr10 = global ptr ptrauth (ptr @external_function_b8, i32 0, i64 26383)
+void (*fptr7)(__int128_t) = external_function_i128;
+void (*fptr8)(__uint128_t) = external_function_u128;
+void (*fptr9)(_BitInt(128)) = external_function_b128;
+void (*fptr10)(_BitInt(8)) = external_function_b8;
+
 // CHECK-LABEL: define{{.*}} void @test_call()
 void test_call() {
   // CHECK:      [[T0:%.*]] = load ptr, ptr @fnptr,

[ojhunt]

This looks reasonable to me, but are we able to add a flag on it - we might need to be able to disable it on Darwin as it's technically an ABI change

[ojhunt]

@ahatanak I've discussed this with @asl in discord, I think this is a sensible change my only real concern is the ABI, even though it would in principle require the effected types being used in an ABI exposed way to matter

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ojhunt]

This basically looks good to me, sans the "can this just be on by default for elf/linux/etc" question

[ojhunt]

there's apple kernel flags in this function, but it looks like darwin args are actually set up in Toolchains/Darwin.cpp - can the apple invocation paths hit this? if not I think it's worth just having this as an opt out rather than an opt in. We might be constrained by ABI, but I don't think you have any reason to restrict this.

My request for a cli/langopt was so that we can stick with our existing ABI if need be, not because I thought you couldn't do this :D

[ojhunt]

@asl ping ^