Implement CFI type checks for non-virtual calls.

[pcc]

Summary

This uses the same class metadata currently used for virtual call and
cast checks.

The new flag is -fsanitize=cfi-nvcall. For consistency, the -fsanitize=cfi-vptr
flag has been renamed -fsanitize=cfi-vcall.

[kcc]

LGTM, but please also add compiler-rt test(s)

[jfb]

test/CodeGenCXX/cfi-nvcall.cpp

31

Could you add a comment that explains the difference between these two calls to g(), and why the strict check looks for C specifically here whereas the non-strict one accepts A? If c is actually an A then the code is still wrong, but if it's a B then the programmer is technically wrong but a lot of code does this like LLVM itself (I think you had an explanation with examples in an email?).

[pcc]

• Add a comment regarding strictness

test/CodeGenCXX/cfi-nvcall.cpp

31

Done

[pcc]

In D8756#150707, @kcc wrote:

LGTM, but please also add compiler-rt test(s)

D8792

[jfb]

lgtm

docs/ControlFlowIntegrity.rst

 To enable Clang's available CFI schemes, use the flag ``-fsanitize=cfi``.
 As currently implemented, CFI relies on link-time optimization (LTO); the CFI
 schemes imply ``-flto``, and the linker used must support LTO, for example
 via the `gold plugin`_. To allow the checks to be implemented efficiently,
 the program must be structured such that certain object files are compiled
 with CFI enabled, and are statically linked into the program. This may
 preclude the use of shared libraries in some cases.
 
-Clang currently implements forward-edge CFI for virtual calls. More schemes
-are under development.
+Clang currently implements forward-edge CFI for calls and bad cast
+checking. More schemes are under development.
 
 .. _gold plugin: http://llvm.org/docs/GoldPlugin.html
 
 Forward-Edge CFI for Virtual Calls
 ----------------------------------
 
 This scheme checks that virtual calls take place using a vptr of the correct
 dynamic type; that is, the dynamic type of the called object must be a
 derived class of the static type of the object used to make the call.
-This CFI scheme can be enabled on its own using ``-fsanitize=cfi-vptr``.
+This CFI scheme can be enabled on its own using ``-fsanitize=cfi-vcall``.
 
 For this scheme to work, all translation units containing the definition
 of a virtual member function (whether inline or not) must be compiled
-with ``-fsanitize=cfi-vptr`` enabled and be statically linked into the
+with ``-fsanitize=cfi-vcall`` enabled and be statically linked into the
 program. Classes in the C++ standard library (under namespace ``std``) are
 exempted from checking, and therefore programs may be linked against a
 pre-built standard library, but this may change in the future.
 
 Performance
 ~~~~~~~~~~~
 
 A performance overhead of less than 1% has been measured by running the

 For this scheme to work, all translation units containing the definition
 of a virtual member function (whether inline or not) must be compiled with
 ``-fsanitize=cfi-derived-cast`` or ``-fsanitize=cfi-unrelated-cast`` enabled
 and be statically linked into the program. Classes in the C++ standard library
 (under namespace ``std``) are exempted from checking, and therefore programs
 may be linked against a pre-built standard library, but this may change in
 the future.
 
+Non-Virtual Call Checking
+-------------------------
+
+This scheme checks that non-virtual calls take place using an object of
+the correct dynamic type; that is, the dynamic type of the called object
+must be a derived class of the static type of the object used to make the
+call. The checks are currently only introduced where the object is of a
+polymorphic class type.  This CFI scheme can be enabled on its own using
+``-fsanitize=cfi-nvcall``.
+
+For this scheme to work, all translation units containing the definition
+of a virtual member function (whether inline or not) must be compiled
+with ``-fsanitize=cfi-nvcall`` enabled and be statically linked into the
+program. Classes in the C++ standard library (under namespace ``std``) are
+exempted from checking, and therefore programs may be linked against a
+pre-built standard library, but this may change in the future.
+
 .. _cfi-strictness:
 
 Strictness
 ~~~~~~~~~~
 
 If a class has a single non-virtual base and does not introduce or override
 virtual member functions or fields other than an implicitly defined virtual
 destructor, it will have the same layout and virtual function semantics as

docs/UsersManual.rst

**-f[no-]sanitize=check1,check2,...**
    -  ``-fsanitize=bounds``: Out of bounds array indexing, in cases
       where the array bound can be statically determined.
    -  ``-fsanitize=cfi-cast-strict``: Enables :ref:`strict cast checks
       <cfi-strictness>`.
    -  ``-fsanitize=cfi-derived-cast``: Base-to-derived cast to the wrong
       dynamic type. Implies ``-flto``.
    -  ``-fsanitize=cfi-unrelated-cast``: Cast from ``void*`` or another
       unrelated type to the wrong dynamic type. Implies ``-flto``.
-   -  ``-fsanitize=cfi-vptr``: Use of an object whose vptr is of the
+   -  ``-fsanitize=cfi-nvcall``: Non-virtual call via an object whose vptr is of
+      the wrong dynamic type. Implies ``-flto``.
+   -  ``-fsanitize=cfi-vcall``: Virtual call via an object whose vptr is of the
       wrong dynamic type. Implies ``-flto``.
    -  ``-fsanitize=enum``: Load of a value of an enumerated type which
       is not in the range of representable values for that enumerated
       type.
    -  ``-fsanitize=float-cast-overflow``: Conversion to, from, or
       between floating-point types which would overflow the
       destination.
    -  ``-fsanitize=float-divide-by-zero``: Floating point division by

include/clang/Basic/Sanitizers.def

 
 // DataFlowSanitizer
 SANITIZER("dataflow", DataFlow)
 
 // Control Flow Integrity
 SANITIZER("cfi-cast-strict", CFICastStrict)
 SANITIZER("cfi-derived-cast", CFIDerivedCast)
 SANITIZER("cfi-unrelated-cast", CFIUnrelatedCast)
-SANITIZER("cfi-vptr", CFIVptr)
-SANITIZER_GROUP("cfi", CFI, CFIDerivedCast | CFIUnrelatedCast | CFIVptr)
+SANITIZER("cfi-nvcall", CFINVCall)
+SANITIZER("cfi-vcall", CFIVCall)
+SANITIZER_GROUP("cfi", CFI,
+                CFIDerivedCast | CFIUnrelatedCast | CFINVCall | CFIVCall)
 
 // -fsanitize=undefined-trap includes sanitizers from -fsanitize=undefined
 // that can be used without runtime support, generally by providing extra
 // -fsanitize-undefined-trap-on-error flag.
 SANITIZER_GROUP("undefined-trap", UndefinedTrap,
                 Alignment | Bool | ArrayBounds | Enum | FloatCastOverflow |
                     FloatDivideByZero | IntegerDivideByZero | NonnullAttribute |
                     Null | ObjectSize | Return | ReturnsNonnullAttribute |

lib/CodeGen/CGClass.cpp

 llvm::Value *CodeGenFunction::GetVTablePtr(llvm::Value *This,
                                            llvm::Type *Ty) {
   llvm::Value *VTablePtrSrc = Builder.CreateBitCast(This, Ty->getPointerTo());
   llvm::Instruction *VTable = Builder.CreateLoad(VTablePtrSrc, "vtable");
   CGM.DecorateInstruction(VTable, CGM.getTBAAInfoForVTablePtr());
   return VTable;
 }
 
-void CodeGenFunction::EmitVTablePtrCheckForCall(const CXXMethodDecl *MD,
-                                                llvm::Value *VTable) {
-  if (!SanOpts.has(SanitizerKind::CFIVptr))
-    return;
-
-  EmitVTablePtrCheck(MD->getParent(), VTable);
-}
-
 // If a class has a single non-virtual base and does not introduce or override
 // virtual member functions or fields, it will have the same layout as its base.
 // This function returns the least derived such class.
 //
 // Casting an instance of a base class to such a derived class is technically
 // undefined behavior, but it is a relatively common hack for introducing member
 // functions on class instances with specific properties (e.g. llvm::Operator)
 // that works under most compilers and should not have security implications, so
if (MD->isVirtual()) {
       return RD;
     }
   }
 
   return LeastDerivedClassWithSameLayout(
       RD->bases_begin()->getType()->getAsCXXRecordDecl());
 }
 
+void CodeGenFunction::EmitVTablePtrCheckForCall(const CXXMethodDecl *MD,
+                                                llvm::Value *VTable) {
+  const CXXRecordDecl *ClassDecl = MD->getParent();
+  if (!SanOpts.has(SanitizerKind::CFICastStrict))
+    ClassDecl = LeastDerivedClassWithSameLayout(ClassDecl);
+
+  EmitVTablePtrCheck(ClassDecl, VTable);
+}
+
 void CodeGenFunction::EmitVTablePtrCheckForCast(QualType T,
                                                 llvm::Value *Derived,
                                                 bool MayBeNull) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   auto *ClassTy = T->getAs<RecordType>();
   if (!ClassTy)

lib/CodeGen/CGExprCXX.cpp

if (const CXXDestructorDecl *Dtor = dyn_cast<CXXDestructorDecl>(MD)) {
     return RValue::get(nullptr);
   }
   
   if (const CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(MD)) {
     Callee = CGM.GetAddrOfFunction(GlobalDecl(Ctor, Ctor_Complete), Ty);
   } else if (UseVirtualCall) {
     Callee = CGM.getCXXABI().getVirtualFunctionPointer(*this, MD, This, Ty);
   } else {
+    if (SanOpts.has(SanitizerKind::CFINVCall) &&
+        MD->getParent()->isDynamicClass()) {
+      llvm::Value *VTable = GetVTablePtr(This, Int8PtrTy);
+      EmitVTablePtrCheckForCall(MD, VTable);
+    }
+
     if (getLangOpts().AppleKext && MD->isVirtual() && HasQualifier)
       Callee = BuildAppleKextVirtualCall(MD, Qualifier, Ty);
     else if (!DevirtualizedMethod)
       Callee = CGM.GetAddrOfFunction(MD, Ty);
     else {
       Callee = CGM.GetAddrOfFunction(DevirtualizedMethod, Ty);
     }
   }

lib/CodeGen/CGVTables.cpp

#endif
 
   assert(savedSize == DeferredVTables.size() &&
          "deferred extra v-tables during v-table emission?");
   DeferredVTables.clear();
 }
 
 void CodeGenModule::EmitVTableBitSetEntries(llvm::GlobalVariable *VTable,
                                             const VTableLayout &VTLayout) {
-  if (!LangOpts.Sanitize.has(SanitizerKind::CFIVptr))
+  if (!LangOpts.Sanitize.has(SanitizerKind::CFIVCall) &&
+      !LangOpts.Sanitize.has(SanitizerKind::CFINVCall) &&
+      !LangOpts.Sanitize.has(SanitizerKind::CFIDerivedCast) &&
+      !LangOpts.Sanitize.has(SanitizerKind::CFIUnrelatedCast))
     return;
 
   llvm::Metadata *VTableMD = llvm::ConstantAsMetadata::get(VTable);
 
   std::vector<llvm::MDTuple *> BitsetEntries;
   // Create a bit set entry for each address point.
   for (auto &&AP : VTLayout.getAddressPoints()) {
     // FIXME: Add blacklisting scheme.

lib/CodeGen/ItaniumCXXABI.cpp

 llvm::Value *ItaniumCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF,
                                                       GlobalDecl GD,
                                                       llvm::Value *This,
                                                       llvm::Type *Ty) {
   GD = GD.getCanonicalDecl();
   Ty = Ty->getPointerTo()->getPointerTo();
   llvm::Value *VTable = CGF.GetVTablePtr(This, Ty);
 
+  if (CGF.SanOpts.has(SanitizerKind::CFIVCall))
     CGF.EmitVTablePtrCheckForCall(cast<CXXMethodDecl>(GD.getDecl()), VTable);
 
   uint64_t VTableIndex = CGM.getItaniumVTableContext().getMethodVTableIndex(GD);
   llvm::Value *VFuncPtr =
       CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
   return CGF.Builder.CreateLoad(VFuncPtr);
 }
 
 llvm::Value *ItaniumCXXABI::EmitVirtualDestructorCall(

lib/Driver/SanitizerArgs.cpp

#include "clang/Basic/Sanitizers.def"
   NeedsUbsanRt = Undefined | Integer,
   NotAllowedWithTrap = Vptr,
   RequiresPIE = Memory | DataFlow,
   NeedsUnwindTables = Address | Thread | Memory | DataFlow,
   SupportsCoverage = Address | Memory | Leak | Undefined | Integer | DataFlow,
   RecoverableByDefault = Undefined | Integer,
   Unrecoverable = Address | Unreachable | Return,
   LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFIDerivedCast | CFIUnrelatedCast | CFIVptr,
+  NeedsLTO = CFI,
 };
 }
 
 /// Returns true if set of \p Sanitizers contain at least one sanitizer from
 /// \p Kinds.
 static bool hasOneOf(const clang::SanitizerSet &Sanitizers, uint64_t Kinds) {
 #define SANITIZER(NAME, ID)                                                    \
   if (Sanitizers.has(clang::SanitizerKind::ID) && (Kinds & ID))                \

test/CodeGenCXX/cfi-nvcall.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-nvcall -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-nvcall,cfi-cast-strict -emit-llvm -o - %s | FileCheck --check-prefix=CHECK-STRICT %s
+
+struct A {
+  virtual void f();
+};
+
+struct B : A {
+  int i;
+  void g();
+};
+
+struct C : A {
+  void g();
+};
+
+// CHECK-LABEL: @bg
+// CHECK-STRICT-LABEL: @bg
+extern "C" void bg(B *b) {
+  // CHECK: call i1 @llvm.bitset.test(i8* {{%[^ ]*}}, metadata !"1B")
+  // CHECK-STRICT: call i1 @llvm.bitset.test(i8* {{%[^ ]*}}, metadata !"1B")
+  b->g();
+}
+
+// CHECK-LABEL: @cg
+// CHECK-STRICT-LABEL: @cg
+extern "C" void cg(C *c) {
+  // CHECK: call i1 @llvm.bitset.test(i8* {{%[^ ]*}}, metadata !"1A")
+  // CHECK-STRICT: call i1 @llvm.bitset.test(i8* {{%[^ ]*}}, metadata !"1C")
+  c->g();
+}

test/CodeGenCXX/cfi-vcall.cpp

-// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vptr -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -emit-llvm -o - %s | FileCheck %s
 
 struct A {
   A();
   virtual void f();
 };
 
 struct B : virtual A {
   B();
void af(A *a) {
 
   // CHECK: [[CONTBB]]
   // CHECK: call void %
   a->f();
 }
 
 // CHECK: define internal void @_Z2dfPN12_GLOBAL__N_11DE
 void df(D *d) {
-  // CHECK: {{%[^ ]*}} = call i1 @llvm.bitset.test(i8* {{%[^ ]*}}, metadata !"[{{.*}}cfi-vptr.cpp]N12_GLOBAL__N_11DE")
+  // CHECK: {{%[^ ]*}} = call i1 @llvm.bitset.test(i8* {{%[^ ]*}}, metadata !"[{{.*}}cfi-vcall.cpp]N12_GLOBAL__N_11DE")
   d->f();
 }
 
 D d;
 
 void foo() {
   df(&d);
 }
 
 // CHECK-DAG: !{!"1A", [3 x i8*]* @_ZTV1A, i64 16}
 // CHECK-DAG: !{!"1A", [5 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // CHECK-DAG: !{!"1B", [5 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // CHECK-DAG: !{!"1A", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 64}
 // CHECK-DAG: !{!"1C", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 32}
 // CHECK-DAG: !{!"1A", [10 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // CHECK-DAG: !{!"1B", [10 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // CHECK-DAG: !{!"1C", [10 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 72}
-// CHECK-DAG: !{!"[{{.*}}cfi-vptr.cpp]N12_GLOBAL__N_11DE", [10 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
+// CHECK-DAG: !{!"[{{.*}}cfi-vcall.cpp]N12_GLOBAL__N_11DE", [10 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // CHECK-DAG: !{!"1A", [5 x i8*]* @_ZTV1B, i64 32}
 // CHECK-DAG: !{!"1B", [5 x i8*]* @_ZTV1B, i64 32}
 // CHECK-DAG: !{!"1A", [5 x i8*]* @_ZTV1C, i64 32}
 // CHECK-DAG: !{!"1C", [5 x i8*]* @_ZTV1C, i64 32}

test/Driver/fsanitize.c

 // CHECK-FSAN-DARWIN: unsupported option '-fsanitize=function' for target 'x86_64-apple-darwin10'
 
 // RUN: %clang -target x86_64-apple-darwin10 -fsanitize=function -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FSAN-UBSAN-DARWIN
 // CHECK-FSAN-UBSAN-DARWIN: unsupported option '-fsanitize=function' for target 'x86_64-apple-darwin10'
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-derived-cast -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-DCAST
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-unrelated-cast -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-UCAST
-// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vptr -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-VPTR
-// CHECK-CFI: -emit-llvm-bc{{.*}}-fsanitize=cfi-derived-cast,cfi-unrelated-cast,cfi-vptr
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-nvcall -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NVCALL
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-VCALL
+// CHECK-CFI: -emit-llvm-bc{{.*}}-fsanitize=cfi-derived-cast,cfi-unrelated-cast,cfi-nvcall,cfi-vcall
 // CHECK-CFI-DCAST: -emit-llvm-bc{{.*}}-fsanitize=cfi-derived-cast
 // CHECK-CFI-UCAST: -emit-llvm-bc{{.*}}-fsanitize=cfi-unrelated-cast
-// CHECK-CFI-VPTR: -emit-llvm-bc{{.*}}-fsanitize=cfi-vptr
+// CHECK-CFI-NVCALL: -emit-llvm-bc{{.*}}-fsanitize=cfi-nvcall
+// CHECK-CFI-VCALL: -emit-llvm-bc{{.*}}-fsanitize=cfi-vcall
 
 // RUN: %clang_cl -fsanitize=address -c -MDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -MTd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -LDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -MD -MDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -MT -MTd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -LD -LDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // CHECK-ASAN-DEBUGRTL: error: invalid argument