libhwasan interceptor ABI intercept longjmp/setjmp

[mmalcomson]

Summary

The hwasan interceptor ABI doesn't have interceptors for longjmp and setjmp.
This patch introduces them.

We require the size of the jmp_buf on the platform to be at least as large as the jmp_buf in our implementation.
To enforce this we compile hwasan_type_test.cpp that ensures a compile time failure if this is not true.

Tested on both GCC and clang using an AArch64 virtual machine.

[mmalcomson]

Added a test in the hwasan testsuite.

[eugenis]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

224

This won't work on Android, please add && !SANITIZER_ANDROID.

Just FYI, this is what we do there:
https://android.googlesource.com/platform/bionic/+/919dc05d66b129ad6f34fad95322efb6de245754/libc/arch-arm64/bionic/setjmp.S#214

271

Why can't this asm be replaced with REAL(longjmp)(env, retval) ?

[mmalcomson]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

224

Can do, but I don't fully understand -- could you explain why a bit more?

I was thinking that the Android "platform ABI" would handle longjmp, but the Android "interceptor ABI" would have the same missing feature as any other interceptor ABI.

I would have guessed that the feature check in the bionic sources would be essentially checking for the platform ABI -- since if using the interceptor ABI bionic wouldn't know that hwasan is being used.

271

As I understand it, the order that registers are stored in the jmp_buf structure is undefined by any architecture ABI.
Hence different longjmp/setjmp implementations are likely to have a different order of registers (and a quick comparison of the bionic and glibc setjmps seem to confirm this).

Because of this, if using REAL(longjmp), we'd need to also use REAL(setjmp), and when using REAL(setjmp) we wouldn't know where the stack pointer is in the jmp_buf.

Also, some libc's (like glibc) mangle their stack pointer to improve security, and we wouldn't be able to read it even if we did know where it was (e.g. if hard-coded offset different for each platform we compile for or only targeting one libc).

[mmalcomson]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

224

Ah ... I figure the reason it won't work for Android is to do with the dynamic loader feature mentioned in https://reviews.llvm.org/D55986 .

Hence a setjmp could be from the libc while the longjmp is from the interceptor ... correct?

[eugenis]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

224

Yeah, something like that is possible, but only when hwasan is loaded late (ex. through dlopen). I don't think we want to support that use case anymore.

One reason this would not work on Android is that the symbol for sigsetjmp in bionic is actually called sigsetjmp, and not __sigsetjmp. Also, this code assumes that jmp_buf in the target libc is at least as large as hwasan definition. It is likely to be case, but not guaranteed.

Anyway, I agree that this approach is better than the alternative. Could you fix it to work on Android, too? This is the only AArch64 testing environment we have on the buildbot. I see a few other problems when trying to build, like missing definition for __sigset_t.

271

OK, this is a good argument. We would not want to detect libc version / change this code every time they invent a new jmp_buf mangling scheme.

357

These are not needed if you are not calling through to REAL(func).

compiler-rt/lib/hwasan/hwasan_setjmp.S

2

Please add a license header.

compiler-rt/test/hwasan/TestCases/longjmp-setjmp-interception.c

6

This should say something like

REQUIRES: aarch64-target-arch

[mmalcomson]

I've tried to get this working for Android.

I've built for android (and fixed the build warnings), but have not yet managed to test at runtime on Android (latest hurdle was running a too-old version of Android for the TLS slots to match).

I'm putting what I have up now on the off-chance someone is willing to run the test on their development environment.

If not I'll continue to work on getting an Android environment working with hwasan -- I'm sure it will just take time -- the only difficulty is inexperience with it.

Cheers ;-)

[eugenis]

I've checked this on Android, the test passes.
Indeed, you'd need Android 10, or an AOSP master branch build from 2019 to run this.

Compiler-rt tests can be difficult to setup on Android. LLVM has an unofficial GN build system where this works out of the box, check llvm/utils/gn/README.rst if you feel like it; setting android_ndk_path and android_serial_for_testing should be enough to get the tests running, but you would also need to update the build file under llvm/utils/gn/secondary to add the new source file to a list.

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

243

To make sure that we do not overflow the platform's jmp_buf, could you move this definition to ex. hwasan.h, then add a compile-time check that its sizeof is <= than the system's? See CHECK_TYPE_SIZE macro in sanitizer_platform_limits_posix.h.

We don't allow system includes in this file because then the signatures of the interceptor wrappers would need to match system headers exactly, and that's not always possible across multiple libc versions.

258

We use CamelCase w/o underscores for internal functions.

compiler-rt/lib/hwasan/hwasan_setjmp.S

99

NO_EXEC_STACK_DIRECTIVE should go below #ifdef - otherwise you might get an empty source file that suppressed noexecstack.

Clang assembler defaults to non-executable stack when targeting Android, but other platforms may not be so lucky.

[mmalcomson]

Have addressed raised issues.

Still not managed to test on Android myself (but getting closer).

[eugenis]

LGTM

[thakis]

compiler-rt/lib/hwasan/CMakeLists.txt

19

Looks like this list was sorted alphabetically so far. Want to maintain that?

[mmalcomson]

compiler-rt/lib/hwasan/CMakeLists.txt

19

Ah! Yes, I would like to maintain that.
I'm not familiar with LLVM contribution guidelines, could someone do this without needing to create a new review?
(for reference, I don't have commit rights)

[eugenis]

compiler-rt/lib/hwasan/CMakeLists.txt

19

No problem, I've pushed a fix.

[eugenis]

According to https://github.com/google/sanitizers/issues/1244, there is a non-interceptable _setjmp in __libc_start_main that is later jumped to in pthread_exit.
It seems to break this approach.
Any idea what to do? Detect that jmpbuf is not a hwasan jmpbuf and bail out? This is happening at the very end of a thread's life, so hopefully it should not matter that the stack in not untagged.

[mmalcomson]

In D69045#2030063, @eugenis wrote:

According to https://github.com/google/sanitizers/issues/1244, there is a non-interceptable _setjmp in __libc_start_main that is later jumped to in pthread_exit.
It seems to break this approach.
Any idea what to do? Detect that jmpbuf is not a hwasan jmpbuf and bail out? This is happening at the very end of a thread's life, so hopefully it should not matter that the stack in not untagged.

Well that's unfortunate ...
I'd thought I'd checked that any non-interceptable setjmps or longjmps would come in pairs so that a buffer set by glibc would be read by glibc.

(FWIW this particular setjmp can be intercepted in glibc 2.31 -- so hopefully as time goes on this will be less of a problem).

I think your idea of just avoiding buffers that aren't from HWASAN makes sense -- especially since the interceptor mode is not the main focus.

Just to set expectations: I expect I'll only find time to fix this next month (pretty busy in the near future).

[jdanek]

In D69045#2031667, @mmalcomson wrote:

(FWIW this particular setjmp can be intercepted in glibc 2.31 -- so hopefully as time goes on this will be less of a problem).

I am the reporter of the linked GitHub issue. Are you saying that if I just built with glibc 2.31, without any change to clang, then I would not see this instance of the setjmp error?

I believe I am on 2.31 already:

bash
$ ldd c/examples/send-ssl 
        linux-vdso.so.1 (0x0000ffff8dd01000)
        libclang_rt.hwasan-aarch64.so => /home/ubuntu/llvm/install/lib/clang/11.0.0/lib/linux/libclang_rt.hwasan-aarch64.so (0x0000ffff8d2e0000)
        libqpid-proton-proactor.so.1 => /home/ubuntu/repos/qpid-proton-0.31.0/clang/c/libqpid-proton-proactor.so.1 (0x0000ffff8d200000)
        libqpid-proton-core.so.10 => /home/ubuntu/repos/qpid-proton-0.31.0/clang/c/libqpid-proton-core.so.10 (0x0000ffff8ce4e000)
        libpthread.so.0 => /lib/aarch64-linux-gnu/libpthread.so.0 (0x0000ffff8ce1e000)
        libssl.so.1.1 => /lib/aarch64-linux-gnu/libssl.so.1.1 (0x0000ffff8cd84000)
        libcrypto.so.1.1 => /lib/aarch64-linux-gnu/libcrypto.so.1.1 (0x0000ffff8caf7000)
        libsasl2.so.2 => /lib/aarch64-linux-gnu/libsasl2.so.2 (0x0000ffff8cacc000)
        libc.so.6 => /lib/aarch64-linux-gnu/libc.so.6 (0x0000ffff8c95b000)
        libstdc++.so.6 => /lib/aarch64-linux-gnu/libstdc++.so.6 (0x0000ffff8c779000)
        libgcc_s.so.1 => /lib/aarch64-linux-gnu/libgcc_s.so.1 (0x0000ffff8c754000)
        libdl.so.2 => /lib/aarch64-linux-gnu/libdl.so.2 (0x0000ffff8c740000)
        /lib/ld-linux-aarch64.so.1 (0x0000ffff8dcd1000)
        libm.so.6 => /lib/aarch64-linux-gnu/libm.so.6 (0x0000ffff8c695000)
ubuntu@ubuntu:~/repos/qpid-proton-0.31.0/clang$ /lib/aarch64-linux-gnu/libc.so.6 --version
GNU C Library (Ubuntu GLIBC 2.31-0ubuntu9) stable release version 2.31.
Copyright (C) 2020 Free Software Foundation, Inc.

[mmalcomson]

In D69045#2034976, @jdanek wrote:

In D69045#2031667, @mmalcomson wrote:

(FWIW this particular setjmp can be intercepted in glibc 2.31 -- so hopefully as time goes on this will be less of a problem).

I am the reporter of the linked GitHub issue. Are you saying that if I just built with glibc 2.31, without any change to clang, then I would not see this instance of the setjmp error?

I believe I am on 2.31 already:

Oh that's interesting -- thanks for pointing it out.

Could you just double check objdump your libc -- if it still has a non-ptl symbol then it might be that there was something strange in my testing environment, and that in the wild this is usually non-interceptable.

Interestingly, the reason I added the __libc_longjmp interceptor (lines 282-287 in hwasan_interceptors.cpp in the patch above) was because I was seeing a jump buffer mismatch at pthread_exit in the exact opposite way of what you're seeing now (setjmp in libc_start_main was intercepted and longjmp in unwind_stop was not).
If you have the time, could you remove that interceptor, rebuild, and re-test in your environment? The output from that should give some useful information.

When I look into this I'll have to understand when glibc makes that symbol internal, and why my testing environment ended up with a dynamic one.

I still expect the best option would be to avoid non-hwasan jump buffers, as eugenis suggested, but it might be better to have avoid intercepting that symbol if that's what works without a fallback.

[jdanek]

In D69045#2036038, @mmalcomson wrote:

In D69045#2034976, @jdanek wrote:

I believe I am on 2.31 already:

Could you just double check objdump your libc -- if it still has a non-ptl symbol then it might be that there was something strange in my testing environment, and that in the wild this is usually non-interceptable.

I don't actually know how to check for plt symbols. Let me just try few things, maybe I got lucky

$ readelf -Wa /lib/aarch64-linux-gnu/libc-2.31.so | grep setjmp
   653: 0000000000036ce0     8 FUNC    GLOBAL DEFAULT   13 setjmp@@GLIBC_2.17
  1031: 0000000000036cf0     8 FUNC    GLOBAL DEFAULT   13 _setjmp@@GLIBC_2.17
  1083: 0000000000036d00    88 FUNC    GLOBAL DEFAULT   13 __sigsetjmp@@GLIBC_2.17
    Name: setjmp
$ readelf -Wa /lib/aarch64-linux-gnu/libc-2.31.so | grep longjmp
   245: 00000000000e2c28    68 FUNC    GLOBAL DEFAULT   13 __longjmp_chk@@GLIBC_2.17
   314: 0000000000036db0    68 FUNC    WEAK   DEFAULT   13 _longjmp@@GLIBC_2.17
  1020: 0000000000036db0    68 FUNC    WEAK   DEFAULT   13 longjmp@@GLIBC_2.17
  1220: 0000000000036db0    68 FUNC    WEAK   DEFAULT   13 siglongjmp@@GLIBC_2.17
  1670: 0000000000036db0    68 FUNC    GLOBAL DEFAULT   13 __libc_siglongjmp@@GLIBC_PRIVATE
  2155: 0000000000036db0    68 FUNC    GLOBAL DEFAULT   13 __libc_longjmp@@GLIBC_PRIVATE
    Name: longjmp
    Name: longjmp_target

and, with more context before 24050 at https://gist.github.com/jiridanek/c1171bdcd59e6ce499255176e8211e8c#file-objdump-d-grep

$ objdump -d /lib/aarch64-linux-gnu/libc-2.31.so | grep __libc_start_main -A50 | grep setjmp
   24050:       94004b28        bl      36cf0 <_setjmp@@GLIBC_2.17>

and objdump -d -s -j .plt -j .got.plt /lib/aarch64-linux-gnu/libc-2.31.so at https://gist.github.com/jiridanek/c1171bdcd59e6ce499255176e8211e8c#file-objdump-d-s-j-plt-j-got-plt

[jdanek]

In D69045#2036038, @mmalcomson wrote:

If you have the time, could you remove that interceptor, rebuild, and re-test in your environment? The output from that should give some useful information.

I got essentially the same results as I quoted in https://github.com/google/sanitizers/issues/1245. Some tag-mismatches on mostly global variables (which I believe to be false positive reports) and the sigsegv. I did not get the assertion error for GetTagFromPointer(dst) == 0 any more.

Let me know when you get the time to look into this. I'll find myself better hardware to build clang more quickly, and would gladly help with testing. We could possibly pick some test project, a Python and Ruby module written in C, which does not have other compile dependencies, and is simple to compile.

compiler-rt/lib/hwasan/CMakeLists.txt

set(HWASAN_RTL_SOURCES
   hwasan_interceptors_vfork.S
   hwasan_linux.cpp
   hwasan_memintrinsics.cpp
   hwasan_poisoning.cpp
   hwasan_report.cpp
   hwasan_tag_mismatch_aarch64.S
   hwasan_thread.cpp
   hwasan_thread_list.cpp
+  hwasan_setjmp.S
   )
 
 set(HWASAN_RTL_CXX_SOURCES
   hwasan_new_delete.cpp
   )
 
 set(HWASAN_RTL_HEADERS
   hwasan.h
   hwasan_allocator.h

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

 }
 #endif
 
 #if HWASAN_WITH_INTERCEPTORS
 DEFINE_REAL(int, vfork)
 DECLARE_EXTERN_INTERCEPTOR_AND_WRAPPER(int, vfork)
 #endif
 
+#if HWASAN_WITH_INTERCEPTORS && defined(__aarch64__)
+#if SANITIZER_ANDROID
+typedef unsigned long sigset_t;
+#endif
+// For both bionic and glibc __sigset_t is an unsigned long.
+typedef unsigned long __hw_sigset_t;
+// Setjmp and longjmp implementations are platform specific, and hence the
+// interception code is platform specific too.  As yet we've only implemented
+// the interception for AArch64.
+typedef unsigned long long __jmp_buf[22];
+struct __jmp_buf_tag {
+  // NOTE: The machine-dependent definitions of `__sigsetjmp'
+  // assume that a `jmp_buf' begins with a `__jmp_buf' and that
+  // `__mask_was_saved' follows it.  Do not move these members
+  // or add others before it.
+  __jmp_buf __jmpbuf;         // Calling environment.
+  int __mask_was_saved;       // Saved the signal mask?
+  __hw_sigset_t __saved_mask; // Saved signal mask.
+};
+typedef struct __jmp_buf_tag jmp_buf[1];
+typedef struct __jmp_buf_tag sigjmp_buf[1];
+
+// Get and/or change the set of blocked signals.
+extern "C" int sigprocmask(int __how, const sigset_t *__restrict __set,
+                           sigset_t *__restrict __oset);
+#define SIG_BLOCK 0
+#define SIG_SETMASK 2
+extern "C" int __sigjmp_save(sigjmp_buf env, int savemask) {
+  env[0].__mask_was_saved =
+      (savemask && sigprocmask(SIG_BLOCK, (sigset_t *)0,
+                               (sigset_t *)&env[0].__saved_mask) == 0);
+  return 0;
+}
+
+static void __attribute__((always_inline))
+__hwasan_internal_longjmp(__jmp_buf env, int retval) {
+  // Clear all memory tags on the stack between here and where we're going.
+  unsigned long long stack_pointer = env[13];
+  // The stack pointer should never be tagged, so we don't need to clear the
+  // tag for this function call.
+  __hwasan_handle_longjmp((void *)stack_pointer);
+
+  // Run code for handling a longjmp.
+  // Need to use a register that isn't going to be loaded from the environment
+  // buffer -- hence why we need to specify the register to use.
+  // Must implement this ourselves, since we don't know the order of registers
+  // in different libc implementations and many implementations mangle the
+  // stack pointer so we can't use it without knowing the demangling scheme.
+  register long int retval_tmp asm("x1") = retval;
+  register void *env_address asm("x0") = &env[0];
+  asm volatile("ldp  x19, x20, [%0, #0<<3];"
+               "ldp  x21, x22, [%0, #2<<3];"
+               "ldp  x23, x24, [%0, #4<<3];"
+               "ldp  x25, x26, [%0, #6<<3];"
+               "ldp  x27, x28, [%0, #8<<3];"
+               "ldp  x29, x30, [%0, #10<<3];"
+               "ldp   d8,  d9, [%0, #14<<3];"
+               "ldp  d10, d11, [%0, #16<<3];"
+               "ldp  d12, d13, [%0, #18<<3];"
+               "ldp  d14, d15, [%0, #20<<3];"
+               "ldr  x5, [%0, #13<<3];"
+               "mov  sp, x5;"
+               // Return the value requested to return through arguments.
+               // This should be in x1 given what we requested above.
+               "cmp  %1, #0;"
+               "mov  x0, #1;"
+               "csel x0, %1, x0, ne;"
+               "br x30;"
+               : "+r"(env_address)
+               : "r"(retval_tmp));
+}
+
+INTERCEPTOR(void, siglongjmp, sigjmp_buf env, int val) {
+  if (env[0].__mask_was_saved)
+    // Restore the saved signal mask.
+    (void)sigprocmask(SIG_SETMASK, (sigset_t *)&env[0].__saved_mask,
+                      (sigset_t *)0);
+  __hwasan_internal_longjmp(env[0].__jmpbuf, val);
+}
+
+INTERCEPTOR(void, __libc_longjmp, jmp_buf env, int val) {
+  __hwasan_internal_longjmp(env[0].__jmpbuf, val);
+}
+
+INTERCEPTOR(void, longjmp, jmp_buf env, int val) {
+  __hwasan_internal_longjmp(env[0].__jmpbuf, val);
+}
+#undef SIG_BLOCK
+#undef SIG_SETMASK
+
+#endif // HWASAN_WITH_INTERCEPTORS && __aarch64__
+
 static void BeforeFork() {
   StackDepotLockAll();
 }
 
 static void AfterFork() {
   StackDepotUnlockAll();
 }
 

 #if defined(__linux__)
   INTERCEPT_FUNCTION(vfork);
 #endif  // __linux__
 #if !defined(__aarch64__)
   INTERCEPT_FUNCTION(pthread_create);
 #endif  // __aarch64__
 #endif
 
   inited = 1;
 }
 } // namespace __hwasan

compiler-rt/lib/hwasan/hwasan_setjmp.S

+//===-- hwasan_setjmp.S --------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of HWAddressSanitizer.
+//
+// HWAddressSanitizer runtime.
+//===----------------------------------------------------------------------===//
+
+
+#if HWASAN_WITH_INTERCEPTORS && defined(__aarch64__)
+#include "sanitizer_common/sanitizer_asm.h"
+#include "sanitizer_common/sanitizer_platform.h"
+
+// We want to save the context of the calling function.
+// That requires
+// 1) No modification of the link register by this function.
+// 2) No modification of the stack pointer by this function.
+// 3) (no modification of any other saved register, but that's not really going
+// to occur, and hence isn't as much of a worry).
+//
+// There's essentially no way to ensure that the compiler will not modify the
+// stack pointer when compiling a C function.
+// Hence we have to write this function in assembly.
+
+.section .text
+.file "hwasan_setjmp.S"
+
+.global __interceptor_setjmp
+ASM_TYPE_FUNCTION(__interceptor_setjmp)
+__interceptor_setjmp:
+  CFI_STARTPROC
+  mov x1, #0
+  b __interceptor_sigsetjmp
+  CFI_ENDPROC
+ASM_SIZE(__interceptor_setjmp)
+
+#if SANITIZER_ANDROID
+// Bionic also defines a function `setjmp` that calls `sigsetjmp` saving the
+// current signal.
+.global __interceptor_setjmp_bionic
+ASM_TYPE_FUNCTION(__interceptor_setjmp_bionic)
+__interceptor_setjmp_bionic:
+  CFI_STARTPROC
+  mov x1, #1
+  b __interceptor_sigsetjmp
+  CFI_ENDPROC
+ASM_SIZE(__interceptor_setjmp_bionic)
+#endif
+
+.global __interceptor_sigsetjmp
+ASM_TYPE_FUNCTION(__interceptor_sigsetjmp)
+__interceptor_sigsetjmp:
+  CFI_STARTPROC
+  stp x19, x20, [x0, #0<<3]
+  stp x21, x22, [x0, #2<<3]
+  stp x23, x24, [x0, #4<<3]
+  stp x25, x26, [x0, #6<<3]
+  stp x27, x28, [x0, #8<<3]
+  stp x29, x30, [x0, #10<<3]
+  stp  d8,  d9, [x0, #14<<3]
+  stp d10, d11, [x0, #16<<3]
+  stp d12, d13, [x0, #18<<3]
+  stp d14, d15, [x0, #20<<3]
+  mov x2,  sp
+  str x2,  [x0, #13<<3]
+  // We always have the second argument to __sigjmp_save (savemask) set, since
+  // the _setjmp function above has set it for us as `false`.
+  // This function is defined in hwasan_interceptors.cc
+  b __sigjmp_save
+  CFI_ENDPROC
+ASM_SIZE(__interceptor_sigsetjmp)
+
+
+.macro ALIAS first second
+  .globl \second
+  .equ \second\(), \first
+.endm
+
+#if SANITIZER_ANDROID
+ALIAS __interceptor_sigsetjmp, sigsetjmp
+.weak sigsetjmp
+
+ALIAS __interceptor_setjmp_bionic, setjmp
+.weak setjmp
+#else
+ALIAS __interceptor_sigsetjmp, __sigsetjmp
+.weak __sigsetjmp
+#endif
+
+ALIAS __interceptor_setjmp, _setjmp
+.weak _setjmp
+
+// We do not need executable stack.
+NO_EXEC_STACK_DIRECTIVE
+#endif

compiler-rt/test/hwasan/TestCases/longjmp-setjmp-interception.c

+// RUN: %clang_hwasan -g %s -o %t && not %run %t 2>&1 | FileCheck %s
+// Only implemented for interceptor ABI on AArch64.
+// REQUIRES: aarch64-target-arch
+
+#include <setjmp.h>
+#include <stdio.h>
+
+/* Testing longjmp/setjmp should test that accesses to scopes jmp'd over are
+   caught.  */
+int __attribute__((noinline))
+uses_longjmp(int **other_array, int num, jmp_buf env) {
+  int internal_array[100] = {0};
+  *other_array = &internal_array[0];
+  if (num % 2)
+    longjmp(env, num);
+  else
+    return num % 8;
+}
+
+int __attribute__((noinline)) uses_setjmp(int num) {
+  int big_array[100];
+  int *other_array = NULL;
+  sigjmp_buf cur_env;
+  int temp = 0;
+  if ((temp = sigsetjmp(cur_env, 1)) != 0) {
+    // We're testing that our longjmp interceptor untagged the previous stack.
+    // Hence the tag in memory should be zero.
+    if (other_array != NULL)
+      return other_array[0];
+    // CHECK: READ of size 4 at{{.*}}tags: {{..}}/00
+    return 100;
+  } else
+    return uses_longjmp(&other_array, num, cur_env);
+}
+
+int __attribute__((noinline)) main() {
+  uses_setjmp(1);
+  return 0;
+}