[PATCH] tsan: Remove __pointer_chk_guard@GLIBC_PRIVATE requirement for AArch64

[zatrazz]

Summary

Current AArch64 {sig}{set,long}jmp interposing requires accessing glibc
private __pointer_chk_guard to get process xor mask to demangled the
internal {sig}jmp_buf function pointers.

It causes some packing issues, as described in gcc PR#71042 [1], and is
is not a godd practice to rely on a private glibc namespace (since ABI is
not meant to be stable).

This patch fixes it by changing how libtsan obtains the guarded pointer
value: at initialization a specific routine issues a setjmp call and
using the mangled function pointer and the original value derive the
random guarded pointer.

Checked on aarch64 39-bit VMA.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71042

[dvyukov]

Looks good to me. But I did not read the assembly. Please wait few days for Renato.

lib/tsan/rtl/tsan_platform_linux.cc

273

I think this comment belongs to tsan_rtl_aarch64.S.
This file is platform independent, and the things this comment talks about are all in tsan_rtl_aarch64.S.

[rengolin]

Adding a few people, as I'm on holiday.

I worry that we'll end up with our own private implementation, and create problems for TSAN in the future. Or maybe I didn't understand the problem... :-)

[zatrazz]

In D23318#510050, @rengolin wrote:

Adding a few people, as I'm on holiday.

I worry that we'll end up with our own private implementation, and create problems for TSAN in the future. Or maybe I didn't understand the problem... :-)

TSAN on Linux already requires to handle at lot of glibc internal implementation to work corretly and in this case its is to avoid use a internal linker symbol. What kind of issues are you worried about?

[zatrazz]

lib/tsan/rtl/tsan_platform_linux.cc

273

Alright, I think I can just add a comment explaining this calls a arch-specific code to init the glibc guard pointer.

[rengolin]

Reading again, it seems you're just reimplementing a similar logic, with a known global, which is safer than relying on glibc's own copy. So, even for gcc using TSAN, there will be no deviation.

You're still relying on _ZN14interception12realsetjmpE, but that's a public interface so it should be OK, right?

Apart from the name nit, I have no real concern.

Cheers,
Renato

lib/tsan/rtl/tsan_rtl_aarch64.S

76

Maybe call it __tsan_pointer_chk_guard?

[zatrazz]

In D23318#510362, @rengolin wrote:

Reading again, it seems you're just reimplementing a similar logic, with a known global, which is safer than relying on glibc's own copy. So, even for gcc using TSAN, there will be no deviation.

Yes, in fact the requirement of this patch canme from GCC PR#71042.

You're still relying on _ZN14interception12realsetjmpE, but that's a public interface so it should be OK, right?

It is in fact 'interception::realsetjmp' which is defined by interceptors macros and initialized at 'InitializeInterceptors'. It requires to use this indirection because 'setjmp/_setjmp' is overriden in this same assembly, so linker won't call GLIBC one.

Apart from the name nit, I have no real concern.

Cheers,
Renato

lib/tsan/rtl/tsan_rtl_aarch64.S

76

Since it is a local symbol I didn't bother with the name, but I think your suggestion seems better. I will change it.

lib/tsan/rtl/tsan_platform_linux.cc

 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <stdarg.h>
 #include <sys/mman.h>
 #if SANITIZER_LINUX
 #include <sys/personality.h>
+#include <setjmp.h>
 #endif
 #include <sys/syscall.h>
 #include <sys/socket.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <unistd.h>

 # undef sa_sigaction
 #endif
 
 #if SANITIZER_FREEBSD
 extern "C" void *__libc_stack_end;
 void *__libc_stack_end = 0;
 #endif
 
+#if SANITIZER_LINUX && defined(__aarch64__)
+void InitializeGuardPtr() __attribute__((visibility("hidden")));
+#endif
+
 namespace __tsan {
 
 #ifdef TSAN_RUNTIME_VMA
 // Runtime detected VMA size.
 uptr vmaSize;
 #endif
 
 enum {
#if SANITIZER_LINUX && defined(__aarch64__)
     int old_personality = personality(0xffffffff);
     if (old_personality != -1 && (old_personality & ADDR_NO_RANDOMIZE) == 0) {
       VReport(1, "WARNING: Program is run with randomized virtual address "
               "space, which wouldn't work with ThreadSanitizer.\n"
               "Re-execing with fixed virtual address space.\n");
       CHECK_NE(personality(old_personality | ADDR_NO_RANDOMIZE), -1);
       reexec = true;
     }
+
+    // GLIBC mangles the function pointers in jmp_buf (used in {set,long}*jmp
+    // functions) by XORing them with a random guard pointer.  For AArch64 it
+    // is a global variable rather than a TCB one (as for x86_64/powerpc) and
+    // althought its value is exported by the loader, it lies within a private
+    // GLIBC namespace (meaning it should be only used by GLIBC itself and
+    // the ABI is not stable).
+    // So InitializeGuardPtr obtains the pointer guard value by issuing a
+    // setjmp and checking the resulting pointers values against the original
+    // ones.
+    InitializeGuardPtr();
 #endif
     if (reexec)
       ReExec();
   }
 
 #ifndef SANITIZER_GO
   CheckAndProtect();
   InitTlsSize();

lib/tsan/rtl/tsan_rtl_aarch64.S

 #include "sanitizer_common/sanitizer_asm.h"
+
+.section .bss
+.type pointer_chk_guard, %object
+.size pointer_chk_guard, 8
+pointer_chk_guard:
+.zero 8
+
 .section .text
 
+// Initialize the 'pointer_chk_guard' value by calling glibc setjmp.  It
+// should be called after interceptors have been initialized.
+.hidden _Z18InitializeGuardPtrv
+.global _Z18InitializeGuardPtrv
+.type InitializeGuardPtr, @function
+_Z18InitializeGuardPtrv:
+  CFI_STARTPROC
+  // Allocates a jmp_buf for the setjmp call.
+  stp  x29, x30, [sp, -336]!
+  CFI_DEF_CFA_OFFSET (336)
+  CFI_OFFSET (29, -336)
+  CFI_OFFSET (30, -328)
+  add x29, sp, 0
+  CFI_DEF_CFA_REGISTER (29)
+  add x0, x29, 24
+
+  // Call libc setjmp that mangle the stack pointer value
+  adrp  x1, :got:_ZN14__interception12real__setjmpE
+  ldr   x1, [x1, #:got_lo12:_ZN14__interception12real__setjmpE]
+  ldr   x1, [x1]
+  blr   x1
+
+  // glibc setjmp mangles both the frame pointer (FP, pc+4 on blr) and the
+  // stack pointer (SP). FP will be placed on ((uintptr*)jmp_buf)[11] and
+  // SP at ((uintptr*)jmp_buf)[13].
+  // The mangle operation is just 'value' xor 'pointer guard value' and
+  // if we know the original value (SP) and the expected one, we can derive
+  // the guard pointer value.
+  mov x0, sp
+
+  // Loads the mangled SP pointer.
+  ldr x1, [x29, 128]
+  eor x0, x0, x1
+  adrp  x2, pointer_chk_guard
+  str x0, [x2, #:lo12:pointer_chk_guard]
+  ldp x29, x30, [sp], 336
+  CFI_RESTORE (30)
+  CFI_RESTORE (19)
+  CFI_DEF_CFA (31, 0)
+  ret
+  CFI_ENDPROC
+.size _Z18InitializeGuardPtrv, .-_Z18InitializeGuardPtrv
+
 .hidden __tsan_setjmp
 .comm _ZN14__interception11real_setjmpE,8,8
 .type setjmp, @function
 setjmp:
   CFI_STARTPROC
 
   // save env parameters for function call
   stp     x29, x30, [sp, -32]!
   CFI_DEF_CFA_OFFSET (32)
   CFI_OFFSET (29, -32)
   CFI_OFFSET (30, -24)
 
   // Adjust the SP for previous frame
   add     x29, sp, 0
   CFI_DEF_CFA_REGISTER (29)
 
   // Save jmp_buf
   str     x19, [sp, 16]
   CFI_OFFSET (19, -16)
   mov     x19, x0
 
   // SP pointer mangling (see glibc setjmp)
-  adrp    x2, :got:__pointer_chk_guard
-  ldr     x2, [x2, #:got_lo12:__pointer_chk_guard]
+  adrp    x2, pointer_chk_guard
+  ldr     x2, [x2, #:lo12:pointer_chk_guard]
   add     x0, x29, 32
-  ldr     x2, [x2]
   eor     x1, x2, x0
 
   // call tsan interceptor
   bl      __tsan_setjmp
 
   // restore env parameter
   mov     x0, x19
   ldr     x19, [sp, 16]
_setjmp:
   CFI_DEF_CFA_REGISTER (29)
 
   // Save jmp_buf
   str     x19, [sp, 16]
   CFI_OFFSET (19, -16)
   mov     x19, x0
 
   // SP pointer mangling (see glibc setjmp)
-  adrp    x2, :got:__pointer_chk_guard
-  ldr     x2, [x2, #:got_lo12:__pointer_chk_guard]
+  adrp    x2, pointer_chk_guard
+  ldr     x2, [x2, #:lo12:pointer_chk_guard]
   add     x0, x29, 32
-  ldr     x2, [x2]
   eor     x1, x2, x0
 
   // call tsan interceptor
   bl      __tsan_setjmp
 
   // Restore jmp_buf parameter
   mov     x0, x19
   ldr     x19, [sp, 16]
sigsetjmp:
   // Save jmp_buf and savesigs
   stp     x19, x20, [sp, 16]
   CFI_OFFSET (19, -16)
   CFI_OFFSET (20, -8)
   mov     w20, w1
   mov     x19, x0
 
   // SP pointer mangling (see glibc setjmp)
-  adrp    x2, :got:__pointer_chk_guard
-  ldr     x2, [x2, #:got_lo12:__pointer_chk_guard]
+  adrp    x2, pointer_chk_guard
+  ldr     x2, [x2, #:lo12:pointer_chk_guard]
   add     x0, x29, 32
-  ldr     x2, [x2]
   eor     x1, x2, x0
 
   // call tsan interceptor
   bl      __tsan_setjmp
 
   // restore env parameter
   mov     w1, w20
   mov     x0, x19
__sigsetjmp:
   // Save jmp_buf and savesigs
   stp     x19, x20, [sp, 16]
   CFI_OFFSET (19, -16)
   CFI_OFFSET (20, -8)
   mov     w20, w1
   mov     x19, x0
 
   // SP pointer mangling (see glibc setjmp)
-  adrp    x2, :got:__pointer_chk_guard
-  ldr     x2, [x2, #:got_lo12:__pointer_chk_guard]
+  adrp    x2, pointer_chk_guard
+  ldr     x2, [x2, #:lo12:pointer_chk_guard]
   add     x0, x29, 32
-  ldr     x2, [x2]
   eor     x1, x2, x0
 
   // call tsan interceptor
   bl      __tsan_setjmp
 
   mov     w1, w20
   mov     x0, x19
   ldp     x19, x20, [sp, 16]