This is an archive of the discontinued LLVM Phabricator instance.

[HWASan] Intercept setjmp/longjmp on x86_64.
ClosedPublic

Authored by morehouse on Sep 14 2021, 3:30 PM.

Diff Detail

Event Timeline

morehouse created this revision.Sep 14 2021, 3:30 PM
morehouse requested review of this revision.Sep 14 2021, 3:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2021, 3:30 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
xiangzhangllvm added inline comments.Sep 14 2021, 5:57 PM
compiler-rt/lib/hwasan/hwasan_setjmp_x86_64.S
37

We should insert endbr64 for CET enable program. It has no effect for non-CET program.

49–54

Why not use JB_xxx which defined in the compiler-rt ? That is more readable

// Save callee save registers.
  movq %rbx, (JB_RBX*8)(%rdi)
  movq %rbp, (JB_RBP*8)(%rdi)
  movq %r12, (JB_R12*8)(%rdi)
  movq %r13, (JB_R13*8)(%rdi)
  movq %r14, (JB_R14*8)(%rdi)
  movq %r15, (JB_R15*8)(%rdi)
58

here too

62

here too

64

we 'd better consider Shadow Stack for CET

// Check if Shadow Stack is enabled.
  testl $X86_FEATURE_1_SHSTK, %fs:FEATURE_1_OFFSET
  jz .L_skip_ssp

// Get the current Shadow-Stack-Pointer and save it.
  rdsspq %rax
  movq %rax, SHADOW_STACK_POINTER_OFFSET(%rdi)

// Make a tail call to __sigjmp_save; it takes the same args.
.L_skip_ssp:
  jmp __sigjmp_save
xiangzhangllvm added inline comments.Sep 14 2021, 6:05 PM
compiler-rt/lib/hwasan/hwasan_interceptors.cpp
68

Here should be no space

morehouse added inline comments.Sep 15 2021, 8:09 AM
compiler-rt/lib/hwasan/hwasan_interceptors.cpp
68

This spaces are inserted by clang-format. Looks funny here, but not sure I want to override clang-format on every change.

compiler-rt/lib/hwasan/hwasan_setjmp_x86_64.S
49–54

I considered doing this, but it seems inline assembly can't use those macros, so we don't get to use them in our longjmp implementation.

So rather than using macros in setjmp and constants in longjmp, I just used constants everywhere.

FWIW, the aarch64 implementation doesn't use macros either.

64

Is there a way I can test CET in QEMU?

I don't want to complicate things without a way to verify it works.

LGTM modulo the CET discussion

compiler-rt/lib/hwasan/hwasan_interceptors.cpp
68

+1 we do what the computer tells us to do for code formatting

xiangzhangllvm added inline comments.Sep 15 2021, 5:41 PM
compiler-rt/lib/hwasan/hwasan_setjmp_x86_64.S
64

Not sure it can be tested in QEMU, I didn't do that test. but current standard setjmp/longjmp has support it in glibc.

The C/C++ source code can be compiled with CET option, but asm code can not, we'd better consider it.

Or, at least, add a "TODO" here, if you worry about the testing.

morehouse marked an inline comment as done.Sep 16 2021, 5:49 AM
morehouse added inline comments.
compiler-rt/lib/hwasan/hwasan_setjmp_x86_64.S
64

TODO is already present on line 29.

morehouse marked an inline comment as done.Sep 16 2021, 10:01 AM
morehouse added inline comments.
compiler-rt/lib/hwasan/hwasan_setjmp_x86_64.S
64

Other assembly for x86_64 will also need to be adapted for CET (e.g., vfork interceptor).

I propose we wait until we can test properly and then update everything together.

xiangzhangllvm accepted this revision.Sep 16 2021, 5:32 PM

LGTM , thanks for your work for HWASAN !

This revision is now accepted and ready to land.Sep 16 2021, 5:32 PM
This revision was landed with ongoing or failed builds.Sep 17 2021, 7:11 AM
This revision was automatically updated to reflect the committed changes.

Hi @morehouse,

I see warnings like this with this patch:

23:54:36 [297/1283] Building ASM object compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.x86_64.dir/hwasan_setjmp_x86_64.S.o
23:54:36 <instantiation>:3:3: warning: __sigsetjmp changed binding to STB_WEAK
23:54:36   .weak __sigsetjmp
23:54:36   ^
23:54:36 <instantiation>:3:3: warning: _setjmp changed binding to STB_WEAK
23:54:36   .weak _setjmp
23:54:36   ^

I use gcc 9.3.0 with glibc 2.17 and linux 3.10.

Is there anything that should/could be done about the warnings?

Hi @morehouse,

I see warnings like this with this patch:

23:54:36 [297/1283] Building ASM object compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.x86_64.dir/hwasan_setjmp_x86_64.S.o
23:54:36 <instantiation>:3:3: warning: __sigsetjmp changed binding to STB_WEAK
23:54:36   .weak __sigsetjmp
23:54:36   ^
23:54:36 <instantiation>:3:3: warning: _setjmp changed binding to STB_WEAK
23:54:36   .weak _setjmp
23:54:36   ^

I use gcc 9.3.0 with glibc 2.17 and linux 3.10.

Is there anything that should/could be done about the warnings?

I'm unable to reproduce locally, but it looks like we've also had this warning on the aarch64 buildbots for a while.

Does https://reviews.llvm.org/D110178 silence the warning for you?

Hi @morehouse,

I see warnings like this with this patch:

23:54:36 [297/1283] Building ASM object compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.x86_64.dir/hwasan_setjmp_x86_64.S.o
23:54:36 <instantiation>:3:3: warning: __sigsetjmp changed binding to STB_WEAK
23:54:36   .weak __sigsetjmp
23:54:36   ^
23:54:36 <instantiation>:3:3: warning: _setjmp changed binding to STB_WEAK
23:54:36   .weak _setjmp
23:54:36   ^

I use gcc 9.3.0 with glibc 2.17 and linux 3.10.

Is there anything that should/could be done about the warnings?

I'm unable to reproduce locally, but it looks like we've also had this warning on the aarch64 buildbots for a while.

Does https://reviews.llvm.org/D110178 silence the warning for you?

Yes I just tested with the patch and the warnings went away. Thanks!