Page MenuHomePhabricator

[libunwind][CET] Support exception handling stack unwind in CET environment
ClosedPublic

Authored by xiongji90 on Jul 14 2021, 1:47 AM.

Details

Summary

Control-flow Enforcement Technology (CET), published by Intel, introduces shadow stack feature aiming to ensure a return from a function is directed to where the function was called.
In a CET enabled system, each function call will push return address into normal stack and shadow stack, when the function returns, the address stored in shadow stack will be popped and compared with the return address, program will fail if the 2 addresses don't match.
In exception handling, the control flow may skip some stack frames and we must adjust shadow stack to avoid violating CET restriction.
In order to achieve this, we count the number of stack frames skipped and adjust shadow stack by this number before jumping to landing pad.
We have run libcxx, libcxxabi, libunwind tests with this patch in a CET enabled Fedora34 system, no failed tests found.
To build an application with CET enabled. we need to ensure:

  1. build the source code with "-fcf-protection=full"
  2. all the libraries linked with .o files must be CET enabled too

To run libcxx and libcxxabi tests with libunwind in a CET enabled system, we built libc++ and libc++abi.so with "-fcf-protection". Another patch has been uploaded for review to enable building llvm toolchian with CET enabled: https://reviews.llvm.org/D105603 This patch is not enough to enable all components but it does build libcxx, libcxxabi with CET enabled.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
xiongji90 added inline comments.Aug 1 2021, 8:42 PM
libunwind/src/UnwindLevel1.c
42

Hi, @hjl.tools
I tried to use the undocumented intrinsic "builtin_eh_return" to replace inline asm. The code for x86_64 is like following:
builtin_eh_return(0, libunwind_Registers_x86_64_jumpto);
With "-fcf-protection=full", both gcc and clang will use a "jmpq *%rcx" to jump to
libunwind_Registers_x86_64 which meets our requirement.
However, this intrinsic is used to jump to landing pad and expects to pass parameters via rax and rdx while libunwind_Registers_x86_64_jumpto is not a landing pad but a "normal" function which doesn't return. libunwind_Registers_x86_64_jumpto follows x86_64 abi to receive input parameters which means it expects input parameters from rdi, rsi.... And libunwind_Registers_x86_jumpto follows x86 abi to receive input parameters which means it expects input parameters stored in stack by caller. So I am afraid builtin_eh_return can't fully meet our requirement.
Thanks very much.

xiongji90 updated this revision to Diff 363381.EditedAug 1 2021, 11:16 PM

Chang default value of LIBUNWIND_ENABLE_CET to OFF in libunwind CMakeFile.
Since the LLVM_BUILD_CET_ENABLE has not been added to LLVM building system, we can set LIBUNWIND_ENABLE_CET default value to OFF currently.
When LLVM_BUILD_CET_ENABLE is supported in LLVM building, we can set the default value to LLVM_BUILD_CET_ENABLE.

xiongji90 added inline comments.Aug 2 2021, 1:39 AM
libunwind/src/UnwindLevel1.c
47

Hi, @compnerd
I think using combination of following:

  1. using attribute((noinline)) to decorate target functions
  2. building source file containing target functions with -fno-optimize-sibling-calls

can help us to get away with the issue mentioned above. I only found gcc/clang compiling options for "-fno-optimize-sibling-calls" and didn't find corresponding pragma or attribute, so if we choose to use "-fno-optimize-sibling-calls", tail call optimization for other functions will be disabled too which seems to be overshooting.
And if choosing the combination of "noinline" and "-fno-optimize-sibling-calls", we are relying on gcc/clang's implementation which is a implicit dependency, using macro here should be safe to all compilers.

Thanks very much.

xiongji90 added inline comments.
libunwind/src/UnwindLevel1.c
47

Hi, @compnerd
If you have concern about the macro "unw_phase_resume", I can try to update the patch to use combination of "noinline" and "-fno-optimize-sibling-calls". And do we have any way to use "no-optimize-sibling-calls" to decorate a function? I only found compiling option "-fno-optimize-sibling-calls".
Thanks very much.

Hi, @compnerd
I also uploaded a patch to enable building libc++ and libc++abi with CET enabled https://reviews.llvm.org/D107560
libc++ and libc++abi source code doesn't include any assembly, we just need to build the source with "-fcf-protection=full". Could you help review the libc++,libc++abi patch too?
BTW, if using combination of "noinline" and "-fno-optimize-sibling-calls" is the right approach, I will update the patch.
Thanks very much.

Have you checked check-cxxabi test/forced_unwind[12].pass.cpp? Do they pass with CET?

libunwind/src/UnwindLevel1.c
47

I think using a macro is a right call here, more robust than dancing with -noinline, no-optimize-sibling-calls and using functions and computing how many additional frames are skipped.

51

Question: why is this 1 + (fn)?

55

Is the calling convention here wrong?

173
227
291
libunwind/src/cet_unwind.h
21

Since CET is x86 only, (defined(_LIBUNWIND_TARGET_I386) || defined(_LIBUNWIND_TARGET_X86_64)) can be omitted.

Have you checked check-cxxabi test/forced_unwind[12].pass.cpp? Do they pass with CET?

Hi, @MaskRay
Yes, all libcxx and libcxxabi case including forced_unwind[12].pass.cpp have been tested using libunwind. Those "forced_unwind" test cases depend on _Unwind_ForcedUnwind and this patch also supports _Unwind_ForcedUnwind in CET system.
Thanks very much.

xiongji90 added inline comments.Aug 9 2021, 10:27 PM
libunwind/src/UnwindLevel1.c
51

Hi, @MaskRay
This is related to libunwind implementation for _Unwind_RaiseException/Resume and _Unwind_ForcedUnwind.
For example, following is _Unwind_RaiseException code:

/// Called by __cxa_throw. Only returns if there is a fatal error.
_LIBUNWIND_EXPORT _Unwind_Reason_Code
_Unwind_RaiseException(_Unwind_Exception *exception_object) {

_LIBUNWIND_TRACE_API("_Unwind_RaiseException(ex_obj=%p)",
                     (void *)exception_object);
unw_context_t uc;
unw_cursor_t cursor;
__unw_getcontext(&uc);

// Mark that this is a non-forced unwind, so _Unwind_Resume()
// can do the right thing.
exception_object->private_1 = 0;
exception_object->private_2 = 0;

// phase 1: the search phase
_Unwind_Reason_Code phase1 = unwind_phase1(&uc, &cursor, exception_object);
if (phase1 != _URC_NO_REASON)
  return phase1;

// phase 2: the clean up phase
return unwind_phase2(&uc, &cursor, exception_object);

}

In phase2, _Unwind_RaiseException will call unwind_phase2 and will never return if landing pad is found. So, we need to adjust CET shadow stack for "unwind_phase2" too.
In unwind_phase2, we will record how many stack frames we need to skip(starting from _Unwind_RaiseException and unwind_phase2 is not included), store the value into framesWalked. At last, we use _LIBUNWIND_POP_CET_SSP to adjust CET shadow stack by 1 + framesWalked frames.

It seems we can have a clear way to achieve this, initializing framesWalked to 1 instead 0 and add some comments to demonstrate the initialization is for unwind_phase2.
Thanks very much.

55

Hi, @MaskRay
The code here is to emulate x86 calling convention. As we know, x86 uses stack to pass input parameter and store return address.
We will jump to "libunwind_Registers_x86_jumpto" instead of calling it to avoid any change to CET shadow stack.
In libunwind, "
libunwind_Registers_x86_jumpto" is a pure x86 assembly function, the implementation expects the stack frame to be a standard x86 function call stack frame like following:

+______________+
+ input param +
+______________+
+ return addr +
+___________ +
.....
libunwind_Registers_x86_jumpto will never return, so the return address stored in stack will not be used. However, the implementation does rely on the stack layout, following code is from __libunwind_Registers_x86_jumpto:

DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_x86_jumpto)

  1. extern "C" void __libunwind_Registers_x86_jumpto(Registers_x86 *); #
  2. On entry:
  3. + +
  4. +-----------------------+
  5. + thread_state pointer +
  6. +-----------------------+
  7. + return address +
  8. +-----------------------+ <-- SP
  9. + + _LIBUNWIND_CET_ENDBR movl 4(%esp), %eax
    1. set up eax and ret on new stack location movl 28(%eax), %edx # edx holds new stack pointer subl $8,%edx movl %edx, 28(%eax) movl 0(%eax), %ebx

......
As we can see, we must ensure the input parameter is placed in 4(%esp), 0(%esp) stores dummy return address which will never be used.
So, after pushing input parameter for libunwind_Registers_x86_jumpto into stack, we have to "sub $4, %%esp" in order to emulate pushing return address into stack, the pushing is aimed to make stack layout to meet libunwind_Registers_x86_jumpto's requirement.
Thanks very much.

xiongji90 updated this revision to Diff 365384.Aug 10 2021, 1:52 AM
xiongji90 added inline comments.
libunwind/src/UnwindLevel1.c
173

Fixed.
Thanks very much.

227

Fixed.
Thanks very much.

291

Fixed.
Thanks very much.

Mostly looks good.

libunwind/CMakeLists.txt
184

add_compile_flags.

When LIBUNWIND_ENABLE_CET=on, it is an error if the compiler does not support the options.

libunwind/src/UnwindLevel1.c
55

sub $4, %%esp is for the dummay return address. Haven't used x86-32 assembly for a while and forgot this. Sorry!

You may just write asm instead of __asm__. Other places use asm directly as well.

173

git diff -U0 --no-color 'HEAD^' | clang/tools/clang-format/clang-format-diff.py -i -p1 reformat as needed.

libunwind/src/cet_unwind.h
18

on Linux x86 platforms.

libunwind/test/lit.site.cfg.in
30

perhaps config.x86_cet

  1. Check if compiler used to build libunwind supports CET compiling options.
  2. rename "cet_on" to "x86_cet"
xiongji90 added inline comments.Aug 10 2021, 11:25 PM
libunwind/CMakeLists.txt
184

Hi, @MaskRay
I update the patch to check "LIBUNWIND_SUPPORTS_FCF_PROTECTION_EQ_FULL_FLAG" and "LIBUNWIND_SUPPORTS_MSHSTK_FLAG", if either flag is not set, error will be reported.
Thanks very much.

libunwind/src/UnwindLevel1.c
55

Hi, @MaskRay
I also think using C++ keyword "asm" is better but libunwind source has already used "asm": https://github.com/llvm/llvm-project/blob/main/libunwind/src/config.h#L79 So I aligned with this style.
Thanks very much.

173

Fixed.
Thanks very much.

libunwind/src/cet_unwind.h
18

Fixed.
Thanks very much.

libunwind/test/lit.site.cfg.in
30

Fixed.
Thanks very much.

MaskRay added inline comments.Aug 11 2021, 12:32 AM
libunwind/src/cet_unwind.h
2

This file probably should just be moved into config.h, then other files don't need to add a new #include.

20

Perhaps rename to _LIBUNWIND_USE_CET

MaskRay accepted this revision.Aug 11 2021, 12:33 AM

Hi, @compnerd
Do you have any other concerns or comments?
Thanks very much.

MaskRay accepted this revision.Aug 12 2021, 12:56 PM

It'd be good to get compnerd's opinion as well.

This revision is now accepted and ready to land.Aug 12 2021, 12:56 PM
compnerd accepted this revision.Aug 20 2021, 1:44 PM

Minor comments about the implementation, but I think that this is reasonable.

libunwind/CMakeLists.txt
187

I would recommend SEND_ERROR here instead of FATAL_ERROR. This will allow getting subsequent errors as well.

190

I would recommend SEND_ERROR here instead of FATAL_ERROR.

libunwind/src/UnwindCursor.hpp
456

Most of the warnings indicate the name. I would suggest we follow the same convention to help quickly identify the function which is unimplemented.

libunwind/src/UnwindLevel1.c
56
57–58
66
67–68
libunwind/src/cet_unwind.h
11–12
18

Hi, @compnerd and @MaskRay
I just updated the patch to fix some code format/comments issue, do I need your approval again to commit this patch?
Thanks very much.

libunwind/CMakeLists.txt
187

Done.

Thanks very much.

190

Done.

Thanks very much.

libunwind/src/cet_unwind.h
11–12

Done.

Thanks very much.

18

Done.

Thanks very much.

MaskRay accepted this revision.Aug 23 2021, 5:58 PM

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

xiongji90 updated this revision to Diff 368265.Aug 23 2021, 9:07 PM
xiongji90 added inline comments.Aug 23 2021, 9:10 PM
libunwind/src/UnwindCursor.hpp
456

Done.

Thanks very much.

libunwind/src/UnwindLevel1.c
57–58

Done.

Thanks very much.

67–68

Done.

Thanks very much.

xiongji90 updated this revision to Diff 368299.Aug 24 2021, 1:35 AM

Rebase the patch to latest llvm and address some tiny issues.

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

Hi, @MaskRay
Left unresolved comments are resolved and I also rebased the patch to latest llvm libunwind code.

Thanks very much.

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

Hi, @MaskRay
Left unresolved comments are resolved and I also rebased the patch to latest llvm libunwind code.

Thanks very much.

You marked your own comments as "Done" but not reviewers' comments as "Done".

So I currently see 35 among 79 comments are done, while the rest are not.

Please mark resolved ones as done.

If you need a reviewer to commit this patch on your behalf, please provide git commit --amend --author="name <email>".

xiongji90 marked 41 inline comments as done.Aug 24 2021, 7:28 PM
xiongji90 updated this revision to Diff 368540.Aug 24 2021, 7:56 PM
xiongji90 marked an inline comment as done.
xiongji90 marked an inline comment as done.Aug 24 2021, 7:59 PM

There are still many unresolved comments. Please mark resolved ones as done and then click "Submit".

Hi, @MaskRay
Left unresolved comments are resolved and I also rebased the patch to latest llvm libunwind code.

Thanks very much.

You marked your own comments as "Done" but not reviewers' comments as "Done".

So I currently see 35 among 79 comments are done, while the rest are not.

Please mark resolved ones as done.

If you need a reviewer to commit this patch on your behalf, please provide git commit --amend --author="name <email>".

Hi, @MaskRay

Sorry for the mistake, I just marked all reviewers' comments as "Done". I just got the commit access and will commit the code soon.

Thanks very much for your kind help.

libunwind/src/cet_unwind.h
2

Hi, @MaskRay
I suggest to keep cet_unwind.h currently since CET is still a narrow used hardware feature right now and all macro definitions in config.h seems to have general usage. In the long term, if CET is enabled by default in x86/x86_64 platforms, we can move the contents in cet_unwind.h into config.h.

Thanks very much.

20

Done.
Thanks very much.

xiongji90 marked an inline comment as done.Aug 24 2021, 8:17 PM
This revision was landed with ongoing or failed builds.Aug 26 2021, 1:14 AM
This revision was automatically updated to reflect the committed changes.