llvm dev - Nov 2019 - HWASAN Exception handling question

Hi,

On Fri, Sep 20, 2019 at 6:48 AM Matthew Malcomson
<Matthew.Malcomson at arm.com> wrote:
>
> Hello again,
>
> I have been thinking more about the GCC implementation of hwasan and
> found a few more questions that I would really appreciate help with.
>
> ---
> I've noticed a match-all condition in the compiler inline
> instrumentation, but can't see where it's used in the libhwasan
function
> call checks.  Is that a planned behaviour or am I just missing the part
> in the code where this happens?
>
>  From reading the git history I'm guessing it's not in the library
since
> the feature was introduced for the kernel specifically and the kernel
> doesn't use the library ... or is that wild speculation on my part?
Yes, I think this is exactly right.
> ---
> Would it be OK to add `report_load{size}` functions to the library?  I
> notice that LLVM emits an inline-assembler `brk` into the IR for the
> inline tag-mismatch report.
>
> I'm a little uncomfortable putting architecture specific assembly code
> into the mid-end of GCC (even though the entire HWASAN is AArch64
> specific in GCC) and would like to put some "just report"
functions into
> libhwasan (in the same manner that libasan has
> `__asan_report_(load|store){1,2,4,8,16,_n}{,_noabort}` functions).
>
> Would that be OK?  It's a simple patch that I already have locally.  I
> guess tag-mismatch reporting would then contain an extra function in the
> stack report?
Does __hwasan_tag_mismatch / __hwasan_tag_mismatch_stub work for you?
The first one has a non-standard ABI, but it can save register state
at the point of the fault in the user code.
> ---
> As I understand it, when the mainline kernel gets patched to accept
> tagged pointers in syscalls, the relaxed ABI will be behind a `prctl`
> call rather than being generically turned on.  I guess this means that
> Android will eventually have the same requirement?
>
> If/when that happens, would the initial call to `prctl` be put in
> libhwasan, or would something like that be done in Bionic?  (n.b. this
> call needs to be done for every program since the setting doesn't
> propagate across fork).  I have a patch that adds the relevant `prctl`
> call to what `__hwasan_init` does in libhwasan.  I do this because I'm
> using a glibc unmodified for hwasan.
I agree, __hwasan_init is the right place for this.
> ---
> I've noticed code around maintaining a thread-specific ring-buffer of
> the stack frames that have been used (recording the PC and SP) -- it
> looks like this is in order to give better messages on tag-mismatch, is
> that correct?
Yes, for stack errors. We emit the tag offset of each local variable
in DWARF debug info, and the ring buffer contains PC, SP and the base
tag for recent stack frames. The code in PrintStackAllocations
combines this info to find possible source(s) of a mismatching pointer
tag.
> ---
> Would the addition of `longjmp` and `setjmp` to the "interceptor
ABI"
> build of libhwasan be OK?  I understand that LLVM pretty much only use
> that ABI for testing, but I'm working without a glibc that supports the
> "platform ABI".
Sure, patches are welcome.
> Thanks,
> Matthew

Hello,

When looking into the way that hwasan handles C++ exceptions I found
myself wondering about the need for landing pad cleanups (as documented
in the code
https://github.com/llvm-mirror/compiler-rt/blob/69445f095c22aac2388f939bedebf224a6efcdaf/lib/hwasan/hwasan_exceptions.cpp#L51
).

The comment above that code says:
// We only untag frames without a landing pad because landing pads are
// responsible for untagging the stack themselves if they resume.

I think the code as implemented untags all frames as it goes past
whether or not they have a landing pad, and am hoping people can check
or correct my understanding.

It seems that personality routine will eventually return
_URC_CONTINUE_UNWIND if this stack frame is to be unwound past (and
hence the frame will eventually get untagged).  If the frame has landing
pads then that just means the personality routine will be called
multiple times for this frame before returning that value.


I took a look by testing code generated with a clang patched to avoid
adding instrumentation to landing pads and it seems that the personality
wrapper does indeed clear stack frames with a landing pad in the basic
case.


Is there an edge case where the personality wrapper is known to not be
sufficient?  If not would removing that extra instrumentation sound
reasonable?


The test I ran was to generate code with a clang patched with the diff
below, and run it in a debugger.
I'm attaching the annotated gdb session to demonstrate my reasoning.  I
checked that the shadow memory is not untagged in a landing pad and that
after _Unwind_Resume the personality wrapper is run again, eventually
returning _URC_CONTINUE_UNWIND and untagging the shadow memory.


diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
index df7606d..ca97d72 100644
--- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -1110,8 +1110,7 @@ bool HWAddressSanitizer::sanitizeFunction(Function
&F) {
             continue;
           }

-      if (isa<ReturnInst>(Inst) || isa<ResumeInst>(Inst) ||
-          isa<CleanupReturnInst>(Inst))
+      if (isa<ReturnInst>(Inst) || isa<CleanupReturnInst>(Inst))
           RetVec.push_back(&INST);

         if (auto *DDI = dyn_cast<DbgDeclareInst>(&Inst))
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: gdb-session.vsh
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191112/a91846c2/attachment-0001.ksh>

HI Matthew,

I think you may be correct. It makes sense that an unwinder would need to
visit the frame that called _Unwind_Resume again when unwinding. And when
it does, there will be no landing pad associated with the _Unwind_Resume
call, so the unwinder will be called with _URC_CONTINUE_UNWIND and the
stack frame will be cleared. I verified that with your change to the LLVM
pass and this change to the hwasan test suite:

diff --git a/compiler-rt/test/hwasan/TestCases/try-catch.cpp
b/compiler-rt/test/hwasan/TestCases/try-catch.cpp
index 8e35a9dd2a5..4d186bf17a6 100644
--- a/compiler-rt/test/hwasan/TestCases/try-catch.cpp
+++ b/compiler-rt/test/hwasan/TestCases/try-catch.cpp
@@ -23,6 +23,11 @@ void h() {

 __attribute__((noinline))
 void g() {
+  // Make sure that we need to resume when unwinding past this function.
+  struct S {
+    ~S() { optimization_barrier((void *)this); }
+  } s;
+
   char x[1000];
   optimization_barrier(x);
   h();

the hwasan tests continue to pass on Android (with both the libgcc unwinder
and LLVM libunwind).

There are a couple of things that I can think of that could make things go
wrong here:
(1) The function's landing pad adjusts SP such that it no longer covers the
local variables before calling _Unwind_Resume. This would mean that some
local variables won't get their tags cleared.
(2) The landing pad tail calls _Unwind_Resume (which would probably require
doing (1) first). That would mean that the unwinder wouldn't see the
_Unwind_Resume caller's stack frame at all.

I'm not sure why a compiler would do (1) since it would seem to result in
larger code and more unwind info. And I've been unable to provoke the
compiler into tail calling _Unwind_Resume either. Tim, do you know of any
circumstances where the AArch64 backend would do either of these things? If
not, I think we might consider going ahead with your proposed change.

Peter

On Tue, Nov 12, 2019 at 6:06 AM Matthew Malcomson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hello,
>
> When looking into the way that hwasan handles C++ exceptions I found
> myself wondering about the need for landing pad cleanups (as documented
> in the code
>
>
https://github.com/llvm-mirror/compiler-rt/blob/69445f095c22aac2388f939bedebf224a6efcdaf/lib/hwasan/hwasan_exceptions.cpp#L51
> ).
>
> The comment above that code says:
> // We only untag frames without a landing pad because landing pads are
> // responsible for untagging the stack themselves if they resume.
>
> I think the code as implemented untags all frames as it goes past
> whether or not they have a landing pad, and am hoping people can check
> or correct my understanding.
>
> It seems that personality routine will eventually return
> _URC_CONTINUE_UNWIND if this stack frame is to be unwound past (and
> hence the frame will eventually get untagged).  If the frame has landing
> pads then that just means the personality routine will be called
> multiple times for this frame before returning that value.
>
>
> I took a look by testing code generated with a clang patched to avoid
> adding instrumentation to landing pads and it seems that the personality
> wrapper does indeed clear stack frames with a landing pad in the basic
> case.
>
>
> Is there an edge case where the personality wrapper is known to not be
> sufficient?  If not would removing that extra instrumentation sound
> reasonable?
>
>
> The test I ran was to generate code with a clang patched with the diff
> below, and run it in a debugger.
> I'm attaching the annotated gdb session to demonstrate my reasoning.  I
> checked that the shadow memory is not untagged in a landing pad and that
> after _Unwind_Resume the personality wrapper is run again, eventually
> returning _URC_CONTINUE_UNWIND and untagging the shadow memory.
>
>
> diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> index df7606d..ca97d72 100644
> --- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> +++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> @@ -1110,8 +1110,7 @@ bool HWAddressSanitizer::sanitizeFunction(Function
> &F) {
>              continue;
>            }
>
> -      if (isa<ReturnInst>(Inst) || isa<ResumeInst>(Inst) ||
> -          isa<CleanupReturnInst>(Inst))
> +      if (isa<ReturnInst>(Inst) ||
isa<CleanupReturnInst>(Inst))
>            RetVec.push_back(&INST);
>
>          if (auto *DDI = dyn_cast<DbgDeclareInst>(&Inst))
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191114/7e76be6b/attachment.html>