[PATCH] D15358: CFI runtime library (cross-DSO support)

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/cfi/cfi.cc:38
@@ +37,3 @@
+
+#define MEM_TO_SHADOW(x) (__cfi_shadow + ((x >> kShadowGranularity) << 1))
+
----------------
This can be a function rather than a macro.

================
Comment at: lib/cfi/cfi.cc:59
@@ +58,3 @@
+
+  if (cfi_check == (uptr)(-1) || cfi_check == 0) {
+    memset((void *)shadow_begin, cfi_check, shadow_end - shadow_begin);
----------------
I think the code would be easier to read if this function were inlined into its caller, mainly because of the magic numbers you are using here.

================
Comment at: lib/cfi/cfi.cc:78
@@ +77,3 @@
+
+// This is a workaround for a glibc bug:
+// https://sourceware.org/bugzilla/show_bug.cgi?id=15199
----------------
Yes, but there's also a reason why we might prefer to use this function all the time: an attacker may have corrupted the dynamic loader's internal data structures so as to make `dlsym` return some value of its choosing.

================
Comment at: lib/cfi/cfi.cc:110
@@ +109,3 @@
+
+  for (const ElfW(Sym) *p = symtab; (ElfW(Addr))p < strtab; ++p) {
+    char *name = (char*)(strtab + p->st_name);
----------------
This is reasonable if all major linkers place strtab immediately after symtab. Have you verified that this is the case?

================
Comment at: lib/cfi/cfi.cc:112
@@ +111,3 @@
+    char *name = (char*)(strtab + p->st_name);
+    if (strcmp(name, "CFI_Check") == 0) {
+      assert(p->st_info == ELF32_ST_INFO(STB_GLOBAL, STT_FUNC));
----------------
We should probably not use this as the function name, as it intrudes on the user's namespace. Instead, please use a C/C++ implementation reserved identifier (e.g. `__CFI_Check`).

================
Comment at: lib/cfi/cfi.cc:132
@@ +131,3 @@
+      // Need to fill shadow for both.
+      // FIXME: reject writable? There is no r/o segment, just rx and rw. Why?
+      uptr cur_beg = info->dlpi_addr + phdr->p_vaddr;
----------------
I believe that this is controlled by the RELRO program header.

================
Comment at: lib/cfi/cfi.cc:149
@@ +148,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE extern "C"
+void CFI_SlowPath(uptr CallSiteTypeId, void *Ptr) {
+  uptr Addr = (uptr)Ptr;
----------------
Implementation reserved identifier please.

================
Comment at: lib/cfi/cfi.cc:158
@@ +157,3 @@
+  }
+  if (cfi_check == 0xFFFFU) {
+    VReport(2, "CFI: unchecked call (shadow=FFFF): %p\n", Ptr);
----------------
It's a little confusing that this function can return both function addresses and magic numbers. I would inline `find_cfi_check_for_address` into here.

================
Comment at: lib/cfi/cfi.cc:175
@@ +174,3 @@
+
+  FlagParser ubsan_parser;
+  __ubsan::RegisterUbsanFlags(&ubsan_parser, uf);
----------------
Can we link against ubsan dependent on whether diagnostics are enabled? That would save us from having to link ubsan into production builds.

================
Comment at: test/cfi/cross-dso/icall/icall-from-dso.cpp:11
@@ +10,3 @@
+  fprintf(stderr, "=1=\n");
+  g();
+  // CHECK: =2=
----------------
Was this intended to be a positive test? This isn't a positive test because it is a direct call.

================
Comment at: test/cfi/cross-dso/icall/icall.cpp:14
@@ +13,3 @@
+  fprintf(stderr, "=1=\n");
+  f();
+  // CHECK: =2=
----------------
Same here.

================
Comment at: test/cfi/cross-dso/lit.local.cfg:1
@@ +1,1 @@
+config.substitutions = [(k, v + "-fsanitize-cfi-cross-dso ") if k == "%clangxx_cfi " else (k, v) for (k, v) in config.substitutions ]
----------------
It would be less confusing to add a new substitution instead of rewriting the existing one.

================
Comment at: test/cfi/cross-dso/simple-fail.cpp:72
@@ +71,3 @@
+    // Invisible to CFI. Test virtual call later.
+    memcpy(&a, &p, sizeof(a));
+  }
----------------
We should make the other test cases do this as well. It doesn't have to be done now though.


Repository:
  rL LLVM

http://reviews.llvm.org/D15358