[cfi] Safe handling of unaddressable vtable pointers (compiler-rt).

[eugenis]

Summary

Avoid crashing when printing diagnostics for vtable-related CFI
errors. In diagnostic mode, the frontend does an additional check of
the vtable pointer against the set of all known vtable addresses and
lets the runtime handler know if it is safe to inspect the vtable.

[samsonov]

LGTM

[pcc]

I have comments.

[pcc]

This regresses the case where the target vtable is uninstrumented. Please add a test case showing that we at least print the name of the module in that case.

[pcc]

test/cfi/cross-dso/target_out_of_bounds.cpp

41

Why not just memset(p, 0, sizeof(A));?

51–55

Likewise memset(p, 0xFE, sizeof(A));

[eugenis]

test/cfi/cross-dso/target_out_of_bounds.cpp

41

Because that would test a different thing.

51–55

That, again, would be quite different.
This code is testing an invalid TypeInfo pointer, not an invalid vptr.

[pcc]

test/cfi/cross-dso/target_out_of_bounds.cpp

41

Okay, I see. Please give the variables better names to make this code less confusing. Same for the other block. Please also change the loops to just an assignment of the vtable pointer.

[eugenis]

added the module name to the invalid-vtable report

[pcc]

LGTM with nit

test/cfi/target_uninstrumented.cpp

34 ↗

(On Diff #46833)

Shouldn't you check specifically that the name matches the DSO name? Maybe have your test output to %T/target-uninstrumented.so and have this match against that?

[eugenis]

test/cfi/cross-dso/target_out_of_bounds.cpp

41

done

test/cfi/target_uninstrumented.cpp

34 ↗

(On Diff #46833)

I've made the check a bit stricter. I don't think there is any benefit in making it stricter than that, and it could make the test fragile.

[pcc]

test/cfi/target_uninstrumented.cpp

35 ↗

(On Diff #46837)

%T gives you a directory name so you can just have your test output to a subpath of that like in my example and you won't have any of the tmp stuff in your path name.

[pcc]

lgtm

[eugenis]

http://llvm.org/viewvc/llvm-project?rev=259717&view=rev

lib/cfi/cfi.cc

ALWAYS_INLINE void CfiSlowPathCommon(u64 CallSiteTypeId, void *Ptr,
   uptr Addr = (uptr)Ptr;
   VReport(3, "__cfi_slowpath: %llx, %p\n", CallSiteTypeId, Ptr);
   ShadowValue sv = ShadowValue::load(Addr);
   if (sv.is_invalid()) {
     VReport(1, "CFI: invalid memory region for a check target: %p\n", Ptr);
 #ifdef CFI_ENABLE_DIAG
     if (DiagData) {
       __ubsan_handle_cfi_check_fail(
-          reinterpret_cast<__ubsan::CFICheckFailData *>(DiagData), Addr);
+          reinterpret_cast<__ubsan::CFICheckFailData *>(DiagData), Addr, false);
       return;
     }
 #endif
     Trap();
   }
   if (sv.is_unchecked()) {
     VReport(2, "CFI: unchecked call (shadow=FFFF): %p\n", Ptr);
     return;

lib/ubsan/ubsan_handlers.h

 
 struct CFICheckFailData {
   CFITypeCheckKind CheckKind;
   SourceLocation Loc;
   const TypeDescriptor &Type;
 };
 
 /// \brief Handle control flow integrity failures.
-RECOVERABLE(cfi_check_fail, CFICheckFailData *Data, ValueHandle Function)
-
+RECOVERABLE(cfi_check_fail, CFICheckFailData *Data, ValueHandle Function,
+            uptr VtableIsValid)
 }
 
 #endif // UBSAN_HANDLERS_H

lib/ubsan/ubsan_handlers.cc

if (!FName)
     FName = "(unknown)";
   Diag(FLoc, DL_Note, "%0 defined here") << FName;
 }
 
 namespace __ubsan {
 #ifdef UBSAN_CAN_USE_CXXABI
 SANITIZER_WEAK_ATTRIBUTE
 void HandleCFIBadType(CFICheckFailData *Data, ValueHandle Vtable,
-                      ReportOptions Opts);
+                      bool ValidVtable, ReportOptions Opts);
 #else
 static void HandleCFIBadType(CFICheckFailData *Data, ValueHandle Vtable,
-                             ReportOptions Opts) {
+                             bool ValidVtable, ReportOptions Opts) {
   Die();
 }
 #endif
 }  // namespace __ubsan
 
 void __ubsan::__ubsan_handle_cfi_check_fail(CFICheckFailData *Data,
-                                           ValueHandle Value) {
+                                            ValueHandle Value,
+                                            uptr ValidVtable) {
   GET_REPORT_OPTIONS(false);
   if (Data->CheckKind == CFITCK_ICall)
     handleCFIBadIcall(Data, Value, Opts);
   else
-    HandleCFIBadType(Data, Value, Opts);
+    HandleCFIBadType(Data, Value, ValidVtable, Opts);
 }
 
 void __ubsan::__ubsan_handle_cfi_check_fail_abort(CFICheckFailData *Data,
-                                                 ValueHandle Value) {
+                                                  ValueHandle Value,
+                                                  uptr ValidVtable) {
   GET_REPORT_OPTIONS(true);
   if (Data->CheckKind == CFITCK_ICall)
     handleCFIBadIcall(Data, Value, Opts);
   else
-    HandleCFIBadType(Data, Value, Opts);
+    HandleCFIBadType(Data, Value, ValidVtable, Opts);
   Die();
 }
 
 #endif  // CAN_SANITIZE_UB

lib/ubsan/ubsan_handlers_cxx.cc

void __ubsan::__ubsan_handle_dynamic_type_cache_miss_abort(
   // Note: -fsanitize=vptr is always recoverable.
   GET_REPORT_OPTIONS(false);
   if (HandleDynamicTypeCacheMiss(Data, Pointer, Hash, Opts))
     Die();
 }
 
 namespace __ubsan {
 void HandleCFIBadType(CFICheckFailData *Data, ValueHandle Vtable,
-                      ReportOptions Opts) {
+                      bool ValidVtable, ReportOptions Opts) {
   SourceLocation Loc = Data->Loc.acquire();
   ErrorType ET = ErrorType::CFIBadType;
 
   if (ignoreReport(Loc, Opts, ET))
     return;
 
   ScopedReport R(Opts, Loc, ET);
-  DynamicTypeInfo DTI = getDynamicTypeInfoFromVtable((void *)Vtable);
+  DynamicTypeInfo DTI = ValidVtable
+                            ? getDynamicTypeInfoFromVtable((void *)Vtable)
+                            : DynamicTypeInfo(0, 0, 0);
 
   const char *CheckKindStr;
   switch (Data->CheckKind) {
   case CFITCK_VCall:
     CheckKindStr = "virtual call";
     break;
   case CFITCK_NVCall:
     CheckKindStr = "non-virtual call";

test/cfi/cross-dso/target_out_of_bounds.cpp

 // RUN: %clangxx_cfi_dso_diag %s -o %t
-// RUN: %expect_crash %t 2>&1 | FileCheck %s
+// RUN: %t zero 2>&1 | FileCheck --check-prefix=CHECK-ZERO %s
+// RUN: %t unaddressable 2>&1 | FileCheck --check-prefix=CHECK-UNADDR %s
+// RUN: %t 2>&1 | FileCheck --check-prefix=CHECK-TYPEINFO %s
+
+// RUN: %clangxx_cfi_diag %s -o %t2
+// RUN: %t2 zero 2>&1 | FileCheck --check-prefix=CHECK-ZERO %s
+// RUN: %t2 unaddressable 2>&1 | FileCheck --check-prefix=CHECK-UNADDR %s
+// RUN: %t2 2>&1 | FileCheck --check-prefix=CHECK-TYPEINFO %s
 
 // REQUIRES: cxxabi
 
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 
 struct A {
   virtual void f();
 };
 
 void A::f() {}
 
 int main(int argc, char *argv[]) {
+  char *volatile p = reinterpret_cast<char *>(new A());
+  if (argc > 1 && strcmp(argv[1], "unaddressable") == 0) {
+    // Create an object with a vtable in an unaddressable memory region.
+    memset(p, 0xFE, sizeof(A));
+    // CHECK-UNADDR: runtime error: control flow integrity check for type 'A' failed during cast
+    // CHECK-UNADDR: fefefefe{{.*}}: note: invalid vtable
+    // CHECK-UNADDR: <memory cannot be printed>
+    // CHECK-UNADDR: runtime error: control flow integrity check for type 'A' failed during cast
+    // CHECK-UNADDR: fefefefe{{.*}}: note: invalid vtable
+    // CHECK-UNADDR: <memory cannot be printed>
+  } else if (argc > 1 && strcmp(argv[1], "zero") == 0) {
     // Create an object with a vtable outside of any known DSO, but still in an
-  // addressable area. Current implementation of handlers in UBSan is not robust
-  // enough to handle unaddressable vtables. TODO: fix this.
+    // addressable area.
     void *empty = calloc(1, 128);
     uintptr_t v = (uintptr_t)empty + 64;
-  char *volatile p = reinterpret_cast<char *>(new A());
     for (uintptr_t *q = (uintptr_t *)p; q < (uintptr_t *)(p + sizeof(A)); ++q)
       *q = v;
+    // CHECK-ZERO: runtime error: control flow integrity check for type 'A' failed during cast
+    // CHECK-ZERO: note: invalid vtable
+    // CHECK-ZERO: 00 00 00 00 00 00 00 00
+    // CHECK-ZERO: runtime error: control flow integrity check for type 'A' failed during cast
+    // CHECK-ZERO: note: invalid vtable
+    // CHECK-ZERO: 00 00 00 00 00 00 00 00
+  } else {
+    // Create an object with a seemingly fine vtable, but with an unaddressable
+    // typeinfo pointer.
+    void *empty = calloc(1, 128);
+    memset(empty, 0xFE, 128);
+    uintptr_t v = (uintptr_t)empty + 64;
+    for (uintptr_t *q = (uintptr_t *)p; q < (uintptr_t *)(p + sizeof(A)); ++q)
+      *q = v;
+    // CHECK-TYPEINFO: runtime error: control flow integrity check for type 'A' failed during cast
+    // CHECK-TYPEINFO: note: invalid vtable
+    // CHECK-TYPEINFO: fe fe fe fe fe fe fe fe
+    // CHECK-TYPEINFO: runtime error: control flow integrity check for type 'A' failed during cast
+    // CHECK-TYPEINFO: note: invalid vtable
+    // CHECK-TYPEINFO: fe fe fe fe fe fe fe fe
+  }
 
-  // CHECK: runtime error: control flow integrity check for type 'A' failed during cast
   A *volatile pa = reinterpret_cast<A *>(p);
-
-  // CHECK: untime error: control flow integrity check for type 'A' failed during virtual call
-  pa->f();
+  pa = reinterpret_cast<A *>(p);
 }