[tsan] Lazily call 'personality' to minimize sandbox violations

[thurstond]

My previous patch, "Re-exec TSan with no ASLR if memory layout is incompatible on Linux (#78351)" (0784b1e) hoisted the 'personality' call, to
share the code between Android and non-Android Linux. Unfortunately, this eager call to
'personality' may trigger sandbox violations on non-Android Linux.

This patch fixes the issue by only calling 'personality' on non-Android Linux if the
memory mapping is incompatible. This may still cause a sandbox violation, but only if it
was going to abort anyway due to an incompatible memory mapping.

(The behavior on Android Linux is unchanged by this patch or the previous patch.)

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Thurston Dang (thurstond)

Changes

My previous patch, "Re-exec TSan with no ASLR if memory layout is incompatible on Linux (#78351)" (0784b1e) hoisted the 'personality' call, to
share the code between Android and non-Android Linux. Unfortunately, this eager call to
'personality' may trigger sandbox violations on non-Android Linux.

This patch fixes the issue by only calling 'personality' on non-Android Linux if the
memory mapping is incompatible. This may still cause a sandbox violation, but only if it
was going to abort anyway due to an incompatible memory mapping.

(The behavior on Android Linux is unchanged by this patch or the previous patch.)

---

Full diff: https://github.com/llvm/llvm-project/pull/79334.diff

1 Files Affected:

• (modified) compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp (+9-1)

diff --git a/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp b/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp
index 0d0b1aba1f852a5..c723dba556ed2f7 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp
@@ -244,12 +244,12 @@ static void ReExecIfNeeded() {
   }
 
 #    if SANITIZER_LINUX
+#      if SANITIZER_ANDROID && (defined(__aarch64__) || defined(__x86_64__))
   // ASLR personality check.
   int old_personality = personality(0xffffffff);
   bool aslr_on =
       (old_personality != -1) && ((old_personality & ADDR_NO_RANDOMIZE) == 0);
 
-#      if SANITIZER_ANDROID && (defined(__aarch64__) || defined(__x86_64__))
   // After patch "arm64: mm: support ARCH_MMAP_RND_BITS." is introduced in
   // linux kernel, the random gap between stack and mapped area is increased
   // from 128M to 36G on 39-bit aarch64. As it is almost impossible to cover
@@ -267,6 +267,14 @@ static void ReExecIfNeeded() {
   if (reexec) {
     // Don't check the address space since we're going to re-exec anyway.
   } else if (!CheckAndProtect(false, false, false)) {
+    // ASLR personality check.
+    // N.B. 'personality' is sometimes forbidden by sandboxes, so we only call
+    // this as a last resort (when the memory mapping is incompatible and TSan
+    // would fail anyway).
+    int old_personality = personality(0xffffffff);
+    bool aslr_on =
+        (old_personality != -1) && ((old_personality & ADDR_NO_RANDOMIZE) == 0);
+
     if (aslr_on) {
       // Disable ASLR if the memory layout was incompatible.
       // Alternatively, we could just keep re-execing until we get lucky

[dvyukov]

We probably could test this by defining own personality function in a test that will simply abort. This way we can have a list of functions that must not be called by the runtime (prohibited by sandboxes).
Though my bet is that it will fail on some platforms for some reasons. So probably the test should be limited to Linux/x86 only.

[thurstond]

I'm not quite sure how I could add this to the TSan tests, since the sandbox behavior (e.g., allowing/disallowing 'personality') is dependent on the vendor's configuration.

Taken to an extreme, there's probably some sandbox out there that disallows 'printf', but it would be unreasonable to avoid printf entirely in TSan.

[dvyukov]

I meant adding this to a test:

extern "C" int personality(unsigned long) { abort(); }

Such test should abort w/o this fix.
Probably better to add to sanitizer_common tests so that all sanitizers are tested.

Of course, there can be sandboxes that prohibit too much and sanitizers won't work under these sandboxes. We can't fix that. But we do want to avoid personality and we want it to not regress tomorrow.

[thurstond]

Ah, I see! I've added compiler-rt/test/sanitizer_common/TestCases/Linux/sandbox_forbidden_functions.cpp. I tested that the test failed with TSan in the absence of this fix, and passes with this fix.

Note that it does have a false positive when TSan is run with high-entropy ASLR: in that environment, calling 'personality' (to check/disable ASLR) is the intended behavior.

[dvyukov]

Note that it does have a false positive when TSan is run with high-entropy ASLR

Will it be flaky on bots then? That's not good.

[thurstond]

It will be flaky on bots that have very high-entropy ASLR. AFAIK there are currently no such bots (because without the re-exec patch, all TSan tests would be failing consistently on those bots).

With that caveat in mind, please let me know if you would prefer to not test 'personality'.

[dvyukov]

If it would be only about bots, then I would say let's try to add it (perhaps with an additional comment).
But it will also fail on local developer ninja check-sanitizer runs, right?

[thurstond]

It will fail on local developer ninja check-sanitizer if they are running with very high entropy ASLR.

[dvyukov]

Then I afraid it's probably better to disable the test.
But also work under that sandbox will probably break soon.

[thurstond]

I've removed the test in the latest commit (0b4c846).

With the re-exec patch + this patch:

• low entropy ASLR, no sandbox: TSan works
• low entropy ASLR, with sandbox: TSan works
• high entropy ASLR, no sandbox: TSan works
• high entropy ASLR, with sandbox: TSan will trigger a sandbox violation, but TSan didn't work anyway with high entropy ASLR. Moreover, this is currently only a hypothetical case.