This is an archive of the discontinued LLVM Phabricator instance.

stop short-circuiting the SSP code for sspstrong
AbandonedPublic

Authored by strcat on Jul 22 2016, 5:06 PM.

Details

Summary

The StackProtector::RequiresStackProtector method is supposed to add layout information for alloca instructions that need to be protected by the canary. This is supposed to protect normal local variables (including function pointers, etc.) from linear overflows.

However, this method contains an early return for sspstrong and sspreq in the code for handling calls to alloca and variable length arrays (not regular arrays, with the IR Clang generates):

// SSP-Strong: Enable protectors for any call to alloca, regardless
// of size.
if (Strong)
  return true;

The method has special handling for sspstrong/sspreq following this early return, but it's not being used. It ends up returning early, resulting in the function being protected with a canary but without marking the arrays it's trying to protect (not only the alloca/VLA triggering the issue) so they get treated as normal local variables.

Sample code:

#include <string.h>
#include <alloca.h>

int foo(char *bar) {
    char *buf = alloca(20);
    strcpy(buf, bar);
    return strlen(buf);
}

Compiler output at -O0:

--- old_x86.s	2016-07-22 08:44:37.534862251 -0400
+++ new_x86.s	2016-07-22 08:44:18.778486803 -0400
@@ -17,12 +17,12 @@
 	subq	$48, %rsp
 	movq	%fs:40, %rax
 	movq	%rax, -8(%rbp)
-	movq	%rdi, -24(%rbp)
-	leaq	-44(%rbp), %rdi
-	movq	%rdi, -16(%rbp)
-	movq	-24(%rbp), %rsi
+	movq	%rdi, -48(%rbp)
+	leaq	-28(%rbp), %rdi
+	movq	%rdi, -40(%rbp)
+	movq	-48(%rbp), %rsi
 	callq	strcpy
-	movq	-16(%rbp), %rdi
+	movq	-40(%rbp), %rdi
 	callq	strlen
 	movq	%fs:40, %rcx
 	cmpq	-8(%rbp), %rcx

Diff Detail

Event Timeline

strcat updated this revision to Diff 65195.Jul 22 2016, 5:06 PM
strcat retitled this revision from to stop short-circuiting the SSP code for sspstrong.
strcat updated this object.
strcat added reviewers: hfinkel, probinson, void.
strcat updated this object.
strcat updated this object.
strcat updated this object.Jul 22 2016, 5:08 PM
strcat updated this object.
strcat updated this object.
strcat updated this object.Jul 22 2016, 5:12 PM
strcat added a reviewer: llvm-commits.
probinson edited edge metadata.Jul 22 2016, 5:42 PM

llvm-commits should be a subscriber, not a reviewer. I haven't seen any notice about this review show up on llvm-commits so maybe you should abandon it and start over.

Also: needs a test.

strcat added a subscriber: llvm-commits.

Yeah, I guess I need to remake it with llvm-commits as a subscriber from the start.

I'm not sure how to make a test for it though. The change in the assembly is fairly subtle.