Skip to content

[Clang][attr] Add 'kcfi_salt' attribute #141846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

bwendling
Copy link
Collaborator

@bwendling bwendling commented May 28, 2025

The new 'kcfi_salt' attribute is used to differentiate between two functions with the same type signature. It's applied to the function declaration, function definition, and function typedefs of the function to ensure the "salt" is passed through to code generation.

Simple example:

#define __kcfi_salt __attribute__((kcfi_salt("pepper")))

extern int foo(void) __kcfi_salt;
int f1_salt(void) __kcfi_salt { /* ... */ }

Example of using this in a structure:

#define __kcfi_salt __attribute__((kcfi_salt("pepper")))

// Convenient typedefs to avoid nested declarator syntax.
typedef int (*fptr_t)(void);
typedef int (*fptr_salted_t)(void) __kcfi_salt;

struct widget_generator {
  fptr_t init;
  fptr_salted_t exec;
  fptr_t teardown;
};

// 1st .c file:
static int internal_init(void) { /* ... */ }
static int internal_salted_exec(void) __kcfi_salt { /* ... */ }
static int internal_teardown(void) { /* ... */ }

static struct widget_generator _generator = {
  .init = internal_init,
  .exec = internal_salted_exec,
  .teardown = internal_teardown,
}

struct widget_generator *widget_gen = _generator;

// 2nd .c file:
int generate_a_widget(void) {
  int ret;

  // Called with non-salted CFI.
  ret = widget_gen.init();
  if (ret)
    return ret;

  // Called with salted CFI.
  ret = widget_gen.exec();
  if (ret)
    return ret;

  // Called with non-salted CFI.
  return widget_gen.teardown();
}

Link: ClangBuiltLinux/linux#1736
Link: KSPP/linux#365

The new 'cfi_salt' attribute is used to differentiate between two
functions with the same type signature. It's applied to the function
declaration, function definition, and function typedefs of the function
to ensure the "salt" is passed through to code generation.

Simple example:

    #define __cfi_salt __attribute__((cfi_salt("pepper")))

    extern int foo(void) __cfi_salt;
    int f1_salt(void) __cfi_salt { /* ... */ }

Example of using this in a structure:

    #define __cfi_salt __attribute__((cfi_salt("pepper")))

    // Convenient typedefs to avoid nested declarator syntax.
    typedef int (*fptr_t)(void);
    typedef int (*fptr_salted_t)(void) __cfi_salt;

    struct widget_generator {
      fptr_t init;
      fptr_salted_t exec;
      fptr_t teardown;
    };

    // 1st .c file:
    static int internal_init(void) { /* ... */ }
    static int internal_salted_exec(void) __cfi_salt { /* ... */ }
    static int internal_teardown(void) { /* ... */ }

    static struct widget_generator _generator = {
      .init = internal_init,
      .exec = internal_salted_exec,
      .teardown = internal_teardown,
    }

    struct widget_generator *widget_gen = _generator;

    // 2nd .c file:
    int generate_a_widget(void) {
      int ret;

      // Called with non-salted CFI.
      ret = widget_gen.init();
      if (ret)
        return ret;

      // Called with salted CFI.
      ret = widget_gen.exec();
      if (ret)
        return ret;

      // Called with non-salted CFI.
      return widget_gen.teardown();
    }

Link: ClangBuiltLinux/linux#1736
Link: KSPP/linux#365
Signed-off-by: Bill Wendling <morbo@google.com>
@bwendling bwendling requested review from kees and samitolvanen May 28, 2025 20:15
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels May 28, 2025
@llvmbot
Copy link
Member

llvmbot commented May 28, 2025

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Bill Wendling (bwendling)

Changes

The new 'cfi_salt' attribute is used to differentiate between two functions with the same type signature. It's applied to the function declaration, function definition, and function typedefs of the function to ensure the "salt" is passed through to code generation.

Simple example:

#define __cfi_salt __attribute__((cfi_salt("pepper")))

extern int foo(void) __cfi_salt;
int f1_salt(void) __cfi_salt { /* ... */ }

Example of using this in a structure:

#define __cfi_salt __attribute__((cfi_salt("pepper")))

// Convenient typedefs to avoid nested declarator syntax.
typedef int (*fptr_t)(void);
typedef int (*fptr_salted_t)(void) __cfi_salt;

struct widget_generator {
  fptr_t init;
  fptr_salted_t exec;
  fptr_t teardown;
};

// 1st .c file:
static int internal_init(void) { /* ... */ }
static int internal_salted_exec(void) __cfi_salt { /* ... */ }
static int internal_teardown(void) { /* ... */ }

static struct widget_generator _generator = {
  .init = internal_init,
  .exec = internal_salted_exec,
  .teardown = internal_teardown,
}

struct widget_generator *widget_gen = _generator;

// 2nd .c file:
int generate_a_widget(void) {
  int ret;

  // Called with non-salted CFI.
  ret = widget_gen.init();
  if (ret)
    return ret;

  // Called with salted CFI.
  ret = widget_gen.exec();
  if (ret)
    return ret;

  // Called with non-salted CFI.
  return widget_gen.teardown();
}

Link: ClangBuiltLinux/linux#1736
Link: KSPP/linux#365


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

11 Files Affected:

  • (modified) clang/include/clang/Basic/Attr.td (+7)
  • (modified) clang/include/clang/Basic/AttrDocs.td (+58)
  • (modified) clang/lib/AST/TypePrinter.cpp (+3)
  • (modified) clang/lib/CodeGen/CGCall.h (+7)
  • (modified) clang/lib/CodeGen/CGExpr.cpp (+3-2)
  • (modified) clang/lib/CodeGen/CGExprCXX.cpp (+6-8)
  • (modified) clang/lib/CodeGen/CodeGenFunction.cpp (+31-4)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+12-3)
  • (modified) clang/lib/CodeGen/CodeGenModule.h (+1-1)
  • (modified) clang/lib/Sema/SemaDeclAttr.cpp (+27)
  • (added) clang/test/CodeGen/kcfi-salt.c (+190)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index db02449a3dd12..b3a35ec3a16a4 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3830,6 +3830,13 @@ def CFICanonicalJumpTable : InheritableAttr {
   let SimpleHandler = 1;
 }
 
+def CFISalt : DeclOrTypeAttr {
+  let Spellings = [Clang<"cfi_salt">];
+  let Args = [StringArgument<"Salt">];
+  let Subjects = SubjectList<[Function, Field, Var, TypedefName], ErrorDiag>;
+  let Documentation = [CFISaltDocs];
+}
+
 // C/C++ Thread safety attributes (e.g. for deadlock, data race checking)
 // Not all of these attributes will be given a [[]] spelling. The attributes
 // which require access to function parameter names cannot use the [[]] spelling
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 65d66dd398ad1..4c3a488076c54 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -3639,6 +3639,64 @@ make the function's CFI jump table canonical. See :ref:`the CFI documentation
   }];
 }
 
+def CFISaltDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Use ``__attribute__((cfi_salt("<salt>")))`` on a function declaration, function
+definition, or typedef to help distinguish CFI hashes between functions with
+the same type signature.
+
+Example use:
+
+.. code-block:: c
+
+  // .h file:
+  #define __cfi_salt __attribute__((cfi_salt("vogon")))
+
+  // Convenient typedefs to avoid nested declarator syntax.
+  typedef int (*fptr_t)(void); // Non-salted function call.
+  typedef int (*fptr_salted_t)(void) __cfi_salt;
+
+  struct widget_generator {
+    fptr_t init;
+    fptr_salted_t exec;
+    fptr_t teardown;
+  };
+
+  // 1st .c file:
+  static int internal_init(void) { /* ... */ }
+  static int internal_salted_exec(void) __cfi_salt { /* ... */ }
+  static int internal_teardown(void) { /* ... */ }
+
+  static struct widget_generator _generator = {
+    .init = internal_init,
+    .exec = internal_salted_exec,
+    .teardown = internal_teardown,
+  }
+
+  struct widget_generator *widget_gen = _generator;
+
+  // 2nd .c file:
+  int generate_a_widget(void) {
+    int ret;
+
+    // Called with non-salted CFI.
+    ret = widget_gen.init();
+    if (ret)
+      return ret;
+
+    // Called with salted CFI.
+    ret = widget_gen.exec();
+    if (ret)
+      return ret;
+
+    // Called with non-salted CFI.
+    return widget_gen.teardown();
+  }
+
+  }];
+}
+
 def DocCatTypeSafety : DocumentationCategory<"Type Safety Checking"> {
   let Content = [{
 Clang supports additional attributes to enable checking type safety properties
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index 4793ef38c2c46..b241892f3f836 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -2099,6 +2099,9 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
   case attr::ExtVectorType:
     OS << "ext_vector_type";
     break;
+  case attr::CFISalt:
+    OS << "cfi_salt(\"" << cast<CFISaltAttr>(T->getAttr())->getSalt() << "\")";
+    break;
   }
   OS << "))";
 }
diff --git a/clang/lib/CodeGen/CGCall.h b/clang/lib/CodeGen/CGCall.h
index 0b4e3f9cb0365..f54dd64d182e5 100644
--- a/clang/lib/CodeGen/CGCall.h
+++ b/clang/lib/CodeGen/CGCall.h
@@ -96,6 +96,8 @@ class CGCallee {
     VirtualInfoStorage VirtualInfo;
   };
 
+  QualType ExprType;
+
   explicit CGCallee(SpecialKind kind) : KindOrFunctionPointer(kind) {}
 
   CGCallee(const FunctionDecl *builtinDecl, unsigned builtinID)
@@ -221,6 +223,11 @@ class CGCallee {
     return VirtualInfo.FTy;
   }
 
+  /// Retain the full type of the callee before canonicalization. This may have
+  /// attributes important for later processing (e.g. KCFI).
+  QualType getExprType() const { return ExprType; }
+  void setExprType(QualType Ty) { ExprType = Ty; }
+
   /// If this is a delayed callee computation of some sort, prepare
   /// a concrete callee.
   CGCallee prepareConcreteCallee(CodeGenFunction &CGF) const;
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index bae28b45afaa3..3811fa6a875f8 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -6260,12 +6260,13 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType,
           !cast<FunctionDecl>(TargetDecl)->isImmediateFunction()) &&
          "trying to emit a call to an immediate function");
 
+  CGCallee Callee = OrigCallee;
+  Callee.setExprType(CalleeType);
+
   CalleeType = getContext().getCanonicalType(CalleeType);
 
   auto PointeeType = cast<PointerType>(CalleeType)->getPointeeType();
 
-  CGCallee Callee = OrigCallee;
-
   if (SanOpts.has(SanitizerKind::Function) &&
       (!TargetDecl || !isa<FunctionDecl>(TargetDecl)) &&
       !isa<FunctionNoProtoType>(PointeeType)) {
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index 024254b0affe4..471c1b9a58cdb 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -470,16 +470,14 @@ CodeGenFunction::EmitCXXMemberPointerCallExpr(const CXXMemberCallExpr *E,
 
   // Ask the ABI to load the callee.  Note that This is modified.
   llvm::Value *ThisPtrForCall = nullptr;
-  CGCallee Callee =
-    CGM.getCXXABI().EmitLoadOfMemberFunctionPointer(*this, BO, This,
-                                             ThisPtrForCall, MemFnPtr, MPT);
-
-  CallArgList Args;
-
-  QualType ThisType =
-    getContext().getPointerType(getContext().getTagDeclType(RD));
+  CGCallee Callee = CGM.getCXXABI().EmitLoadOfMemberFunctionPointer(
+      *this, BO, This, ThisPtrForCall, MemFnPtr, MPT);
+  Callee.setExprType(MemFnExpr->getType());
 
   // Push the this ptr.
+  QualType ThisType =
+      getContext().getPointerType(getContext().getTagDeclType(RD));
+  CallArgList Args;
   Args.add(RValue::get(ThisPtrForCall), ThisType);
 
   RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, 1);
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index e2357563f7d56..3902b0e0b26c7 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2887,10 +2887,37 @@ void CodeGenFunction::EmitSanitizerStatReport(llvm::SanitizerStatKind SSK) {
 
 void CodeGenFunction::EmitKCFIOperandBundle(
     const CGCallee &Callee, SmallVectorImpl<llvm::OperandBundleDef> &Bundles) {
-  const FunctionProtoType *FP =
-      Callee.getAbstractInfo().getCalleeFunctionProtoType();
-  if (FP)
-    Bundles.emplace_back("kcfi", CGM.CreateKCFITypeId(FP->desugar()));
+  const CGCalleeInfo &CI = Callee.getAbstractInfo();
+  const FunctionProtoType *FP = CI.getCalleeFunctionProtoType();
+  if (!FP)
+    return;
+
+  const AttributedType *AT = nullptr;
+  GlobalDecl GD = CI.getCalleeDecl();
+  StringRef Salt;
+
+  auto GetAttributedType = [](QualType Ty) {
+    if (const auto *ET = dyn_cast<ElaboratedType>(Ty))
+      if (const auto *TT = dyn_cast<TypedefType>(ET->desugar()))
+        return dyn_cast<AttributedType>(TT->desugar());
+
+    return dyn_cast<AttributedType>(Ty);
+  };
+
+  if (GD) {
+    if (const auto *D = dyn_cast<ValueDecl>(GD.getDecl()))
+      AT = GetAttributedType(D->getType());
+  } else {
+    QualType ExprTy = Callee.getExprType();
+    if (!ExprTy.isNull())
+      AT = GetAttributedType(ExprTy);
+  }
+
+  if (AT)
+    if (const auto *CFISalt = dyn_cast<CFISaltAttr>(AT->getAttr()))
+      Salt = CFISalt->getSalt();
+
+  Bundles.emplace_back("kcfi", CGM.CreateKCFITypeId(FP->desugar(), Salt));
 }
 
 llvm::Value *
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 6d2c705338ecf..ad2e4a714deba 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2221,7 +2221,7 @@ llvm::ConstantInt *CodeGenModule::CreateCrossDsoCfiTypeId(llvm::Metadata *MD) {
   return llvm::ConstantInt::get(Int64Ty, llvm::MD5Hash(MDS->getString()));
 }
 
-llvm::ConstantInt *CodeGenModule::CreateKCFITypeId(QualType T) {
+llvm::ConstantInt *CodeGenModule::CreateKCFITypeId(QualType T, StringRef Salt) {
   if (auto *FnType = T->getAs<FunctionProtoType>())
     T = getContext().getFunctionType(
         FnType->getReturnType(), FnType->getParamTypes(),
@@ -2232,6 +2232,9 @@ llvm::ConstantInt *CodeGenModule::CreateKCFITypeId(QualType T) {
   getCXXABI().getMangleContext().mangleCanonicalTypeName(
       T, Out, getCodeGenOpts().SanitizeCfiICallNormalizeIntegers);
 
+  if (!Salt.empty())
+    Out << "." << Salt;
+
   if (getCodeGenOpts().SanitizeCfiICallNormalizeIntegers)
     Out << ".normalized";
 
@@ -2898,9 +2901,15 @@ void CodeGenModule::createFunctionTypeMetadataForIcall(const FunctionDecl *FD,
 void CodeGenModule::setKCFIType(const FunctionDecl *FD, llvm::Function *F) {
   llvm::LLVMContext &Ctx = F->getContext();
   llvm::MDBuilder MDB(Ctx);
+  llvm::StringRef Salt;
+
+  if (const auto *AT = dyn_cast<AttributedType>(FD->getType()))
+    if (const auto *CFISalt = dyn_cast<CFISaltAttr>(AT->getAttr()))
+      Salt = CFISalt->getSalt();
+
   F->setMetadata(llvm::LLVMContext::MD_kcfi_type,
-                 llvm::MDNode::get(
-                     Ctx, MDB.createConstant(CreateKCFITypeId(FD->getType()))));
+                 llvm::MDNode::get(Ctx, MDB.createConstant(CreateKCFITypeId(
+                                            FD->getType(), Salt))));
 }
 
 static bool allowKCFIIdentifier(StringRef Name) {
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index 1db5c3bc4e4ef..5081e1f042546 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -1616,7 +1616,7 @@ class CodeGenModule : public CodeGenTypeCache {
   llvm::ConstantInt *CreateCrossDsoCfiTypeId(llvm::Metadata *MD);
 
   /// Generate a KCFI type identifier for T.
-  llvm::ConstantInt *CreateKCFITypeId(QualType T);
+  llvm::ConstantInt *CreateKCFITypeId(QualType T, StringRef Salt);
 
   /// Create a metadata identifier for the given type. This may either be an
   /// MDString (for external identifiers) or a distinct unnamed MDNode (for
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 54bac40982eda..b7bd1fd48cd0a 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -6671,6 +6671,29 @@ static void handleEnforceTCBAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   D->addAttr(AttrTy::Create(S.Context, Argument, AL));
 }
 
+static void handleCFISaltAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  StringRef Argument;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Argument))
+    return;
+
+  auto *Attr = CFISaltAttr::Create(S.Context, Argument, AL);
+
+  if (auto *DD = dyn_cast<ValueDecl>(D)) {
+    QualType Ty = DD->getType();
+    if (!Ty->getAs<AttributedType>())
+      DD->setType(S.Context.getAttributedType(Attr, Ty, Ty));
+  } else if (auto *TD = dyn_cast<TypedefDecl>(D)) {
+    TypeSourceInfo *TSI = TD->getTypeSourceInfo();
+    QualType Ty = TD->getUnderlyingType();
+
+    if (!Ty->hasAttr(attr::CFISalt))
+      TD->setModedTypeSourceInfo(TSI,
+                                 S.Context.getAttributedType(Attr, Ty, Ty));
+  } else {
+    llvm_unreachable("Unexpected Decl argument type!");
+  }
+}
+
 template <typename AttrTy, typename ConflictingAttrTy>
 static AttrTy *mergeEnforceTCBAttrImpl(Sema &S, Decl *D, const AttrTy &AL) {
   // Check if the new redeclaration has different leaf-ness in the same TCB.
@@ -7473,6 +7496,10 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
     handleCountedByAttrField(S, D, AL);
     break;
 
+  case ParsedAttr::AT_CFISalt:
+    handleCFISaltAttr(S, D, AL);
+    break;
+
   // Microsoft attributes:
   case ParsedAttr::AT_LayoutVersion:
     handleLayoutVersion(S, D, AL);
diff --git a/clang/test/CodeGen/kcfi-salt.c b/clang/test/CodeGen/kcfi-salt.c
new file mode 100644
index 0000000000000..7728e0525b3ee
--- /dev/null
+++ b/clang/test/CodeGen/kcfi-salt.c
@@ -0,0 +1,190 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -x c++ -o - %s | FileCheck %s --check-prefixes=CHECK,MEMBER
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -fpatchable-function-entry-offset=3 -o - %s | FileCheck %s --check-prefixes=CHECK,OFFSET
+
+// Note that the interleving of functions, which normally would be in sequence,
+// is due to the fact that Clang outputs them in a non-sequential order.
+
+#if !__has_feature(kcfi)
+#error Missing kcfi?
+#endif
+
+#define __cfi_salt __attribute__((cfi_salt("pepper")))
+
+typedef int (*fn_t)(void);
+typedef int __cfi_salt (*fn_salt_t)(void);
+
+typedef unsigned int (*ufn_t)(void);
+typedef unsigned int __cfi_salt (*ufn_salt_t)(void);
+
+/// Must emit __kcfi_typeid symbols for address-taken function declarations
+// CHECK: module asm ".weak __kcfi_typeid_[[F4:[a-zA-Z0-9_]+]]"
+// CHECK: module asm ".set __kcfi_typeid_[[F4]], [[#%d,LOW_SODIUM_HASH:]]"
+// CHECK: module asm ".weak __kcfi_typeid_[[F4_SALT:[a-zA-Z0-9_]+]]"
+// CHECK: module asm ".set __kcfi_typeid_[[F4_SALT]], [[#%d,ASM_SALTY_HASH:]]"
+
+/// Must not __kcfi_typeid symbols for non-address-taken declarations
+// CHECK-NOT: module asm ".weak __kcfi_typeid_{{f6|_Z2f6v}}"
+
+int f1(void);
+int f1_salt(void) __cfi_salt;
+
+unsigned int f2(void);
+unsigned int f2_salt(void) __cfi_salt;
+
+static int f3(void);
+static int f3_salt(void) __cfi_salt;
+
+extern int f4(void);
+extern int f4_salt(void) __cfi_salt;
+
+static int f5(void);
+static int f5_salt(void) __cfi_salt;
+
+extern int f6(void);
+extern int f6_salt(void) __cfi_salt;
+
+struct cfi_struct {
+  fn_t __cfi_salt fptr;
+  fn_salt_t td_fptr;
+};
+
+int f7_salt(struct cfi_struct *ptr);
+int f7_typedef_salt(struct cfi_struct *ptr);
+
+// CHECK-LABEL: @{{__call|_Z6__callPFivE}}
+// CHECK:         call{{.*}} i32
+// CHECK-NOT:     "kcfi"
+// CHECK-SAME:    ()
+int __call(fn_t f) __attribute__((__no_sanitize__("kcfi"))) {
+  return f();
+}
+
+// CHECK-LABEL: @{{call|_Z4callPFivE}}
+// CHECK:         call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#LOW_SODIUM_HASH]]) ]
+// CHECK-LABEL: @{{call_salt|_Z9call_saltPFivE}}
+// CHECK:         call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#%d,SALTY_HASH:]]) ]
+// CHECK-LABEL: @{{call_salt_ty|_Z12call_salt_tyPFivE}}
+// CHECK:         call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#SALTY_HASH]]) ]
+int call(fn_t f) { return f(); }
+int call_salt(fn_t __cfi_salt f) { return f(); }
+int call_salt_ty(fn_salt_t f) { return f(); }
+
+// CHECK-LABEL: @{{ucall|_Z5ucallPFjvE}}
+// CHECK:         call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#%d,LOW_SODIUM_UHASH:]]) ]
+// CHECK-LABEL: @{{ucall_salt|_Z10ucall_saltPFjvE}}
+// CHECK:         call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#%d,SALTY_UHASH:]]) ]
+// CHECK-LABEL: @{{ucall_salt_ty|_Z13ucall_salt_tyPFjvE}}
+// CHECK:         call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#SALTY_UHASH]]) ]
+unsigned int ucall(ufn_t f) { return f(); }
+unsigned int ucall_salt(ufn_t __cfi_salt f) { return f(); }
+unsigned int ucall_salt_ty(ufn_salt_t f) { return f(); }
+
+int test1(struct cfi_struct *ptr) {
+  return call(f1) +
+         call_salt(f1_salt) +
+         call_salt_ty(f1_salt) +
+
+         __call((fn_t)f2) +
+         __call((fn_t)f2_salt) +
+
+         ucall(f2) +
+         ucall_salt(f2_salt) +
+         ucall_salt_ty(f2_salt) +
+
+         call(f3) +
+         call_salt(f3_salt) +
+         call_salt_ty(f3_salt) +
+
+         call(f4) +
+         call_salt(f4_salt) +
+         call_salt_ty(f4_salt) +
+
+         f5() +
+         f5_salt() +
+
+         f6() +
+         f6_salt() +
+
+         f7_salt(ptr) +
+         f7_typedef_salt(ptr);
+}
+
+// CHECK-LABEL: define dso_local{{.*}} i32 @{{f1|_Z2f1v}}(){{.*}} !kcfi_type
+// CHECK-SAME:  ![[#LOW_SODIUM_TYPE:]]
+// CHECK-LABEL: define dso_local{{.*}} i32 @{{f1_salt|_Z7f1_saltv}}(){{.*}} !kcfi_type
+// CHECK-SAME:  ![[#SALTY_TYPE:]]
+int f1(void) { return 0; }
+int f1_salt(void) __cfi_salt { return 0; }
+
+// CHECK-LABEL: define dso_local{{.*}} i32 @{{f2|_Z2f2v}}(){{.*}} !kcfi_type
+// CHECK-SAME:  ![[#LOW_SODIUM_UTYPE:]]
+// CHECK: define dso_local{{.*}} i32 @{{f2_salt|_Z7f2_saltv}}(){{.*}} !kcfi_type
+// CHECK-SAME:  ![[#SALTY_UTYPE:]]
+unsigned int f2(void) { return 2; }
+unsigned int f2_salt(void) __cfi_salt { return 2; }
+
+// CHECK-LABEL: define internal{{.*}} i32 @{{f3|_ZL2f3v}}(){{.*}} !kcfi_type
+// CHECK-SAME:  ![[#LOW_SODIUM_TYPE]]
+// CHECK-LABEL: define internal{{.*}} i32 @{{f3_salt|_ZL7f3_saltv}}(){{.*}} !kcfi_type
+// CHECK-SAME:  ![[#SALTY_TYPE]]
+static int f3(void) { return 1; }
+static int f3_salt(void) __cfi_salt { return 1; }
+
+// CHECK: declare !kcfi_type ![[#LOW_SODIUM_TYPE]]{{.*}} i32 @[[F4]]()
+// CHECK: declare !kcfi_type ![[#SALTY_TYPE]]{{.*}} i32 @[[F4_SALT]]()
+
+/// Must not emit !kcfi_type for non-address-taken local functions
+// CHECK-LABEL: define internal{{.*}} i32 @{{f5|_ZL2f5v}}()
+// CHECK-NOT:   !kcfi_type
+// CHECK-SAME:  {
+// CHECK-LABEL: define internal{{.*}} i32 @{{f5_salt|_ZL7f5_saltv}}()
+// CHECK-NOT:   !kcfi_type
+// CHECK-SAME:  {
+static int f5(void) { return 2; }
+static int f5_salt(void) __cfi_salt { return 2; }
+
+// CHECK: declare !kcfi_type ![[#LOW_SODIUM_TYPE]]{{.*}} i32 @{{f6|_Z2f6v}}()
+// CHECK: declare !kcfi_type ![[#SALTY_TYPE]]{{.*}} i32 @{{f6_salt|_Z7f6_saltv}}()
+
+// CHECK-LABEL: @{{f7_salt|_Z7f7_saltP10cfi_struct}}
+// CHECK:         call{{.*}} i32 %{{.*}}() [ "kcfi"(i32 [[#SALTY_HASH]]) ]
+// CHECK-LABEL: @{{f7_typedef_salt|_Z15f7_typedef_saltP10cfi_struct}}
+// CHECK:         call{{.*}} i32 %{{.*}}() [ "kcfi"(i32 [[#SALTY_HASH]]) ]
+int f7_salt(struct cfi_struct *ptr) { return ptr->fptr(); }
+int f7_typedef_salt(struct cfi_struct *ptr) { return ptr->td_fptr(); }
+
+#ifdef __cplusplus
+// MEMBER-LABEL: define dso_local void @_Z16test_member_callv() #0 !kcfi_type
+// MEMBER:         call void %[[#]](ptr{{.*}} [ "kcfi"(i32 [[#%d,MEMBER_LOW_SODIUM_HASH:]]) ]
+// MEMBER:         call void %[[#]](ptr{{.*}} [ "kcfi"(i32 [[#%d,MEMBER_SALT_HASH:]]) ]
+
+// MEMBER-LABEL: define{{.*}} void @_ZN1A1fEv(ptr{{.*}} %this){{.*}} !kcfi_type
+// MEMBER-SAME:  [[#MEMBER_LOW_SODIUM_TYPE:]]
+// MEMBER-LABEL: define{{.*}} void @_ZN1A1gEv(ptr{{.*}} %this){{.*}} !kcfi_type
+// MEMBER-SAME:  [[#MEMBER_SALT_TYPE:]]
+struct A {
+  void f() {}
+  void __cfi_salt g() {}
+};
+
+void test_member_call(void) {
+  void (A::* p)() = &A::f;
+  (A().*p)();
+
+  void __cfi_salt (A::* q)() = &A::g;
+  (A().*q)();
+}
+#endif
+
+// CHECK:  ![[#]] = !{i32 4, !"kcfi", i32 1}
+// OFFSET: ![[#]] = !{i32 4, !"kcfi-offset", i32 3}
+//
+// CHECK:  ![[#LOW_SODIUM_TYPE]] = !{i32 [[#LOW_SODIUM_HASH]]}
+// CHECK:  ![[#SALTY_TYPE]] = !{i32 [[#SALTY_HASH]]}
+//
+// CHECK:  ![[#LOW_SODIUM_UTYPE]] = !{i32 [[#LOW_SODIUM_UHASH]]}
+// CHECK:  ![[#SALTY_UTYPE]] = !{i32 [[#SALTY_UHASH]]}
+//
+// MEMBER: ![[#MEMBER_LOW_SODIUM_TYPE]] = !{i32 [[#MEMBER_LOW_SODIUM_HASH]]}
+// MEMBER: ![[#MEMBER_SALT_TYPE]] = !{i32 [[#MEMBER_SALT_HASH]]}

@efriedma-quic
Copy link
Collaborator

See #135836 for discussion of why you can't use AttributedType like this. CC @PiJoules

@bwendling
Copy link
Collaborator Author

bwendling commented May 28, 2025

@efriedma-quic Okay, I'm not entirely sure I understand @erichkeane's concerns, but that's fine. Are you suggesting adding the salt information to the FunctionType instead? I avoided doing that because I didn't want to increase the size of FunctionType for an attribute.

@efriedma-quic
Copy link
Collaborator

Yes, it needs to be attached to the FunctionType because otherwise you run into weird issues where two types with the same canonical type have different semantics. (In C, the primary issue is that the types are compatible for type merging even though they aren't the same. In C++ you run into issues with template instantiation.)

If you need to store an arbitrary string, we might want to reconsider how we store some of the data in FunctionType. Maybe some sort of key-value array, instead of a bunch of bitfields. Might need to do some benchmarking to see what works best.

@bwendling
Copy link
Collaborator Author

I'll need to store a StringRef, which is 16 bytes. I could potentially add that into the FunctionTypeExtraBitfields object, which currently has only 2 bytes. The name Bitfields is a bit of a misnomer in that sense. Another option is to do what was done with Arm (FunctionTypeArmAttributes) but for CFI (FunctionTypeCFIAttributes?). Doing that limits the size impact to only those functions that have the cfi_salt attribute.

Do you have any preference?

@efriedma-quic
Copy link
Collaborator

Something along the lines of FunctionTypeArmAttributes seems fine.

@bwendling
Copy link
Collaborator Author

I moved the cfi_salt into the FunctionProtoType fields. I left it open for further additions. One bit of optimization I could do is merge the Arm stuff into this new struct, but wanted to do something a bit less cluttered first.

@erichkeane erichkeane requested a review from AaronBallman June 5, 2025 13:30
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing seems insufficient, particularly around conversion rules/etc of these types. I would like to see quite a bit more for Sema/inside of templates/how this applies to instantiations of all these things, etc.

Also, this is ANOTHER bit on function types that we're using for CFI. I'm wondering if this is actually a generally useful enough feature to be using this much compiler resources.

@AaronBallman ?

@bwendling
Copy link
Collaborator Author

@erichkeane If the Arm attribute code were merged into ExtraAttributeInfo, it would save us a bit in FunctionTypeExtraBitfields.

I can add more testing I suppose, but the tests I have here are a copy of the current CFI tests, which test all of the ways CFI is used. So I'm not sure how much more needs to be done... The important thing that's tested here is that the CFI code changes in a deterministic way when the "salt" is added. It's not a test for all of CFI.

@AaronBallman
Copy link
Collaborator

Also, this is ANOTHER bit on function types that we're using for CFI. I'm wondering if this is actually a generally useful enough feature to be using this much compiler resources.

@AaronBallman ?

I think CFI is pretty important for security posture (it basically tries to ensure that indirect jumps don't go to unintended locations). So I think it's useful, but I am also a bit worried that we're missing the big picture of how much work needs to be done to fully support CFI. @bwendling do you expect to land more related attributes after this one?

@bwendling
Copy link
Collaborator Author

@AaronBallman I know of no other CFI attribute requests. (But keep in mind that I'm not the CFI expert I play on TV.)

@AaronBallman
Copy link
Collaborator

@AaronBallman I know of no other CFI attribute requests.

Excellent, glad to hear there's not some massive set of changes waiting in the wings.

(But keep in mind that I'm not the CFI expert I play on TV.)

Sorry, but you touched it last, so you own it. I don't make the rules. ;-) (I kid, I kid)

// Note that the interleving of functions, which normally would be in sequence,
// is due to the fact that Clang outputs them in a non-sequential order.

#if !__has_feature(kcfi)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need Sema tests for all the semantic diagnostics we'd expect to get from this. That should include things like writing the attribute on something other than a function or a typedef, not passing any arguments or passing too many arguments, stuff I called out in the documentation as open questions, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, because this is a DeclOrTypeAttr we should also have sema tests for type mismatches, e.g.:

int func(void);
int (*fp)(void) [[clang::kcfi_salt("pepper")]] = func; // Is this okay?

int func2(void) [[clang::kcfi_salt("pepper")]];
int (*fp2)(void) = func2; // Is this okay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prototype mismatch at assignment type is a really good question. I hadn't considered this at all, but yes, it would really be a nice addition to catch misuse at build time. Is it complex to add?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how complex it would be to add checks for this. We would have to check both initializations and assignments, which could get messy. Then we would need to deal with things like:

// The user explicitly casts 'func', indicating that they know what they're doing
// (obvious lies). Do we accept it?
int func(void);
int (*fp)(void) __kcfi_salt("pepper") =
    (int (*)(void) __kcfi_salt("pepper")) func;

There are a lot of corner cases that are popping up in review, which indicates to me that there are going to be a lot more coming. Are we sure this attribute it worth it?

Copy link
Member

@samitolvanen samitolvanen Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of corner cases that are popping up in review, which indicates to me that there are going to be a lot more coming. Are we sure this attribute it worth it?

Looking at the KCFI type hashes in an x86_64 defconfig vmlinux.o, we have 38 different function types that each have >100 indirectly callable functions. The five most popular function types each have >1k possible indirect call targets:

      1    1662 0xA540670C = void (*)(void)
      2    1179 0x36B1C5A6 = int (*)(void)
      3    1067 0x6F4FC011 = enum print_line_t (*)(struct trace_iterator *, int, struct trace_event *)
      4    1030 0xDF43C25C = ssize_t (*)(struct device *, struct device_attribute *, char *)
      5    1030 0xB02B34D9 = long (*)(const struct pt_regs *)
                ...
     38     104 0xF318988E

So yes, I think it would be worthwhile to have a way to better differentiate between these functions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you work out what a few of those are? The attribute only helps if we can actually partition them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you work out what a few of those are? The attribute only helps if we can actually partition them.

I updated the previous comment to include the most popular types. The first two we should be able to partition better. I'm less sure about trace functions, sysfs callbacks, or syscall stubs though. Note that I only looked at the available call targets, it's possible that there are no actual indirect call sites for some types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how complex it would be to add checks for this.

I think you'd mostly be updating ASTContext::mergeFunctionTypes(); that's where we check things like calling convention mismatches, noreturn mismatches, and other type properties of a function.

I think we also need some logic to handle declaration merging for things like:

typedef int foo;
typedef int foo __attribute__((kcfi_salt("pepper"))); // Should this be allowed on a redeclaration?
typedef int foo __attribute__((kcfi_salt("spice"))); // Should be rejected, right?

(around Sema::MergeTypedefNameDecl())) and similar for tentative variable declarations (around Sema::MergeVarDecl() probably)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typedef int (*foo)(void);
typedef int (*foo)(void) __attribute__((kcfi_salt("pepper"))); // Should this be allowed on a redeclaration?
typedef int (*foo)(void) __attribute__((kcfi_salt("spice"))); // Should be rejected, right?

These are rejected, even if the first one is removed. If the salt strings are the same, then it's accepted. I believe that's the correct behavior. I'll add tests for it.

Copy link

github-actions bot commented Jul 8, 2025

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

@bwendling bwendling changed the title [Clang][attr] Add cfi_salt attribute [Clang][attr] Add 'kcfi_salt' attribute Jul 9, 2025
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the functional changes are looking reasonable. It's still missing all of the sema tests though, as well as a release note.

// Note that the interleving of functions, which normally would be in sequence,
// is due to the fact that Clang outputs them in a non-sequential order.

#if !__has_feature(kcfi)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, because this is a DeclOrTypeAttr we should also have sema tests for type mismatches, e.g.:

int func(void);
int (*fp)(void) [[clang::kcfi_salt("pepper")]] = func; // Is this okay?

int func2(void) [[clang::kcfi_salt("pepper")]];
int (*fp2)(void) = func2; // Is this okay?

Copy link
Contributor

@kees kees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the docs look good, and other than what Aaron noted for needed Sema tests, the other tests look like what I'd expect.

I've run this on an updated Linux kernel with added tests for using kcfi_salt and CFI correctly trips.

Comment on lines 3649 to 3651
Use ``__attribute__((kcfi_salt("<salt>")))`` on a function declaration, function
definition, or typedef to help distinguish CFI hashes between functions with
the same type signature.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in the kernel patch, it would be nice to summarize the use case, i.e. in what cases we would want to apply this.

@andyhhp
Copy link

andyhhp commented Jul 17, 2025

Does it really need to be spelt kcfi_salt ?

Another use-case is simply for static analysis (the Eclair folks are interested in this capability too), so the utility of this attribute really does go beyond just KCFI.

@pcc
Copy link
Contributor

pcc commented Jul 17, 2025

This could also be used with PAuth ABI or LTO-based CFI. I guess the name isn't that important because we can always document kcfi_salt as being compatible with the other CFI schemes if that ever gets implemented, but it would be slightly confusing.

@bwendling
Copy link
Collaborator Author

Does it really need to be spelt kcfi_salt ?

Another use-case is simply for static analysis (the Eclair folks are interested in this capability too), so the utility of this attribute really does go beyond just KCFI.

We could change it back to cfi_salt. But beware that this is now a C-only feature.

Comment on lines +11 to +12
#if 0
// FIXME: These should fail.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to fix that as part of this PR, or is this for follow-up work?

int b(void) __cfi_salt("salt 'n") __cfi_salt("pepper");
bar_t bar_fn __cfi_salt("salt 'n");
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more tests I'd like to see:

void func(int a) __cfi_salt("pepper");
void func(int a) { } // Okay, inherits the attribute from the declaration.

void blah() __attribute__((cfi_salt)); // Error, missing argument to the attribute
void blarg() __attribute__((cfi_salt(5))); // Error, argument to the attribute is not a string

void okay() [[clang::cfi_salt("test")]]; // Uses the [[]] syntax which appertains to the function type
void (*fp)() = okay; // Is this okay to drop the type attribute? Or is this an error?

int f7_salt(struct cfi_struct *ptr) { return ptr->fptr(); }
int f7_typedef_salt(struct cfi_struct *ptr) { return ptr->td_fptr(); }

#ifdef __cplusplus
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is never run in C++ mode and the attribute is now C-only, so I think this block can be removed.

Comment on lines +3932 to +3935
def CFISalt : DeclOrTypeAttr {
let Spellings = [Clang<"cfi_salt">];
let Args = [StringArgument<"Salt">];
let Subjects = SubjectList<[Function, Field, Var, TypedefName], ErrorDiag>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for missing this earlier, but should this be a TypeAttr and only apply to function types? I don't see any documentation or test cases for it being applied to a field, var, or typedef name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants