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

[pcc]

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.

[kcc]

CMakeLists.txt

210 ↗

(On Diff #25221)

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

lib/safestack/safestack.cc

38 ↗

(On Diff #25221)

please no macros where constants work

45 ↗

(On Diff #25221)

do you really need this?

57 ↗

(On Diff #25221)

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

80 ↗

(On Diff #25221)

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

101 ↗

(On Diff #25221)

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

114 ↗

(On Diff #25221)

use CHECK_EQ

126 ↗

(On Diff #25221)

UnmapOrDie

204 ↗

(On Diff #25221)

use CHECK

276 ↗

(On Diff #25221)

please make this similar to other sanitizers (SANITIZER_CAN_USE_PREINIT_ARRAY).

[pcc]

• Use constants, simplfy pthread stuff
• Remove more macros
• Use MmapOrDie etc.
• Use CHECK_* instead of assert

CMakeLists.txt

210 ↗

(On Diff #25221)

Updated.

lib/safestack/safestack.cc

38 ↗

(On Diff #25221)

Done

45 ↗

(On Diff #25221)

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

57 ↗

(On Diff #25221)

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.

80 ↗

(On Diff #25221)

Removed

101 ↗

(On Diff #25221)

Done

114 ↗

(On Diff #25221)

Done

126 ↗

(On Diff #25221)

Done

204 ↗

(On Diff #25221)

Done

276 ↗

(On Diff #25221)

Done

test/safestack/CMakeLists.txt

6 ↗

(On Diff #25221)

Done

[kcc]

lib/safestack/safestack.cc

20 ↗

(On Diff #26088)

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

64 ↗

(On Diff #26088)

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

83 ↗

(On Diff #26088)

do you still need this comment?

test/safestack/check-pthread-cleanup.c

10 ↗

(On Diff #26088)

kBufferSize

[pcc]

• remove comment
• Remove MAP_FAILED check, remove unused #includes
• Use enum constant

lib/safestack/safestack.cc

20 ↗

(On Diff #26088)

Done

64 ↗

(On Diff #26088)

Removed

83 ↗

(On Diff #26088)

Removed

test/safestack/check-pthread-cleanup.c

10 ↗

(On Diff #26088)

Done

[pcc]

• Remove C++ marker

[kcc]

LGTM

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

[ksvladimir]

lib/safestack/safestack.cc

166 ↗

(On Diff #26095)

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

[pcc]

• Use SANITIZER_CAN_USE_PREINIT_ARRAY in one more place

lib/safestack/safestack.cc

166 ↗

(On Diff #26095)

Done

[pcc]

• Add LTO test

[pcc]

• Fix copy-pasteo

[jfb]

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 ↗

(On Diff #26109)

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 ↗

(On Diff #26109)

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

44 ↗

(On Diff #26109)

nullptr here and other pointers below.

54 ↗

(On Diff #26109)

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.

71 ↗

(On Diff #26109)

Drop the (void) case.

106 ↗

(On Diff #26109)

s/intersepting/intercepting/

109 ↗

(On Diff #26109)

s/rutine/routine/

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

174 ↗

(On Diff #26109)

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.

225 ↗

(On Diff #26109)

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

test/safestack/CMakeLists.txt

9 ↗

(On Diff #26109)

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.

14 ↗

(On Diff #26109)

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]

cmake/config-ix.cmake

259 ↗

(On Diff #26109)

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

lib/safestack/safestack.cc

174 ↗

(On Diff #26109)

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.

225 ↗

(On Diff #26109)

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]

• s/0/nullptr/ where appropriate
• Address remaining review comments

cmake/config-ix.cmake

259 ↗

(On Diff #26109)

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 ↗

(On Diff #26109)

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

44 ↗

(On Diff #26109)

Done

54 ↗

(On Diff #26109)

Done

71 ↗

(On Diff #26109)

Done

106 ↗

(On Diff #26109)

Done

109 ↗

(On Diff #26109)

Removed

174 ↗

(On Diff #26109)

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

225 ↗

(On Diff #26109)

Removed

test/safestack/CMakeLists.txt

9 ↗

(On Diff #26109)

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.

14 ↗

(On Diff #26109)

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]

• clang-format

[jfb]

lgtm after the two comments are addressed.

cmake/config-ix.cmake

340 ↗

(On Diff #26109)

Tagging @rnk FYI.

lib/safestack/safestack.cc

61 ↗

(On Diff #26537)

Overflow check here too, and with guard.

174 ↗

(On Diff #26109)

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]

• Add more overflow checks; add detailed documentation of limitations (from Laszlo)

[pcc]

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

174 ↗

(On Diff #26109)

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.