[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) |
clang/test/CodeGen/kcfi-salt.c
Outdated
| // 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you work out what a few of those are? The attribute only helps if we can actually partition them.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
…t it's specifically for the kernel.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@kees, what should the behavior of |
bwendling
changed the title
Yeah, I think that's reasonable. |
There was a problem hiding this comment.
This is looking reasonable to me. Some minor questions on the documentation, but otherwise I leave it to @erichkeane for the final sign off.
|
|
||
| **Notes:** | ||
|
|
||
| * The salt string can contain any characters, including spaces and quotes |
There was a problem hiding this comment.
Embedded nulls? (If so, we should have a test case for that situation.) Also, do we need to say anything about the text encoding? (Any other weird things we need to be aware of @cor3ntin?)
There was a problem hiding this comment.
Unicode, wide-characters, an excessive amount of characters (think "War and Peace"), etc. I'm thinking of backing off of this claim and simply say non-null ASCII characters, unless we need can support those easily (we're using StringRef...).
There was a problem hiding this comment.
I think non-null ASCII characters would be good; we can basically claim it is a bucket of non-zero bytes more than a textual string, really. Or, maybe the salt should be a 64-bit numeric value? e.g., __cfi_salt(0xDEADBEEF) (If that's a dumb suggestion, feel free to ignore it, I'm not certain what the backend wants to do with the string itself.)
clang/lib/Sema/SemaType.cpp
Outdated
| return false; | ||
|
|
||
| const auto *FnTy = unwrapped.get()->getAs<FunctionProtoType>(); | ||
| if (!FnTy) |
There was a problem hiding this comment.
So this is intentionally disallowing functions without a prototype? Do you have a test for that? Will it diagnose?
There was a problem hiding this comment.
The attribute is accessible only via the FunctionProtoType. To be more accurate, it really needs to be on extern function forward decls and the definition, because otherwise the attribute won't propagate correctly.
I'll add a test case to ensure this.
There was a problem hiding this comment.
Then my question is: why the if(!FnTy)? Do we have a case where this matters?
Likely a few of the returns here should just be asserts.
There was a problem hiding this comment.
Well, the FnTy is a FunctionProtoType, which is what holds the information. If you want an example, look at the attribute handling below this code. However, I'm not able to trigger this code path. I suppose I can remove it if you like.
There was a problem hiding this comment.
Exactly :) In reality, 7958 should be a castAs, not a getAs.
There was a problem hiding this comment.
It DOES look like that, but he is perhaps correct that handleFunctionTypeAttr only works on prototype-functions? I just want to make sure that we either:
- diagnose (not silently ignore) here
- Are already diagnosing elsewhere and don't get here in this case, at which point this could be a
castAs - Diagnose elsewhere, BUT still get here: we should perhaps understand how that happens.
There was a problem hiding this comment.
From my look at the code paths, I think you can get here with a function without a prototype. For example, this function handles noreturn too, yet: https://godbolt.org/z/q895rn3zr
There was a problem hiding this comment.
Huh, interesting, I could have sworn we had a different location for no-prototype-attribute handling, but I don't think so anymore (after digging in for a few mins).
My bullets still are relevant. I wouldn't expect an attribute that doesn't pass the builtin-TableGen'd stuff to reach this function (like for arg counts/appertainment/etc), so if this IS getting here, it either needs to diagnose here, or figure out WHY we're continuing after failing tablegen.
There was a problem hiding this comment.
I wouldn't expect an attribute that doesn't pass the builtin-TableGen'd stuff to reach this function (like for arg counts/appertainment/etc), so if this IS getting here, it either needs to diagnose here, or figure out WHY we're continuing after failing tablegen.
Type attributes do not have as much automagical hookup with tablegen as declaration attributes; so IIRC all of them have to do manual processing like this. So I do expect to get here (at least, with my psychic debugger), and I think we likely need to diagnose explicitly.
There was a problem hiding this comment.
Ah, hrmph, that is unfortunate. I'd at least like a test to show that it diagnoses for non-prototype functions (even if it doesn't need to be diagnosed here because TG takes care of it).
Perhaps at one point we SHOULD start automatically pulling stuff from TG here/automating better.
Wrong. They are nits. It's just you lacking a fundamental understanding of the code. Sorry for the confusion, but I wrote the code to avoid ICEs. I don't simply go around adding dead paths to code. |
…ostic messages to the same line.
|
You need to realize that submitted this code for review in May. It's like that I forgot some of the details of why I wrote something. But what I wrote was intentionally put there for a reason. There's a new commit. PTAL. |
|
And if you're flagging this for "changes required", please list those changes. |
Honestly, this is a case where I was heavily unmotivated to review your code due to past behavior, and didn't until asked explicitly by Aaron. It waited due to your inability to respect reviewers in the past.
Due to me "lacking a fundamental understanding of the code" I need to spend extensive amounts of time familiarizing myself with "the code" before I can release this review. |
I'm sorry for my behavior. I don't intend to be confrontational, but people keep arguing with me about code that I've written intentionally to avoid ICEs and the like. But when you wrote this:
I found it insulting and lost my temper, because I understood what you're said before this. I shouldn't have, but really I know why you want asserts there. I didn't make it an assert because it would ICE otherwise. |
|
|
||
| **Notes:** | ||
|
|
||
| * The salt string can contain any characters, including spaces and quotes |
There was a problem hiding this comment.
I think non-null ASCII characters would be good; we can basically claim it is a bucket of non-zero bytes more than a textual string, really. Or, maybe the salt should be a 64-bit numeric value? e.g., __cfi_salt(0xDEADBEEF) (If that's a dumb suggestion, feel free to ignore it, I'm not certain what the backend wants to do with the string itself.)
clang/lib/Sema/SemaType.cpp
Outdated
| return false; | ||
|
|
||
| const auto *FnTy = unwrapped.get()->getAs<FunctionProtoType>(); | ||
| if (!FnTy) |
There was a problem hiding this comment.
I think what's being asked for is a test case like:
// RUN: %clang_cc1 -std=c89 -fsyntax-only -fsanitize=kcfi -verify %s
// K&R C function without a prototype
void func() __attribute__((cfi_salt("pepper"))); // expected-error {{attribute only applies to function types with a prototype}}
void (*fp)() __attribute__((cfi_salt("pepper"))); // expected-error {{attribute only applies to function pointer types with a prototype}}
(or something along those lines). As it stands, it looks like this code will silently drop the attribute because getAs will return null and we'll early return here.
| int bad1() __cfi_salt(); // expected-error{{'cfi_salt' attribute takes one argument}} | ||
| int bad2() __cfi_salt(42); // expected-error{{expected string literal as argument of 'cfi_salt' attribute}} | ||
| int bad3() __attribute__((cfi_salt("a", "b", "c"))); // expected-error{{'cfi_salt' attribute takes one argument}} |
There was a problem hiding this comment.
| int bad1() __cfi_salt(); // expected-error{{'cfi_salt' attribute takes one argument}} | |
| int bad2() __cfi_salt(42); // expected-error{{expected string literal as argument of 'cfi_salt' attribute}} | |
| int bad3() __attribute__((cfi_salt("a", "b", "c"))); // expected-error{{'cfi_salt' attribute takes one argument}} | |
| int bad1(void) __cfi_salt(); // expected-error{{'cfi_salt' attribute takes one argument}} | |
| int bad2(void) __cfi_salt(42); // expected-error{{expected string literal as argument of 'cfi_salt' attribute}} | |
| int bad3(void) __attribute__((cfi_salt("a", "b", "c"))); // expected-error{{'cfi_salt' attribute takes one argument}} |
This makes it more clear as to what's being tested (it's not the function prototype that matters for these tests, it's the attribute argument list).
There was a problem hiding this comment.
I think non-null ASCII characters would be good
Agreed: It can both be included in the existing mangle string -> hash step, and I expect this to be a "meaningful" string for authors to use (e.g. "sched" or "kvm") etc.
There was a problem hiding this comment.
I specified that it should be non-NULL ASCII characters.
clang/lib/Sema/SemaType.cpp
Outdated
| return false; | ||
|
|
||
| const auto *FnTy = unwrapped.get()->getAs<FunctionProtoType>(); | ||
| if (!FnTy) |
There was a problem hiding this comment.
It DOES look like that, but he is perhaps correct that handleFunctionTypeAttr only works on prototype-functions? I just want to make sure that we either:
- diagnose (not silently ignore) here
- Are already diagnosing elsewhere and don't get here in this case, at which point this could be a
castAs - Diagnose elsewhere, BUT still get here: we should perhaps understand how that happens.
| @@ -7941,6 +7942,31 @@ static bool handleFunctionTypeAttr(TypeProcessingState &state, ParsedAttr &attr, | |||
| return true; | |||
| } | |||
|
|
|||
| if (attr.getKind() == ParsedAttr::AT_CFISalt) { | |||
| if (attr.getNumArgs() != 1) | |||
There was a problem hiding this comment.
I'm still a little surprised we actually GET to this. I would expect whoever does the diagnostic here should ensure we don't get to this attribute, and I'd like us to understand why that is (that is, that this can get here).
I realize this is all OLD code (this function) that likely was written with different expectations, so I want to make sure we're not using those for not a good reason.
There was a problem hiding this comment.
We get here before the "normal" function type attribute processing, which is:
clang::Sema::ActOnDeclarator() calls
clang::Sema::HandleDeclarator() calls
clang::Sema::ActOnFunctionDeclarator() calls
clang::Sema::ProcessDeclAttributes() calls
clang::Sema::ProcessDeclAttributeList()
This eventually calls checkCommonAttributeFeatures, which checks general attribute features (things that are specified in Attr.td). The diagnostics we emit in this place (handleFunctionTypeAttr) are things which aren't specified in Attr.td, like a lack of a FunctionProtoType (which I'll add, a la Aaron's suggestion above). Whether this is the "correct" way to process these types of function attributes or we should adopt another more "modern" method is something I don't know. Function attributes appear to be handled in a far more complex way than attributes on other objects.
There was a problem hiding this comment.
The diagnostics we emit in this place (handleFunctionTypeAttr) are things which aren't specified in Attr.td, like a lack of a FunctionProtoType (which I'll add, a la Aaron's suggestion above). Whether this is the "correct" way to process these types of function attributes or we should adopt another more "modern" method is something I don't know. Function attributes appear to be handled in a far more complex way than attributes on other objects.
Yup, that matches my recollection. Basically, type attributes in general require a fair amount of special work to fit them into type system properly, so we've never really automated a lot of the checking for them like we did with decl and stmt attributes. It's always been on my list of things I'd like to fix, but it's never made it to the top of my queue.
I don't think @bwendling has to do any of the generalization, but it does mean we need to manually check for things like appertainment, argument counts, etc explicitly.
There was a problem hiding this comment.
Yeah, I guess I'm just really surprised we haven't bailed out by now. I guess another one for my 'do someday' list as well. Its just odd that we automated the checks, but still continued on with this part of the code.
|
The new commits changes the description to specify "non-NULL ASCII characters" and adds a diagnostic for K&R functions without prototypes. |
| @@ -7941,6 +7942,31 @@ static bool handleFunctionTypeAttr(TypeProcessingState &state, ParsedAttr &attr, | |||
| return true; | |||
| } | |||
|
|
|||
| if (attr.getKind() == ParsedAttr::AT_CFISalt) { | |||
| if (attr.getNumArgs() != 1) | |||
There was a problem hiding this comment.
The diagnostics we emit in this place (handleFunctionTypeAttr) are things which aren't specified in Attr.td, like a lack of a FunctionProtoType (which I'll add, a la Aaron's suggestion above). Whether this is the "correct" way to process these types of function attributes or we should adopt another more "modern" method is something I don't know. Function attributes appear to be handled in a far more complex way than attributes on other objects.
Yup, that matches my recollection. Basically, type attributes in general require a fair amount of special work to fit them into type system properly, so we've never really automated a lot of the checking for them like we did with decl and stmt attributes. It's always been on my list of things I'd like to fix, but it's never made it to the top of my queue.
I don't think @bwendling has to do any of the generalization, but it does mean we need to manually check for things like appertainment, argument counts, etc explicitly.
|
|
||
| StringRef Argument; | ||
| if (!S.checkStringLiteralArgumentAttr(attr, 0, Argument)) | ||
| return false; |
There was a problem hiding this comment.
I think this should be return true; returning false basically says "try to slide this attribute around to something else until it makes sense" but in this case the attribute is wrong and won't make sense anywhere.
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 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