[PATCH] D69045: libhwasan interceptor ABI intercept longjmp/setjmp

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_interceptors.cpp:224
+
+#if HWASAN_WITH_INTERCEPTORS && defined(__aarch64__)
+/*
----------------
mmalcomson wrote:
> mmalcomson wrote:
> > eugenis wrote:
> > > 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
> > > 
> > 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.
> 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?
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. 



================
Comment at: compiler-rt/lib/hwasan/hwasan_interceptors.cpp:271
+  register void *env_address asm ("x0") = &env[0];
+  asm volatile (
+	"ldp	x19, x20, [%0, #0<<3];"
----------------
mmalcomson wrote:
> eugenis wrote:
> > Why can't this asm be replaced with REAL(longjmp)(env, retval) ?
> 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).
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.


================
Comment at: compiler-rt/lib/hwasan/hwasan_interceptors.cpp:351
+  INTERCEPT_FUNCTION(__libc_longjmp);
+  INTERCEPT_FUNCTION(siglongjmp);
 #endif  // __aarch64__
----------------
These are not needed if you are not calling through to REAL(func).


================
Comment at: compiler-rt/lib/hwasan/hwasan_setjmp.S:1
+// We want to save the context of the calling function.
+// That requires
----------------
Please add a license header.


================
Comment at: compiler-rt/test/hwasan/TestCases/longjmp-setjmp-interception.c:5
+#include <stdio.h>
+
+/* Testing longjmp/setjmp should test that accesses to scopes jmp'd over are
----------------
This should say something like

REQUIRES: aarch64-target-arch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69045/new/

https://reviews.llvm.org/D69045