Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
10 ↗(On Diff #26088)

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
10 ↗(On Diff #26088)

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
259

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?

340

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
9 ↗(On Diff #26109)

Could you do the same test with a VLA?

14 ↗(On Diff #26109)

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
10 ↗(On Diff #26109)

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

13 ↗(On Diff #26109)

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

22 ↗(On Diff #26109)

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

test/safestack/check-pthread.c
22 ↗(On Diff #26109)

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
259

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
259

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.

340

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
9 ↗(On Diff #26109)

Done

14 ↗(On Diff #26109)

Done

test/safestack/check-overflow.c
10 ↗(On Diff #26109)

Done

13 ↗(On Diff #26109)

Done

22 ↗(On Diff #26109)

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
22 ↗(On Diff #26109)

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
340

Tagging @rnk FYI.

lib/safestack/safestack.cc
61

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