This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pcc on Nov 3 2014, 9:44 AM.

Details

Summary

This patch adds runtime support for the Safe Stack protection to compiler-rt (see http://reviews.llvm.org/D6094 for the detailed description of the Safe Stack).

This patch is our implementation of the safe stack on top of the current SVN HEAD of compiler-rt (r220991). The patch adds basic runtime support for the safe stack to compiler-rt that manages unsafe stack allocation/deallocation for each thread.

Diff Detail

Event Timeline

ksvladimir updated this revision to Diff 15719.Nov 3 2014, 9:44 AM
ksvladimir retitled this revision from to Protection against stack-based memory corruption errors using SafeStack: compiler-rt runtime support library.
ksvladimir updated this object.
ksvladimir edited the test plan for this revision. (Show Details)
ksvladimir added a reviewer: theraven.
ksvladimir added a subscriber: Unknown Object (MLST).
theraven edited edge metadata.Nov 3 2014, 10:20 AM

I've done a quick review, I'll do a more detailed follow-up once the comments inline have been addressed.

lib/safestack/safestack.cc
35

Yes. It must also be dynamic to respect pthread stack limits and system-wide rlimits.

76

There are magic numbers here again. Please reference either an ABI document or equivalent that indicates that these are safe locations to use.

146

Why is this Linux only? If the goal is to avoid LD_PRELOAD and similar interceptions, then it should be done everywhere (possibly with the exception of Darwin)

153

On FreeBSD, MAP_STACK should also be passed. Not sure about other BSDs. In general, however, these checks should be of the form:
#ifdef MAP_STACK

MAP_STACK

#endif
Not #if defined(linux)

256

This seems somewhat fragile. It doesn't guarantee that the other destructors won't be run after this, only that they get several opportunities to run before.

272

Why is this not on Linux?

281

This should check RLIMIT_STACK.

328

Why noinline? It is externally visible, so a non-inline version will be produced anyway. Why not let LTO builds inline it? Or is the noinline attribute because this function must be compiled *without* the safe stack support itself? If so, document it (or perhaps add the no_safestack attribute instead, to prevent it from being inlined in unsafe contexts).

341

I believe that this should work on most ELF platforms.

colinl added a subscriber: colinl.Nov 3 2014, 10:25 AM

Hi David,

Thanks a lot for the review, your feedback is very valuable! We will apply
it and resubmit the patch as soon as possible.

Two of your points seem to be more challenging to me to address, given my
limited experience with the libc internals:

Comment at: lib/safestack/safestack.cc:75
@@ +74,3 @@
+
+// The following locations are platform-specific

+# define GET_UNSAFE_STACK_PTR() (void*) THREAD_GETMEM_L(0x280)

There are magic numbers here again. Please reference either an ABI
document or equivalent that indicates that these are safe locations to use.

Do you know of any ABI documents that might be relevant to this? On
FreeBSD, we simply added corresponding fields to the tcb struct in libc
(in lib/libthr/arch/amd64/include/pthread_md.h). Do you think it would be
safer to use thread-local variables on Linux for now ? It would impact the
performance though. Perhaps in the future the glibc can be modified to add
the required fields to its tcb structure as well.

Comment at: lib/safestack/safestack.cc:255
@@ +254,3 @@
+void thread_cleanup_handler(void* _iter) {
+ // We want to free the unsafe stack only after all other destructors

+ // have already run. We force this function to be called multiple times.

This seems somewhat fragile. It doesn't guarantee that the other
destructors won't be run after this, only that they get several
opportunities to run before.

Yes, I was worried about this as well. It worked in our tests, but can
certainly break in some cases. The clean solution is to perform this
cleanup in the pthread code itself, which is what we did on FreeBSD. We
hope it will eventually be added to glibc as well. Do you have any better
suggestions on how to handle this in the meantime?

Thanks!

  • Vova
kcc added a subscriber: kcc.Nov 3 2014, 4:13 PM
kcc added inline comments.
lib/safestack/safestack.cc
19

You may find lots of utilities in sanitizer_common useful here.
E.g. try CHECK, CHECK_EQ, etc instead of assert.
In general, I'd like to see more code reuse between safe_stack and sanitizers.

43

Why not simply force -lpthread in the driver?

52

I might be missing something, but can't we just use TLS to store the second stack?

146

Don't you want to use internal_mmap from sanitizer_common/sanitizer_libc.h?

227

Where possible, please reuse functions from sanitizer_common, e.g. here you may try GetThreadStackTopAndBottom

267

please use static for anything that is is not supposed to be called/used from another module.

306

why NDEBUG?
I would prefer tnot to have a separate build for compiler-rt

308

can you use SANITIZER_INTERFACE_ATTRIBUTE?

emaste added a subscriber: emaste.Nov 3 2014, 8:43 PM
theraven added inline comments.Nov 4 2014, 1:54 AM
lib/safestack/safestack.cc
43

The overhead of adding -pthread (-lpthread is non-portable) to single-threaded programs is measurably greater than the overhead of safe stack. We (current hat: FreeBSD) are willing to ship packages with SSP by default and would be very willing to ship packages with safe-stack, but if it forced -pthread then the overhead would be more than we'd be willing to accept.

52

C11 TLS does not have a way of attaching destructors. C++11 does (although the standard is undefined in the presence of shared library loading and unloading, which makes it less useful), but does not have any way of ordering the destruction.

ksvladimir updated this revision to Diff 15777.Nov 4 2014, 11:20 AM
ksvladimir edited edge metadata.

Addresses comments on the previous submission.

You can find the detailed changelog since the previous submission in our repo: https://github.com/cpi-llvm/compiler-rt/commits/safestack-r221153

lib/safestack/safestack.cc
52

The main reason of putting the unsafe stack pointer in the tcb directly is performance: that way, the unsafe stack pointer can be loaded/stored with just one memory access. It's exactly the same optimization as the existing stack protector uses (see X86TargetLowering::getStackCookieLocation in X86ISelLowering.cpp:1925).

Following David's suggestion, I will remove this optimization on Linux for now, until it can be properly implemented on the glibc side as well. I plan to only keep it on Darwin (where it seems to be cleaner, as I will explain in the comments). I will use TLS for unsafe_stack_(start|size|guard) on all platforms, as those are not performance critical in any way.

amanone added a subscriber: amanone.Nov 6 2014, 2:24 AM
ksvladimir updated this revision to Diff 15993.Nov 10 2014, 9:47 AM

Move mmap's #ifdef out of the functions to improve readability.

ksvladimir updated this revision to Diff 16079.Nov 12 2014, 2:34 AM

Added tests for the safestack runtime support library.

abique added a subscriber: abique.Nov 14 2014, 12:16 AM
ksvladimir edited the test plan for this revision. (Show Details)

Simplified safestack public symbol names.

pcc commandeered this revision.Apr 23 2015, 3:42 PM
pcc added a reviewer: ksvladimir.
pcc updated this revision to Diff 24339.Apr 23 2015, 3:45 PM

Refresh

kcc added a comment.May 5 2015, 4:33 PM

a few random comments, not a thorough review yet.

lib/safestack/Makefile.mk
1

I don't think we want to support configure/make

lib/safestack/safestack.cc
56

can this code use more of sanitizer_common?
(here and in the rest of this file)

91

Why?
The recent OSX should support TLS well (or not?)

test/safestack/check-buffer-copy.c
1

(for all tests) please add a one-line test description

pcc updated this revision to Diff 25098.May 6 2015, 3:44 PM
  • Use internal_{mmap,munmap,mprotect} functions in sanitizer_common
  • Remove Darwin platform-specific TLS code
  • Tweak comment
  • Remove Makefile stuff
  • Add test descriptions
  • Rename flag to -fsanitize=safe-stack
pcc added inline comments.May 6 2015, 3:45 PM
lib/safestack/Makefile.mk
1

Removed.

lib/safestack/safestack.cc
56

Yep, this now uses internal_mmap/internal_munmap and internal_mprotect below.

91

This now uses a __thread variable on all platforms.

According to Laszlo this was done solely for efficiency reasons, so we should be fine using __thread at least to begin with. I imagine that if the overhead matters in practice we can try to persuade the relevant people at Apple to allocate a TLS slot for us.

test/safestack/check-buffer-copy.c
1

Done

samsonov added inline comments.
lib/CMakeLists.txt
12

I thought you need to add this if COMPILER_RT_HAS_SAFESTACK is true.

test/safestack/CMakeLists.txt
6

I believe you also need ${SANIITIZER_COMMON_LIT_TEST_DEPS}

test/safestack/check-overflow.c
6

use not (here and below)

test/safestack/lit.site.cfg.in
6

You don't use this attribute anywhere, delete it.

A couple of high-level comments:

I don't really like making this a sanitizer. It is supposed to be useable in the same places stack canaries are used (i.e. in production, with a view to enabling it for 100% of code). Making it an -fsanitize option makes it seem like a debugging tool, not a mitigation tool.

I'm also not 100% convinced by the compiler-rt stuff. This needs to be in libc. What happens when a program dlopens libpthread? The pthread_create symbol won't exist on library load, so it looks as if your interceptor code will just see a null pointer. Then when the first thread is created, it will die horribly.

pcc updated this revision to Diff 25221.May 7 2015, 1:12 PM
  • Address review comments
lib/CMakeLists.txt
12

Done

test/safestack/CMakeLists.txt
6

Done

test/safestack/check-overflow.c
6

Done

test/safestack/lit.site.cfg.in
6

Done

samsonov accepted this revision.May 7 2015, 5:03 PM
samsonov added a reviewer: samsonov.

LGTM for CMake/lit part.

test/safestack/CMakeLists.txt
7

Looks like clang is already contained in SANITIZER_COMMON_LIT_TEST_DEPS

This revision is now accepted and ready to land.May 7 2015, 5:03 PM
kcc added inline comments.May 18 2015, 12:58 PM
CMakeLists.txt
210

is this still valid?
Don't we use -fsanitize= syntax?

lib/safestack/safestack.cc
39

please no macros where constants work

46

do you really need this?

58

Mmm. I don't like that.
Why can't we just ensure that pthread is linked?

81

no macros, please, unless you have too, in which case describe in comments why.

102

I would use MmapOrDie (or create a similar one in sanitizer_common/sanitizer_posix.cc)
and get rid of mmap includes in this file.

115

use CHECK_EQ

127

UnmapOrDie

205

use CHECK

277

please make this similar to other sanitizers (SANITIZER_CAN_USE_PREINIT_ARRAY).

pcc updated this revision to Diff 26088.May 19 2015, 1:31 PM
pcc edited edge metadata.
  • Use constants, simplfy pthread stuff
  • Remove more macros
  • Use MmapOrDie etc.
  • Use CHECK_* instead of assert
CMakeLists.txt
210

Updated.

lib/safestack/safestack.cc
39

Done

46

Removed; they are hints and are not required for correctness. We can bring them back if benchmarks show a clear benefit.

58

My understanding is that on some platforms (FreeBSD?) linking the pthread library has a performance impact.

We could probably consider moving pthread stuff to a separate library which is only linked when pthread is linked. I guess for now we can use the pthread symbols unconditionally. Then any later change will mostly be a matter of moving code around.

81

Removed

102

Done

115

Done

127

Done

205

Done

277

Done

test/safestack/CMakeLists.txt
7

Done

kcc added inline comments.May 19 2015, 2:05 PM
lib/safestack/safestack.cc
21

Please check if you still need so many headers. Remove anything you don't need.

65

MmapOrDie dies if it fails. You should not need the check

84

do you still need this comment?

test/safestack/check-pthread-cleanup.c
11

kBufferSize

pcc updated this revision to Diff 26092.May 19 2015, 2:19 PM
  • remove comment
  • Remove MAP_FAILED check, remove unused #includes
  • Use enum constant
lib/safestack/safestack.cc
21

Done

65

Removed

84

Removed

test/safestack/check-pthread-cleanup.c
11

Done

pcc updated this revision to Diff 26095.May 19 2015, 2:29 PM
  • Remove C++ marker
kcc added a comment.May 19 2015, 3:49 PM

LGTM

Once everything is committed, please make sure this is tested on our bots.

ksvladimir added inline comments.May 19 2015, 4:05 PM
lib/safestack/safestack.cc
167

This should also use SANITIZER_CAN_USE_PREINIT_ARRAY: attribute((constructor(0))) should be added whenever preinit_array is not used.

pcc updated this revision to Diff 26106.May 19 2015, 4:44 PM
  • Use SANITIZER_CAN_USE_PREINIT_ARRAY in one more place
lib/safestack/safestack.cc
167

Done

pcc updated this revision to Diff 26108.May 19 2015, 4:57 PM
  • Add LTO test
pcc updated this revision to Diff 26109.May 19 2015, 4:59 PM
  • Fix copy-pasteo
jfb added a subscriber: jfb.May 21 2015, 2:47 PM

Could you also run clang-format on the code? Preferably separate from other changes to the diff history is easier to read.

cmake/config-ix.cmake
246

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?

327

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.

lib/safestack/safestack.cc
45

nullptr here and other pointers below.

55

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.

72

Drop the (void) case.

107

s/intersepting/intercepting/

110

s/rutine/routine/

Though I'm not sure the comment is really helping me understand much here, I'd just drop it.

175

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.

226

Please add a comment on why 2*sizeof(void*) is correct.

test/safestack/CMakeLists.txt
10

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.

15

Why does Apple require LTO for testing?

test/safestack/check-buffer-copy.c
10

Could you do the same test with a VLA?

15

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, ...).

test/safestack/check-overflow.c
11

noinline would be appropriate, though IPO could still kick in. Maybe slap a volatile on there too?

14

I'd rather memset a bigger range which includes the buffer itself than just changing two values.

23

The optimizer knows that value1 and value2 don't escape main. This check is trivially true.

test/safestack/check-pthread.c
23

The buffer allocation and memset are dead code and can be eliminated.

ksvladimir added inline comments.May 23 2015, 5:15 AM
cmake/config-ix.cmake
246

Good point, in principle the code is fairly cross platform (although we only tested it on x86).

lib/safestack/safestack.cc
175

The primary reason for this check is to handle cases when libsafestack is linked into multiple DSOs and __safestack_init ends up being on multiple constructor lists. It can only be called concurrently if multiple DSOs are being dlopen'ed in parallel. I think that dlopen itself isn't thread-safe and shouldn't be used that way but, perhaps, for extra safety it's useful to make this code non-racy using atomics.

226

In fact, it might depend on the architecture and even on the calling convention. Perhaps we should just remove this function altogether, as it is not really related to SafeStack, and we ended up not using it anyway neither in Chrome nor FreeBSD.

pcc updated this revision to Diff 26537.May 26 2015, 2:34 PM
  • s/0/nullptr/ where appropriate
  • Address remaining review comments
cmake/config-ix.cmake
246

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.

327

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.

lib/safestack/safestack.cc
45

Done

55

Done

72

Done

107

Done

110

Removed

175

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.)

226

Removed

test/safestack/CMakeLists.txt
10

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.

15

See above.

test/safestack/check-buffer-copy.c
10

Done

15

Done

test/safestack/check-overflow.c
11

Done

14

Done

23

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.

test/safestack/check-pthread.c
23

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.

pcc updated this revision to Diff 26546.May 26 2015, 4:17 PM
  • clang-format
jfb added a subscriber: rnk.May 28 2015, 1:49 PM

lgtm after the two comments are addressed.

cmake/config-ix.cmake
327

Tagging @rnk FYI.

lib/safestack/safestack.cc
62

Overflow check here too, and with guard.

175

Gotcha. Would it be useful to have nothing here for now (@pcc just deleted the code), but add a comment explaining @ksvladimir's point?

pcc updated this revision to Diff 26839.May 29 2015, 5:25 PM
  • Add more overflow checks; add detailed documentation of limitations (from Laszlo)
pcc added a comment.May 29 2015, 5:25 PM

I've added more detailed documentation on solutions to the limitations as a TODO in safestack.cc rather than the documentation where it isn't really actionable to a typical reader of the doc who probably just wants to use safe stack.

lib/safestack/safestack.cc
175

I don't think we need to add an explanation here. It would only make sense to do so if we supported multiple copies of the library in multiple DSOs, which isn't necessarily something we'll even want to support.

This revision was automatically updated to reflect the committed changes.
jfb added a reviewer: jfb.Sep 13 2015, 6:21 PM