[PATCH] Protection against stack-based memory corruption errors using SafeStack: compiler-rt runtime support library

Peter Collingbourne peter at pcc.me.uk
Tue May 26 14:35:03 PDT 2015 

================
Comment at: cmake/config-ix.cmake:259
@@ -257,2 +258,3 @@
   mipsel mips64 mips64el powerpc64 powerpc64le)
+filter_available_targets(SAFESTACK_SUPPORTED_ARCH x86_64 i386 i686)
 
----------------
jfb wrote:
> I haven't been through the LLVM-side code yet, but what prevents other architectures from being supported? IIUC nothing is x86 specific since it's in ``lib/Transforms``, right?
Not much as far as I know, but I understand that the general policy for these clauses is that the person who adds support for a particular architecture gets to add their architecture to the list.

================
Comment at: cmake/config-ix.cmake:340
@@ +339,3 @@
+if (SAFESTACK_SUPPORTED_ARCH AND
+    OS_NAME MATCHES "Darwin|Linux|FreeBSD")
+  set(COMPILER_RT_HAS_SAFESTACK TRUE)
----------------
jfb wrote:
> What's missing for Windows support? Thread and TLS support? It's the most significant platform for Chrome (well, and Android), and we're looking forward to LLVM support on Windows. Not having SafeStack would be sad. This can be a different patch, but I think it's important.
> What's missing for Windows support? Thread and TLS support?

LLVM already supports TLS on Windows (I believe), but we will have to find some way to intercept thread creation to allocate both stacks.

> This can be a different patch, but I think it's important.

Agree.

================
Comment at: lib/safestack/safestack.cc:44
@@ +43,3 @@
+  __attribute__((visibility ("default")))
+  __thread void  *__safestack_unsafe_stack_ptr = 0;
+}
----------------
jfb wrote:
> `nullptr` here and other pointers below.
Done

================
Comment at: lib/safestack/safestack.cc:54
@@ +53,3 @@
+static inline void *unsafe_stack_alloc(size_t size, size_t guard) {
+  void *addr = MmapOrDie(size + guard, "unsafe_stack_alloc");
+  MprotectNoAccess((uptr)addr, (uptr)guard);
----------------
jfb wrote:
> It's probably not a huge issue for the unsafe stack, but I'd rather check that `size + guard` doesn't overflow `uptr` before calling `MmapOrDie`. The same applies in other parts of the code below.
Done

================
Comment at: lib/safestack/safestack.cc:71
@@ +70,3 @@
+  if (unsafe_stack_start) {
+    (void)UnmapOrDie(
+      (char*) unsafe_stack_start - unsafe_stack_guard,
----------------
jfb wrote:
> Drop the `(void)` case.
Done

================
Comment at: lib/safestack/safestack.cc:106
@@ +105,3 @@
+  // FIXME: we can do this only any other specific key is set by
+  // intersepting the pthread_setspecific function itself
+  pthread_setspecific(thread_cleanup_key, (void*) 1);
----------------
jfb wrote:
> s/intersepting/intercepting/
Done

================
Comment at: lib/safestack/safestack.cc:109
@@ +108,3 @@
+
+  // Start the original thread rutine
+  return start_routine(start_routine_arg);
----------------
jfb wrote:
> s/rutine/routine/
> 
> Though I'm not sure the comment is really helping me understand much here, I'd just drop it.
Removed

================
Comment at: lib/safestack/safestack.cc:174
@@ +173,3 @@
+  if (initialized)
+    return;
+
----------------
jfb wrote:
> Is this initialization check useful if `__attribute__((constructor(0)))` was used? Can this be done concurrently, or is it just called multiple times from the same thread of execution? If concurrent then initialization is racy.
> Is this initialization check useful if __attribute__((constructor(0))) was used?

I don't think it is. Removed.

(Regarding Vova's comment, we don't currently support linking SafeStack into DSOs.)

================
Comment at: lib/safestack/safestack.cc:225
@@ +224,3 @@
+void *__get_safe_stack_ptr() {
+  return (char*) __builtin_frame_address(0) + 2*sizeof(void*);
+}
----------------
jfb wrote:
> Please add a comment on why `2*sizeof(void*)` is correct.
Removed

================
Comment at: test/safestack/CMakeLists.txt:9
@@ +8,3 @@
+    list(APPEND SAFESTACK_TEST_DEPS
+      LLVMgold
+    )
----------------
jfb wrote:
> Does this force-build gold for this test? Or does it refuse to build the test if gold wasn't built? Also, comment on why PIC and+binutils_incdir require gold for testing.
This (and the Apple stuff below) adds the relevant LTO plugin as an optional test dependency of the test suite if the plugin is being built. This is required to test the scenario where the program is compiled with LTO; see `lto.c`. The tests should still run with the exception of `lto.c` if LTO is not supported.

================
Comment at: test/safestack/CMakeLists.txt:14
@@ +13,3 @@
+    list(APPEND SAFESTACK_TEST_DEPS
+      LTO
+    )
----------------
jfb wrote:
> Why does Apple require LTO for testing?
See above.

================
Comment at: test/safestack/check-buffer-copy.c:9
@@ +8,3 @@
+  int i;
+  char buffer[128];
+
----------------
jfb wrote:
> Could you do the same test with a VLA?
Done

================
Comment at: test/safestack/check-buffer-copy.c:14
@@ +13,3 @@
+    buffer[i] = argv[0][i];
+  buffer[i] = '\0';
+
----------------
jfb wrote:
> Writing and then immediately reading back is pretty brittle if the optimizer decides to kick in and prove the code trivially dead. Could you add some optimizer smarts defeat mechanism here? I'm not sure if compiler-rt has a preferred method of doing this (asm volatile, barrier, volatile, separate noinline call, ...).
Done

================
Comment at: test/safestack/check-overflow.c:10
@@ +9,3 @@
+
+void fct(int *buffer)
+{
----------------
jfb wrote:
> noinline would be appropriate, though IPO could still kick in. Maybe slap a volatile on there too?
Done

================
Comment at: test/safestack/check-overflow.c:13
@@ +12,3 @@
+  buffer[-1] = 36;
+  buffer[6] = 36;
+}
----------------
jfb wrote:
> I'd rather memset a bigger range which includes the buffer itself than just changing two values.
Done

================
Comment at: test/safestack/check-overflow.c:22
@@ +21,3 @@
+  fct(buffer);
+  return value1 != 42 || value2 != 42;
+}
----------------
jfb wrote:
> The optimizer knows that `value1` and `value2` don't escape `main`. This check is trivially true.
Yes, but I'm not sure that there's much we can do to defeat optimization in this case. (We want `value1` and `value2` to live on the safe stack, but inserting a call to `break_optimization` would move them to the unsafe stack.) Perhaps it would be best to simply rely on `-O0` doing the right thing in this case.

================
Comment at: test/safestack/check-pthread.c:22
@@ +21,3 @@
+  char buffer[8096]; // two pages
+  memset(buffer, val, sizeof (buffer));
+
----------------
jfb wrote:
> The buffer allocation and memset are dead code and can be eliminated.
I think the idea behind this part of the test is to check that the thread has an addressable unsafe stack. I've added a call to `break_optimization` to make sure this code is run.

http://reviews.llvm.org/D6096

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/

More information about the cfe-commits mailing list