[PATCH] D47567: Implement CFI for indirect calls via a member function pointer.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC335569: Implement CFI for indirect calls via a member
function pointer. (authored by pcc, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D47567?vs=151272&id=152819#toc
Repository:
rC Clang
https://reviews.llvm.org/D47567
Files:
docs/ControlFlowIntegrity.rst
docs/LTOVisibility.rst
include/clang/Basic/Sanitizers.def
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/CodeGenFunction.h
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/ItaniumCXXABI.cpp
lib/Driver/SanitizerArgs.cpp
lib/Driver/ToolChains/MSVC.cpp
test/CodeGenCXX/cfi-mfcall-incomplete.cpp
test/CodeGenCXX/cfi-mfcall.cpp
test/CodeGenCXX/type-metadata-memfun.cpp
test/CodeGenCXX/type-metadata.cpp
test/Driver/fsanitize.c
Index: lib/CodeGen/CGClass.cpp
===
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -2688,7 +2688,9 @@
SSK = llvm::SanStat_CFI_UnrelatedCast;
break;
case CFITCK_ICall:
-llvm_unreachable("not expecting CFITCK_ICall");
+ case CFITCK_NVMFCall:
+ case CFITCK_VMFCall:
+llvm_unreachable("unexpected sanitizer kind");
}
std::string TypeName = RD->getQualifiedNameAsString();
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -1132,6 +1132,34 @@
return true;
}
+static bool requiresMemberFunctionPointerTypeMetadata(CodeGenModule &CGM,
+ const CXXMethodDecl *MD) {
+ // Check that the type metadata can ever actually be used by a call.
+ if (!CGM.getCodeGenOpts().LTOUnit ||
+ !CGM.HasHiddenLTOVisibility(MD->getParent()))
+return false;
+
+ // Only functions whose address can be taken with a member function pointer
+ // need this sort of type metadata.
+ return !MD->isStatic() && !MD->isVirtual() && !isa(MD) &&
+ !isa(MD);
+}
+
+std::vector
+CodeGenModule::getMostBaseClasses(const CXXRecordDecl *RD) {
+ llvm::SetVector MostBases;
+
+ std::function CollectMostBases;
+ CollectMostBases = [&](const CXXRecordDecl *RD) {
+if (RD->getNumBases() == 0)
+ MostBases.insert(RD);
+for (const CXXBaseSpecifier &B : RD->bases())
+ CollectMostBases(B.getType()->getAsCXXRecordDecl());
+ };
+ CollectMostBases(RD);
+ return MostBases.takeVector();
+}
+
void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
llvm::Function *F) {
llvm::AttrBuilder B;
@@ -1257,7 +1285,20 @@
// In the cross-dso CFI mode, we want !type attributes on definitions only.
if (CodeGenOpts.SanitizeCfiCrossDso)
if (auto *FD = dyn_cast(D))
- CreateFunctionTypeMetadata(FD, F);
+ CreateFunctionTypeMetadataForIcall(FD, F);
+
+ // Emit type metadata on member functions for member function pointer checks.
+ // These are only ever necessary on definitions; we're guaranteed that the
+ // definition will be present in the LTO unit as a result of LTO visibility.
+ auto *MD = dyn_cast(D);
+ if (MD && requiresMemberFunctionPointerTypeMetadata(*this, MD)) {
+for (const CXXRecordDecl *Base : getMostBaseClasses(MD->getParent())) {
+ llvm::Metadata *Id =
+ CreateMetadataIdentifierForType(Context.getMemberPointerType(
+ MD->getType(), Context.getRecordType(Base).getTypePtr()));
+ F->addTypeMetadata(0, Id);
+}
+ }
}
void CodeGenModule::SetCommonAttributes(GlobalDecl GD, llvm::GlobalValue *GV) {
@@ -1378,13 +1419,14 @@
GV->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
}
-void CodeGenModule::CreateFunctionTypeMetadata(const FunctionDecl *FD,
- llvm::Function *F) {
+void CodeGenModule::CreateFunctionTypeMetadataForIcall(const FunctionDecl *FD,
+ llvm::Function *F) {
// Only if we are checking indirect calls.
if (!LangOpts.Sanitize.has(SanitizerKind::CFIICall))
return;
- // Non-static class methods are handled via vtable pointer checks elsewhere.
+ // Non-static class methods are handled via vtable or member function pointer
+ // checks elsewhere.
if (isa(FD) && !cast(FD)->isStatic())
return;
@@ -1476,7 +1518,7 @@
// Don't emit entries for function declarations in the cross-DSO mode. This
// is handled with better precision by the receiving DSO.
if (!CodeGenOpts.SanitizeCfiCrossDso)
-CreateFunctionTypeMetadata(FD, F);
+CreateFunctionTypeMetadataForIcall(FD, F);
if (getLangOpts().OpenMP && FD->hasAttr())
getOpenMPRuntime().emitDeclareSimdFunction(FD, F);
@@ -4925,15 +4967,18 @@
}
}
-llvm::Metadata *CodeGenModule::CreateMetadataIdentifierForType(QualType T) {
- llvm::Metadata *&InternalId =
[PATCH] D47567: Implement CFI for indirect calls via a member function pointer.
pcc added inline comments.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1413
+ FD->getType(), Context.getRecordType(Base).getTypePtr()));
+ F->addTypeMetadata(0, Id);
+}
vlad.tsyrklevich wrote:
> It'd be nice to have a test that reaches this.
I'll add one.
Comment at: clang/lib/CodeGen/CodeGenModule.h:1256
+ std::vector
+ getMostBaseClasses(const CXXRecordDecl *RD);
vlad.tsyrklevich wrote:
> Could be helpful to have a comment here to ensure there is no confusion
> interpreting this as 'the most-base classes' and not 'most of the base
> classes'.
Will do.
Comment at: clang/lib/Driver/SanitizerArgs.cpp:233
+ options::OPT_fno_sanitize_cfi_cross_dso, false);
+ if (CfiCrossDso)
+Supported &= ~CFIMFCall;
vlad.tsyrklevich wrote:
> This will cause supplying both options to fail with `clang: error:
> unsupported option '-fsanitize=cfi-mfcall' for target ...`. Having it error
> out the same way as type generalization below where it states that
> cfi-cross-dso is unsupported with cfi-mfcall seems like a more helpful error.
The issue with that is that it would cause the flag combination `-fsanitize=cfi
-fsanitize-cfi-cross-dso` to fail with an unsupported error. So I think it
would need to work in a similar way as with the supported sanitizers. I guess I
could add something after line 293 below instead.
Comment at: clang/test/CodeGenCXX/type-metadata.cpp:281
// ITANIUM: [[FA_ID]] = distinct !{}
// MS: [[A8]] = !{i64 8, !"?AUA@@"}
vlad.tsyrklevich wrote:
> Any reason not to include AF64/CF64/FAF16 here?
No, I just forgot to add them. I'll do that.
Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cc:645
+ const char *CheckKindStr = Data->CheckKind == CFITCK_NVMFCall
+ ? "non-virtual member function call"
+ : "indirect function call";
vlad.tsyrklevich wrote:
> s/member/pointer to member/ ?
Makes sense.
Comment at: compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc:126
+ case CFITCK_VMFCall:
+CheckKindStr = "virtual member function call";
+break;
vlad.tsyrklevich wrote:
> s/member/pointer to member/ ?
Ditto
https://reviews.llvm.org/D47567
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47567: Implement CFI for indirect calls via a member function pointer.
vlad.tsyrklevich accepted this revision.
vlad.tsyrklevich added a comment.
This revision is now accepted and ready to land.
I think it would be clearer to replace uses of 'member function pointer' with
'pointer to member function'; however, a google search shows that the usage of
both terms is basically the same so not this might be just be my own bias
coming through.
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1413
+ FD->getType(), Context.getRecordType(Base).getTypePtr()));
+ F->addTypeMetadata(0, Id);
+}
It'd be nice to have a test that reaches this.
Comment at: clang/lib/CodeGen/CodeGenModule.h:1256
+ std::vector
+ getMostBaseClasses(const CXXRecordDecl *RD);
Could be helpful to have a comment here to ensure there is no confusion
interpreting this as 'the most-base classes' and not 'most of the base classes'.
Comment at: clang/lib/Driver/SanitizerArgs.cpp:233
+ options::OPT_fno_sanitize_cfi_cross_dso, false);
+ if (CfiCrossDso)
+Supported &= ~CFIMFCall;
This will cause supplying both options to fail with `clang: error: unsupported
option '-fsanitize=cfi-mfcall' for target ...`. Having it error out the same
way as type generalization below where it states that cfi-cross-dso is
unsupported with cfi-mfcall seems like a more helpful error.
Comment at: clang/test/CodeGenCXX/type-metadata.cpp:281
// ITANIUM: [[FA_ID]] = distinct !{}
// MS: [[A8]] = !{i64 8, !"?AUA@@"}
Any reason not to include AF64/CF64/FAF16 here?
Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cc:645
+ const char *CheckKindStr = Data->CheckKind == CFITCK_NVMFCall
+ ? "non-virtual member function call"
+ : "indirect function call";
s/member/pointer to member/ ?
Comment at: compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc:126
+ case CFITCK_VMFCall:
+CheckKindStr = "virtual member function call";
+break;
s/member/pointer to member/ ?
https://reviews.llvm.org/D47567
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47567: Implement CFI for indirect calls via a member function pointer.
pcc updated this revision to Diff 151272.
pcc added a comment.
- Add some more documentation to ControlFlowIntegrity.rst
https://reviews.llvm.org/D47567
Files:
clang/docs/ControlFlowIntegrity.rst
clang/docs/LTOVisibility.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGClass.cpp
clang/lib/CodeGen/CGVTables.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/lib/CodeGen/ItaniumCXXABI.cpp
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/MSVC.cpp
clang/test/CodeGenCXX/cfi-mfcall-incomplete.cpp
clang/test/CodeGenCXX/cfi-mfcall.cpp
clang/test/CodeGenCXX/type-metadata.cpp
clang/test/Driver/fsanitize.c
compiler-rt/lib/ubsan/ubsan_handlers.cc
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc
compiler-rt/test/cfi/mfcall.cpp
Index: compiler-rt/test/cfi/mfcall.cpp
===
--- /dev/null
+++ compiler-rt/test/cfi/mfcall.cpp
@@ -0,0 +1,94 @@
+// RUN: %clangxx_cfi -o %t %s
+// RUN: %expect_crash %run %t a
+// RUN: %expect_crash %run %t b
+// RUN: %expect_crash %run %t c
+// RUN: %expect_crash %run %t d
+// RUN: %expect_crash %run %t e
+// RUN: %run %t f
+// RUN: %run %t g
+
+// RUN: %clangxx_cfi_diag -o %t2 %s
+// RUN: %run %t2 a 2>&1 | FileCheck --check-prefix=A %s
+// RUN: %run %t2 b 2>&1 | FileCheck --check-prefix=B %s
+// RUN: %run %t2 c 2>&1 | FileCheck --check-prefix=C %s
+// RUN: %run %t2 d 2>&1 | FileCheck --check-prefix=D %s
+// RUN: %run %t2 e 2>&1 | FileCheck --check-prefix=E %s
+
+#include
+#include
+
+struct SBase1 {
+ void b1() {}
+};
+
+struct SBase2 {
+ void b2() {}
+};
+
+struct S : SBase1, SBase2 {
+ void f1() {}
+ int f2() { return 1; }
+ virtual void g1() {}
+ virtual int g2() { return 1; }
+ virtual int g3() { return 1; }
+};
+
+struct T {
+ void f1() {}
+ int f2() { return 2; }
+ virtual void g1() {}
+ virtual int g2() { return 2; }
+ virtual void g3() {}
+};
+
+typedef void (S::*S_void)();
+
+typedef int (S::*S_int)();
+typedef int (T::*T_int)();
+
+template
+To bitcast(From f) {
+ assert(sizeof(To) == sizeof(From));
+ To t;
+ memcpy(&t, &f, sizeof(f));
+ return t;
+}
+
+int main(int argc, char **argv) {
+ S s;
+ T t;
+
+ switch (argv[1][0]) {
+case 'a':
+ // A: runtime error: control flow integrity check for type 'int (S::*)()' failed during non-virtual member function call
+ // A: note: S::f1() defined here
+ (s.*bitcast(&S::f1))();
+ break;
+case 'b':
+ // B: runtime error: control flow integrity check for type 'int (T::*)()' failed during non-virtual member function call
+ // B: note: S::f2() defined here
+ (t.*bitcast(&S::f2))();
+ break;
+case 'c':
+ // C: runtime error: control flow integrity check for type 'int (S::*)()' failed during virtual member function call
+ // C: note: vtable is of type 'S'
+ (s.*bitcast(&S::g1))();
+ break;
+case 'd':
+ // D: runtime error: control flow integrity check for type 'int (S::*)()' failed during virtual member function call
+ // D: note: vtable is of type 'T'
+ (reinterpret_cast(t).*&S::g2)();
+ break;
+case 'e':
+ // E: runtime error: control flow integrity check for type 'void (S::*)()' failed during virtual member function call
+ // E: note: vtable is of type 'S'
+ (s.*bitcast(&T::g3))();
+ break;
+case 'f':
+ (s.*&SBase1::b1)();
+ break;
+case 'g':
+ (s.*&SBase2::b2)();
+ break;
+ }
+}
Index: compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc
===
--- compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc
+++ compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc
@@ -122,7 +122,11 @@
case CFITCK_UnrelatedCast:
CheckKindStr = "cast to unrelated type";
break;
+ case CFITCK_VMFCall:
+CheckKindStr = "virtual member function call";
+break;
case CFITCK_ICall:
+ case CFITCK_NVMFCall:
Die();
}
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
--- compiler-rt/lib/ubsan/ubsan_handlers.h
+++ compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -181,6 +181,8 @@
CFITCK_DerivedCast,
CFITCK_UnrelatedCast,
CFITCK_ICall,
+ CFITCK_NVMFCall,
+ CFITCK_VMFCall,
};
struct CFICheckFailData {
Index: compiler-rt/lib/ubsan/ubsan_handlers.cc
===
--- compiler-rt/lib/ubsan/ubsan_handlers.cc
+++ compiler-rt/lib/ubsan/ubsan_handlers.cc
@@ -630,7 +630,7 @@
static void handleCFIBadIcall(CFICheckFailData *Data, ValueHandle Function,
ReportOptions Opts) {
- if (Data->CheckKind != CFITCK_ICall)
+ if (Data->CheckKind != CFITCK_ICall && Data->CheckKind != CFITCK_NVMFCall)
Die();
SourceLocation Loc = Data->Loc.acquire();
@@ -641,9 +641,12 @@
Scoped
