[RISC-V][HWASAN] Add tag mismatch routines for HWASAN required for RISC-V

[smd]

Summary

[3/9] patch series to port HWASAN for riscv64

Depends On D131340

[craig.topper]

compiler-rt/lib/hwasan/CMakeLists.txt

21

this should be alphabetized above x86_64

llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn

73

alphabetize

[kito-cheng]

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S

63

That might not work correctly for rv64if / lp64f, but that is rare combination, so I would suggest you add some #if/#error here to make sure that won't happen :)

[smd]

Addressing comments

[smd]

compiler-rt/lib/hwasan/CMakeLists.txt

21

Fixed, thanks

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S

63

Addressed this issue, thanks

llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn

73

Fixed, thanks

[vitalybuka]

LGTM, but someone who work RISC-V should take a look

[jrtc27]

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S

78

FLEN is irrelevant, only the floating-point ABI matters. This also doesn't fail for lp64q. What you want is:

• __riscv_float_abi_soft - do nothing
• __riscv_float_abi_double - save/restore
• anything else - error

[smd]

Addressing comments

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S

78

Makes sense, fixed. Thanks

[smd]

LGTM, but someone who work RISC-V should take a look

@kito-cheng @craig.topper could you please help to identify someone who could help with reviewing riscv part of these patch series?
Thanks

[vitalybuka]

In D131341#3726897, @smd wrote:

LGTM, but someone who work RISC-V should take a look

@kito-cheng @craig.topper could you please help to identify someone who could help with reviewing riscv part of these patch series?
Thanks

This is new code, which should not regress existing stuff.
So I am OK if you remove "blocker" I added to @kito-cheng.

[jrtc27]

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S

48

The size of jmp_buf, and the order of fields within it, is implementation-specific. Moreover, the latter is not part of the ABI, it can change AFAIK. Since longjmp gets intercepted too this is just about ok (though you haven't modified InternalLongjmp, and I don't get why that's implemented in C, that seems dodgy, then again so does this whole design that you're adding RISC-V support to), though technically you could end up storing to padding bytes in jmp_buf here that don't get copied if someone copies a jmp_buf (real-world code does copy jmp_buf's; tcsh is one example, speaking from experience of having broken jmp_buf copying).

82

Tail calls must use tail not j otherwise you run into issues once the object this is linked into gets too big as JAL only has a 20-bit immediate (the exception is for jumping to functions within the same section in the same file, as those end up in a single input section and can't be split up so will be near each other in the output)

100

This is an AArch64-ism (BTI and PAC are AArch64 extension names) so doesn't belong in RISC-V-specific code (though does *work* because it's #define'd to nothing on non-AArch64 so it can be used in architecture-independent assembly files, currently just the vfork interceptor wrapper).

compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S

70

ASM_TYPE_FUNCTION?

125

call or tail? Bit weird to have a call at the end of a function, returning to it isn't useful and will fall off the end, though the AArch64 code does use bl not b so the equivalent is call...

128–129

ASM_SIZE?

[smd]

Addressing comments

[smd]

Fixed missing label

[smd]

@jrtc27 I would like to thank you for your such elaborate comments, your help is much appreciated. Thanks!

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S

82

Fixed, thanks

100

Fixed, thanks

compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S

70

Fixed, thanks

125

call here is used intentionally to get the address of previous frame: later while printing backtrace it allows to skip all service hwasan frames. I agree it's not obvious, maybe I should add a comment there? Thanks

128–129

Fixed, thanks

compiler-rt/lib/hwasan/CMakeLists.txt

set(HWASAN_RTL_SOURCES
   hwasan_globals.cpp
   hwasan_interceptors.cpp
   hwasan_interceptors_vfork.S
   hwasan_linux.cpp
   hwasan_memintrinsics.cpp
   hwasan_poisoning.cpp
   hwasan_report.cpp
   hwasan_setjmp_aarch64.S
+  hwasan_setjmp_riscv64.S
   hwasan_setjmp_x86_64.S
   hwasan_tag_mismatch_aarch64.S
+  hwasan_tag_mismatch_riscv64.S
   hwasan_thread.cpp
   hwasan_thread_list.cpp
   hwasan_type_test.cpp
   )
 
 set(HWASAN_RTL_CXX_SOURCES
   hwasan_new_delete.cpp
   )

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S

+//===-- hwasan_setjmp_riscv64.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.
+// setjmp interceptor for risc-v.
+// HWAddressSanitizer runtime.
+//===----------------------------------------------------------------------===//
+
+#include "sanitizer_common/sanitizer_asm.h"
+#include "builtins/assembly.h"
+
+#if HWASAN_WITH_INTERCEPTORS && defined(__riscv) && (__riscv_xlen == 64)
+#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_riscv64.S"
+
+.global __interceptor_setjmp
+ASM_TYPE_FUNCTION(__interceptor_setjmp)
+__interceptor_setjmp:
+  CFI_STARTPROC
+  addi x11, x0, 0
+  j    __interceptor_sigsetjmp
+  CFI_ENDPROC
+ASM_SIZE(__interceptor_setjmp)
+
+.global __interceptor_sigsetjmp
+ASM_TYPE_FUNCTION(__interceptor_sigsetjmp)
+__interceptor_sigsetjmp:
+  CFI_STARTPROC
+  sd    ra,   0<<3(x10)
+  sd    s0,   1<<3(x10)
+  sd    s1,   2<<3(x10)
+  sd    s2,   3<<3(x10)
+  sd    s3,   4<<3(x10)
+  sd    s4,   5<<3(x10)
+  sd    s5,   6<<3(x10)
+  sd    s6,   7<<3(x10)
+  sd    s7,   8<<3(x10)
+  sd    s8,   9<<3(x10)
+  sd    s9,   10<<3(x10)
+  sd    s10,  11<<3(x10)
+  sd    s11,  12<<3(x10)
+  sd    sp,   13<<3(x10)
+#if __riscv_float_abi_double
+  fsd   fs0,  14<<3(x10)
+  fsd   fs1,  15<<3(x10)
+  fsd   fs2,  16<<3(x10)
+  fsd   fs3,  17<<3(x10)
+  fsd   fs4,  18<<3(x10)
+  fsd   fs5,  19<<3(x10)
+  fsd   fs6,  20<<3(x10)
+  fsd   fs7,  21<<3(x10)
+  fsd   fs8,  22<<3(x10)
+  fsd   fs9,  23<<3(x10)
+  fsd   fs10, 24<<3(x10)
+  fsd   fs11, 25<<3(x10)
+#elif __riscv_float_abi_soft
+#else
+# error "Unsupported case"
+#endif
+  // 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
+  tail __sigjmp_save
+  CFI_ENDPROC
+ASM_SIZE(__interceptor_sigsetjmp)
+
+
+.macro WEAK_ALIAS first second
+  .weak \second
+  .equ \second\(), \first
+.endm
+
+WEAK_ALIAS __interceptor_sigsetjmp, __sigsetjmp
+
+WEAK_ALIAS __interceptor_setjmp, _setjmp
+#endif
+
+// We do not need executable stack.
+NO_EXEC_STACK_DIRECTIVE

compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S

+#include "sanitizer_common/sanitizer_asm.h"
+
+// The content of this file is RISCV64-only:
+#if defined(__riscv) && (__riscv_xlen == 64)
+
+// The responsibility of the HWASan entry point in compiler-rt is to primarily
+// readjust the stack from the callee and save the current register values to
+// the stack.
+// This entry point function should be called from a __hwasan_check_* symbol.
+// These are generated during a lowering pass in the backend, and are found in
+// RISCVAsmPrinter::EmitHwasanMemaccessSymbols(). Please look there for
+// further information.
+// The __hwasan_check_* caller of this function should have expanded the stack
+// and saved the previous values of x10(arg0), x11(arg1), x1(ra), and x8(fp).
+// This function will "consume" these saved values and treats it as part of its
+// own stack frame. In this sense, the __hwasan_check_* callee and this function
+// "share" a stack frame. This allows us to omit having unwinding information
+// (.cfi_*) present in every __hwasan_check_* function, therefore reducing binary size.
+// This is particularly important as hwasan_check_* instances are duplicated in every
+// translation unit where HWASan is enabled.
+// This function calls HwasanTagMismatch to step back into the C++ code that
+// completes the stack unwinding and error printing. This function is is not
+// permitted to return.
+
+
+// |              ...                |
+// |              ...                |
+// | Previous stack frames...        |
+// +=================================+
+// |              ...                |
+// |                                 |
+// | Stack frame space for x12 - x31.|
+// |                                 |
+// |              ...                |
+// +---------------------------------+ <-- [SP + 96]
+// | Saved x11(arg1), as             |
+// | __hwasan_check_* clobbers it.   |
+// +---------------------------------+ <-- [SP + 88]
+// | Saved x10(arg0), as             |
+// | __hwasan_check_* clobbers it.   |
+// +---------------------------------+ <-- [SP + 80]
+// |                                 |
+// | Stack frame space for x9.       |
+// +---------------------------------+ <-- [SP + 72]
+// |                                 |
+// | Saved x8(fp), as                |
+// | __hwasan_check_* clobbers it.   |
+// +---------------------------------+ <-- [SP + 64]
+// |              ...                |
+// |                                 |
+// | Stack frame space for x2 - x7.  |
+// |                                 |
+// |              ...                |
+// +---------------------------------+ <-- [SP + 16]
+// | Return address (x1) for caller  |
+// | of __hwasan_check_*.            |
+// +---------------------------------+ <-- [SP + 8]
+// | Reserved place for x0, possibly |
+// | junk, since we don't save it.   |
+// +---------------------------------+ <-- [x2 / SP]
+
+// This function takes two arguments:
+//   * x10/a0: The data address.
+//   * x11/a1: The encoded access info for the failing access.
+
+.section .text
+.file "hwasan_tag_mismatch_riscv64.S"
+
+.global __hwasan_tag_mismatch_v2
+ASM_TYPE_FUNCTION(__hwasan_tag_mismatch_v2)
+__hwasan_tag_mismatch_v2:
+  CFI_STARTPROC
+
+  // Set the CFA to be the return address for caller of __hwasan_check_*. Note
+  // that we do not emit CFI predicates to describe the contents of this stack
+  // frame, as this proxy entry point should never be debugged. The contents
+  // are static and are handled by the unwinder after calling
+  // __hwasan_tag_mismatch. The frame pointer is already correctly setup
+  // by __hwasan_check_*.
+  addi fp, sp, 256
+  CFI_DEF_CFA(fp, 0)
+  CFI_OFFSET(ra, -248)
+  CFI_OFFSET(fp, -192)
+
+  // Save the rest of the registers into the preallocated space left by
+  // __hwasan_check.
+  sd x31, 248(sp)
+  sd x30, 240(sp)
+  sd x29, 232(sp)
+  sd x28, 224(sp)
+  sd x27, 216(sp)
+  sd x26, 208(sp)
+  sd x25, 200(sp)
+  sd x24, 192(sp)
+  sd x23, 184(sp)
+  sd x22, 176(sp)
+  sd x21, 168(sp)
+  sd x20, 160(sp)
+  sd x19, 152(sp)
+  sd x18, 144(sp)
+  sd x17, 136(sp)
+  sd x16, 128(sp)
+  sd x15, 120(sp)
+  sd x14, 112(sp)
+  sd x13, 104(sp)
+  sd x12, 96(sp)
+  // sd x11, 88(sp) ; already saved
+  // sd x10, 80(sp) ; already saved
+  sd x9, 72(sp)
+  // sd x8, 64(sp) ; already saved
+  sd x7, 56(sp)
+  sd x6, 48(sp)
+  sd x5, 40(sp)
+  sd x4, 32(sp)
+  sd x3, 24(sp)
+  sd x2, 16(sp)
+  // sd x1, 8(sp) ; already saved
+  // sd x0, 0(sp) ; don't store zero register
+
+  // Pass the address of the frame to __hwasan_tag_mismatch4, so that it can
+  // extract the saved registers from this frame without having to worry about
+  // finding this frame.
+  mv x12, sp
+
+  call __hwasan_tag_mismatch4
+  CFI_ENDPROC
+ASM_SIZE(__hwasan_tag_mismatch_v2)
+
+#endif  // defined(__riscv) && (__riscv_xlen == 64)
+
+// We do not need executable stack.
+NO_EXEC_STACK_DIRECTIVE

llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn

sources = [
     "hwasan_malloc_bisect.h",
     "hwasan_mapping.h",
     "hwasan_memintrinsics.cpp",
     "hwasan_poisoning.cpp",
     "hwasan_poisoning.h",
     "hwasan_report.cpp",
     "hwasan_report.h",
     "hwasan_setjmp_aarch64.S",
+    "hwasan_setjmp_riscv64.S",
     "hwasan_setjmp_x86_64.S",
     "hwasan_tag_mismatch_aarch64.S",
+    "hwasan_tag_mismatch_riscv64.S",
     "hwasan_thread.cpp",
     "hwasan_thread.h",
     "hwasan_thread_list.cpp",
     "hwasan_thread_list.h",
     "hwasan_type_test.cpp",
   ]
 }