[PATCH] D18846: [safestack] Add canary to unsafe stack frames

[Tim Shen via llvm-commits]

timshen added inline comments.

================
Comment at: lib/CodeGen/SafeStack.cpp:399
@@ +398,3 @@
+    StackGuardVar =
+        F.getParent()->getOrInsertGlobal("__stack_chk_guard", StackPtrTy);
+  return IRB.CreateLoad(StackGuardVar, "StackGuard");
----------------
eugenis wrote:
> timshen wrote:
> > timshen wrote:
> > > Is this going to work correctly, for say, PowerPC? As of my knowledge on powerpc64le-linux-gnu, there is no "__stack_chk_guard", but similarly has a data member in TCB.
> > > 
> > > On SSP side I'm actively working on supporting PowerPC - more generally, allowing backends to lower LOAD_STACK_GUARD node manually.
> > To be specific, powerpc64le-linux-gnu is similar to AArch64, which has a stack guard as data member in TCB.
> Then it should implement getStackCookieLocation - or is it only supported in the SelectionDAG code pass now?
Yes, the plan is to only support it in SelectionDAG, because SelectionDAG can pick up tail call optimization (See StackProtectorDescriptor's comment).

Without SafeStack in mind, I was trying to minimize the use of IR approach, since the only reason we need to keep IR form is FastISel.

================
Comment at: lib/CodeGen/SafeStack.cpp:504
@@ +503,3 @@
+  Constant *StackChkFail = F.getParent()->getOrInsertFunction(
+      "__stack_chk_fail", IRB.getVoidTy(), nullptr);
+  IRBFail.CreateCall(StackChkFail, {});
----------------
eugenis wrote:
> timshen wrote:
> > timshen wrote:
> > > OpenBSD doesn't have __stack_chk_fail. It has StackProtector::CreateFailBB.
> > > 
> > > I wonder if it's easy to share some code between SSP and safestack, though I have no idea what safestack is doing.
> > s/It has StackProtector::CreateFailBB/See StackProtector::CreateFailBB/.
> SafeStack maintains a second stack, with the stack pointer either in a thread-local variable or a fixed TLS slot, and moves some locals to that stack.
> 
> Anything that may overflow is on the second stack. StackProtector + SafeStack should apply StackProtector cookies to that second stack, and not to the system stack.
> 
> It would be great to share more code between the two passes.
> 
> Would it be possible to extend the SDAG stuff to handle this case?
> 
Thanks for the explaining!

It sounds like you want to teach StackProtector to allocate the canary in a different place (aka the second stack). It's currently created in StackProtector::CreatePrologue. Is it possible to call moveStaticAllocasToUnsafeStack to move the slot to the second stack?


Repository:
  rL LLVM

http://reviews.llvm.org/D18846