[PATCH] Protection against stack-based memory corruption errors using SafeStack

[JF Bastien]

I like the technical approach overall, but the testing strategy worries me. Specifically, this is now at the IR level and I have a hard time convincing myself that the safe stack's location isn't moved to other registers, or stored on the unsafe stack or heap. This isn't just about the current implementation: I worry of breaking safe stack as LLVM gains new features (e.g. GC roots breaks it, adding similar features would silently do the same), or with different runtimes.

The only way I can think of convincing myself is by using binary instrumentation: keep track of safe stack ranges for each thread at runtime, and use QEMU/Pin/Valgrind/... to find any leaks. This will have false positives, but assuming there's no stack-cookie-like leaks then the lack of any warnings would mean that the safe stacks' locations aren't leaking. This tool could be run on the test suite.

I don't want to block checking in safe stack on such a tool, but I would be wary of using it as a security feature without having tested it.

Thoughts?


================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:340
@@ +339,3 @@
+    // throughout the function. For now we store it in an alloca.
+    DynamicTop = IRB.CreateAlloca(StackPtrTy, 0, "unsafe_stack_dynamic_ptr");
+
----------------
`/* ArraySize= */ nullptr`

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:426
@@ +425,3 @@
+    StaticOffset += Size;
+    StaticOffset = (StaticOffset + Align - 1) / Align * Align;
+
----------------
Use `alignAddr` from `MathExtras.h` instead.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:444
@@ +443,3 @@
+  StaticOffset =
+      (StaticOffset + StackAlignment - 1) / StackAlignment * StackAlignment;
+
----------------
`alignAddr`

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:129
@@ +128,3 @@
+        // exception or not depending on the input value).
+        if (CS.onlyReadsMemory() && I->getType()->isVoidTy())
+          continue;
----------------
pcc wrote:
> jfb wrote:
> > Are there any read-only functions with no return value? I'm not sure that makes much sense, and I don't see a test fo it.
> > Are there any read-only functions with no return value?
> 
> `void noop() {}` ? :)
> 
> > I don't see a test fo it.
> 
> Added.
I asked the wrong question :-)

Are there any read-only functions with no return value which also capture pointers? The example you added looks like what I expected... but I don't get what such a function could do. It seems pretty useless! If so then it doesn't seem worth handling it specially here (why optimize something nobody in their right mind would do?).

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:147
@@ +146,3 @@
+
+      default:
+        // The object is unsafe if it is used in any other way.
----------------
pcc wrote:
> jfb wrote:
> > Don't you end up seeing uses of alloca through intrinsics such as `memset` and `memcpy`? They can be added if needed, but since these two act the same as load/store I'd think that they are safe.
> Yes, but let's add support for these intrinsics in a separate change.
Sounds good. Could you add a FIXME for this?

http://reviews.llvm.org/D6094

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