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

[Peter Collingbourne]

================
Comment at: lib/Transforms/IPO/Inliner.cpp:97
@@ -97,1 +96,3 @@
+    .addAttribute(Attribute::StackProtectStrong)
+    .addAttribute(Attribute::StackProtectReq);
   AttributeSet OldSSPAttr = AttributeSet::get(Caller->getContext(),
----------------
jfb wrote:
> Was `StackProtectReq` simply missing?
> 
> It looks like there's a lattice of stack protection mechanisms, but it's not documented AFAICT. Could you document it somewhere?
> Was StackProtectReq simply missing?

Previously we never needed to remove StackProtectReq, as it was the highest protection level.

> It looks like there's a lattice of stack protection mechanisms, but it's not documented AFAICT. Could you document it somewhere?

The lattice for the `ssp*` attributes were previously documented in http://llvm.org/docs/LangRef.html#function-attributes . I've added similar documentation for `safe_stack`.


================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:12
@@ +11,3 @@
+// and the unsafe stack (explicitly allocated and managed through the runtime
+// support library).
+//
----------------
jfb wrote:
> Add link to safe stack docs.
Done

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:16
@@ +15,3 @@
+
+#define DEBUG_TYPE "safestack"
+#include "llvm/Transforms/Instrumentation.h"
----------------
jfb wrote:
> `DEBUG_TYPE` has to be after includes to avoid nasty ODR violations.
Done

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:94
@@ +93,3 @@
+        if (!cast<const GetElementPtrInst>(I)->hasAllConstantIndices())
+          // GEP with non-constant indices can lead to memory errors.
+          return false;
----------------
jfb wrote:
> I'm assuming that's also true if the GEP is `inbounds`? Could the comment also state this.
Done

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:124
@@ +123,3 @@
+        // the object is considered safe if a pointer to it is passed to a
+        // function that only reads memory nor returns any value. This function
+        // can neither do unsafe writes itself nor capture the pointer (or
----------------
jfb wrote:
> "a function that only reads memory nor returns any value" The "nor" seems wrong here.
Fixed

================
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;
----------------
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.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:147
@@ +146,3 @@
+
+      default:
+        // The object is unsafe if it is used in any other way.
----------------
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.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:172
@@ +171,3 @@
+
+  enum { StackAlignment = 16 };
+
----------------
jfb wrote:
> Why 16? Could you also document how this is used, and how it takes into account target alignment?
Documented

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:254
@@ +253,3 @@
+        /*InsertBefore=*/nullptr,
+        /*ThreadLocalMode=*/GlobalValue::InitialExecTLSModel);
+  } else {
----------------
jfb wrote:
> This TLS model is for variables in modules that will not be loaded dynamically. This sanitizer can be used in executables, and with shared libraries that don't have safestack, but not with executables that used shared libraries with safestack?
> 
> Shouldn't this use GeneralDynamicTLSModel instead?
> 
> I'd like a comment that explains the choice.
> This TLS model is for variables in modules that will not be loaded dynamically. This sanitizer can be used in executables, and with shared libraries that don't have safestack, but not with executables that used shared libraries with safestack?

Added comment.

It can be used only in executables compiled with safestack (and in principle in DSOs where the executable is compiled with safestack, but we currently forbid this.) In each of these cases, the TLS variable lives in the main executable,  so it is safe to use initial-exec. (MSan and DFSan do something similar.)

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:301
@@ +300,3 @@
+  }
+}
+
----------------
jfb wrote:
> Do the `llvm.stacksave`/`llvm.stackrestore`, `gcroot`, `frameaddress` intrinsics need to be handled?
We handle `stacksave`/`stackrestore` in `SafeStack::moveDynamicAllocasToUnsafeStack`.

We don't currently support `gcroot`, added an error path here.

`frameaddress` currently has the expected behavior (see clang docs).

http://reviews.llvm.org/D6094

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