[Clang][attr] Add cfi_salt attribute #141846
Conversation
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>
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Bill Wendling (bwendling) ChangesThe 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: Example of using this in a structure: Link: ClangBuiltLinux/linux#1736 Full diff: https://github.com/llvm/llvm-project/pull/141846.diff 11 Files Affected:
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 Okay, I'm not entirely sure I understand @erichkeane's concerns, but that's fine. Are you suggesting adding the salt information to the |
|
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. |
|
I'll need to store a Do you have any preference? |
|
Something along the lines of FunctionTypeArmAttributes seems fine. |
…FunctionProtoType.
|
I moved the |
There was a problem hiding this comment.
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.
|
@erichkeane If the Arm attribute code were merged into 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. |
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? |
|
@AaronBallman I know of no other CFI attribute requests. (But keep in mind that I'm not the CFI expert I play on TV.) |
Excellent, glad to hear there's not some massive set of changes waiting in the wings.
Sorry, but you touched it last, so you own it. I don't make the rules. ;-) (I kid, I kid) |
| def CFISaltDocs : Documentation { | ||
| let Category = DocCatFunction; | ||
| let Content = [{ | ||
| Use ``__attribute__((cfi_salt("<salt>")))`` on a function declaration, function |
There was a problem hiding this comment.
I think the docs are missing some information about whether a salted function type is compatible with a non-salted one. e.g.,
fptr_t foo = internal_salted_exec; // Salted RHS, unsalted LHS?
fptr_salted_t bar = internal_init; // Unsalted RHS, salted LHS?
Also, does this form a new function type? e.g.,
void func(int) __attribute__((cfi_salt("meow")));
void func(int); // Overload? Ill-formed redefinition?
Also, can salt be deduced? e.g.,
template <typename Ty>
void func(Ty foo);
void bar() __attribute__((cfi_salt("woof")));
void baz() {
func(bar); // Is Ty 'void(void)' or 'void(void) __attribute__((cfi_salt("woof")))'?
}
Since we're talking about C++, how do virtual functions work? e.g.,
struct S {
virtual void foo() __attribute__((cfi_salt("woof")));
virtual void bar() __attribute__((cfi_salt("woof")));
};
struct T : S {
virtual void foo(); // Valid override, or does this hide the base class function?
virtual void bar() __attribute__((cfi_salt("chirp"))); // Valid override, or does this hide the base class function?
}
Do we have to worry about checking salt consistency when deserializing the AST to load modules/PCH files? (Maybe this is handled more generically for all attributes and nothing needs to be done here? CC @ChuanqiXu9 @Bigcheese for more expertise)
Are there any limitations on the string argument? e.g., is it fine to use "" and that's technically salted? Any issues with embedded null characters or the like?
Should salt impact name mangling? e.g.,
// TU1.cpp
extern void func() __attribute__((cfi_salt("neigh")));
// TU2.cpp
void func() {}
// Should we get link errors?
Btw, these all make for some good test cases we should add.
There was a problem hiding this comment.
All good questions. I'm going to see if the attribute could be restricted to C, as I don't think there's a good way to handle any of the issues with virtual functions you mention here.
As for modules/PCH files, those are C++ features, correct? I literally have no experience with them.
Re name mangling: Probably not, as that would be a departure from other attributes, which typically don't interact with name mangling. The example you gave in C would result in a runtime error. Not ideal, but possibly the correct behavior.
There was a problem hiding this comment.
I'm going to see if the attribute could be restricted to C
If you do restrict to C, then that should be all of the CFI attributes which are restricted to C? If so, we should definitely move on that before Clang 21 is released.
As for modules/PCH files, those are C++ features, correct? I literally have no experience with them.
Modules are a C++ feature, but PCH files are both C and C++ (precompiled headers). Basically, when you include a PCH file it's the same as including a textual header but instead you're including a serialized binary blob representing the textual header, so you can run into the same situations with invalid redeclarations. But I think that's handled in a sufficiently generic way that you should be fine without any custom logic.
Re name mangling:
This goes back to whether it's impacting the function type. Generally that's only an issue in C++, but we have the overloadable attribute which allows for overloading in C. And the way that's implemented is that it basically uses the C++ logic, including name mangling. So we may have to care about it, but if the salt attribute is not part of the function type, then there's no overloads and no need to worry.
There was a problem hiding this comment.
I'm going to see if the attribute could be restricted to C
If you do restrict to C, then that should be all of the CFI attributes which are restricted to C? If so, we should definitely move on that before Clang 21 is released.
I don't see why all CFI attributes should be restricted to C. There may be some which can be for both, depending on circumstances.
I may change the name of this attribute 'kcfi_seed' to indicate that it's fairly specifically meant for kernel code.
As for modules/PCH files, those are C++ features, correct? I literally have no experience with them.
Modules are a C++ feature, but PCH files are both C and C++ (precompiled headers). Basically, when you include a PCH file it's the same as including a textual header but instead you're including a serialized binary blob representing the textual header, so you can run into the same situations with invalid redeclarations. But I think that's handled in a sufficiently generic way that you should be fine without any custom logic.
This would be a generic problem for all PCH's then; not just this one.
Re name mangling:
This goes back to whether it's impacting the function type. Generally that's only an issue in C++, but we have the
overloadableattribute which allows for overloading in C. And the way that's implemented is that it basically uses the C++ logic, including name mangling. So we may have to care about it, but if the salt attribute is not part of the function type, then there's no overloads and no need to worry.
Okay, sure.
There was a problem hiding this comment.
I don't see why all CFI attributes should be restricted to C. There may be some which can be for both, depending on circumstances.
Ah, this may be my ignorance of the domain then. I was thinking that if cfi_salt had these kinds of questions, any of the other cfi attributes would run into similar issues. If that's not the case, then yay!
This would be a generic problem for all PCH's then; not just this one.
Yeah, I was trying to remember whether you needed any special code to handle this scenario because so many of our attributes are not an issue when they have different arguments (consider deprecated: if the message is different between TUs, it's not a big deal). But I think I've convinced myself that there's no extra code needed.
| // 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) |
There was a problem hiding this comment.
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.
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:
Example of using this in a structure:
Link: ClangBuiltLinux/linux#1736
Link: KSPP/linux#365