This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][CET] Enable CET for compiler-rt builtin library
ClosedPublic

Authored by xiongji90 on Sep 15 2021, 1:23 AM.

Details

Summary

Control-flow Enforcement Technology (CET), published by Intel, introduces indirect branch tracking(IBT) feature aiming to ensure the target address of an indirect jump/call is not tampered.
When IBT is enabled, each function or target of any indirect jump/call will start with an 'endbr32/64' instruction otherwise the program will crash during execution.
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

This patch aims to enable CET for compiler-rt builtins library, we add an option "COMPILER_RT_ENABLE_CET" whose default value is OFF to enable CET for compiler-rt in building time and when this option is "ON", "-fcf-protection=full" is added to BUILTINS_CFLAG and the "endbr32/64" will be placed in the beginning of each assembly function. And we also enable running all builtin tests with CET enabled when COMPILER_RT_ENABLE_CET is on. All builtin tests pass on CET enabled system.

Diff Detail

Event Timeline

xiongji90 created this revision.Sep 15 2021, 1:23 AM
xiongji90 requested review of this revision.Sep 15 2021, 1:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 1:23 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
xiongji90 added a subscriber: MaskRay.

Hi, @MaskRay
could you help review this patch?
Thanks very much.

efriedma added inline comments.Sep 15 2021, 11:30 AM
compiler-rt/lib/builtins/assembly.h
19

Should this be guarded by COMPILER_RT_ENABLE_CET somehow? Or does cet.h do that?

I'm a little concerned about the defined(linux) check, since the CMake lets you enable CET on non-Linux targets.

xiongji90 added inline comments.Sep 15 2021, 7:59 PM
compiler-rt/lib/builtins/assembly.h
19

Hi, @efriedma
You are correct, we need "CET" macro to guard here, I will update the patch.
Currently, CET is implemented on Linux + intel platform(x86_64/x86) and "cet.h" is used on Linux platform to provide some CET related macro defs.
This is why I check "linux" here. I know MS is also working on enabling CET on Windows platform but I have seen any Windows OS version published claiming to support CET. Enabling CET on a platform requires hardware, OS, toolchain, runtime libraries to fully support CET.

Thanks very much.

include <cet.h> and define CET endbr macros only when "CET" is defined. When using gcc or clang, "CET" is defined when -fcf-protection option is used.

xiongji90 marked an inline comment as done.Sep 15 2021, 11:20 PM
xiongji90 updated this revision to Diff 373387.Sep 17 2021, 8:32 PM
xiongji90 added inline comments.
compiler-rt/lib/builtins/assembly.h
17

CET is a x86/x86_64 specific macro, so remove the unnecessary x86/x86_64 macro check.

Hi, @compnerd , @MaskRay and @efriedma

Could you help review this patch? Thanks very much!

MaskRay added inline comments.Sep 22 2021, 11:39 PM
compiler-rt/CMakeLists.txt
74

This can be dropped.

if(NOT COMPILER_RT_HAS_FCF_PROTECTION_FLAG) takes care of it.

xiongji90 updated this revision to Diff 374751.Sep 24 2021, 1:30 AM
xiongji90 marked an inline comment as done.
xiongji90 added inline comments.Sep 24 2021, 1:47 AM
compiler-rt/CMakeLists.txt
262

Hi, @MaskRay
I moved the "-fcf-protection" option check here and dropped the "MSVC" check.
I also moved the "check_c_compiler_flag" for "-fcf-protection=full" into config-ix.cmake

MaskRay accepted this revision.Sep 24 2021, 12:17 PM

LGTM, but please give other reviewers some time for response.

compiler-rt/lib/builtins/i386/udivdi3.S
26

the indentation is off

compiler-rt/test/builtins/CMakeLists.txt
52

On non-x86 COMPILER_RT_ENABLE_CET should have failed when the control flow reaches here.

This revision is now accepted and ready to land.Sep 24 2021, 12:17 PM
MaskRay requested changes to this revision.Sep 24 2021, 12:20 PM

Ugh, wait. Can functions like __floatundisf be indirect call targets? Compilers generate direct calls to them and we don't expect users to indirectly call them.

I also don't find AArch64 bti notation.

This revision now requires changes to proceed.Sep 24 2021, 12:20 PM

I tried to use it locally and ran into following issues:

ld.lld: warning: /usr/lib64/clang/14.0.0/lib/linux/clang_rt.crtbegin-x86_64.o: -z force-ibt: file does not have GNU_PROPERTY_X86_FEATURE_1_IBT property
ld.lld: warning: /usr/lib64/clang/14.0.0/lib/linux/clang_rt.crtend-x86_64.o: -z force-ibt: file does not have GNU_PROPERTY_X86_FEATURE_1_IBT property

Apparently crt* files are not built with fcf-protection=full flag.

xiongji90 updated this revision to Diff 378114.Oct 8 2021, 12:40 AM
xiongji90 marked an inline comment as done.Oct 8 2021, 12:54 AM

I tried to use it locally and ran into following issues:

ld.lld: warning: /usr/lib64/clang/14.0.0/lib/linux/clang_rt.crtbegin-x86_64.o: -z force-ibt: file does not have GNU_PROPERTY_X86_FEATURE_1_IBT property
ld.lld: warning: /usr/lib64/clang/14.0.0/lib/linux/clang_rt.crtend-x86_64.o: -z force-ibt: file does not have GNU_PROPERTY_X86_FEATURE_1_IBT property

Apparently crt* files are not built with fcf-protection=full flag.

Hi, @manojgupta
The latest patch have also enable CET for crt objects, all CRT tests pass on CET platform when CET is enabled for compiler-rt.

Thanks very much.

compiler-rt/test/builtins/CMakeLists.txt
52

Hi, @MaskRay
If target arch is not i386 or x86_64, we will report fatal error now.

xiongji90 updated this revision to Diff 378126.Oct 8 2021, 1:36 AM
xiongji90 marked an inline comment as done.
xiongji90 marked an inline comment as done.Oct 8 2021, 1:55 AM

Ugh, wait. Can functions like __floatundisf be indirect call targets? Compilers generate direct calls to them and we don't expect users to indirectly call them.

I also don't find AArch64 bti notation.

Hi, @MaskRay
I agree that we don't expect users to call those builtin functions indirectly but compiler doesn't have any mechanism to forbid users to do that currently. As long as those builtin functions are exported, user can call them directly or indirectly and this may lead to CET IBT violation.
I suggest to keep the "endbr" here, keeping the "endbr" in builtin functions just adds several endbr instructions into final binary and it can make CET enabling more unified, all functions will start with "endbr" when IBT is enabled, it will also avoid crash when some users are playing tricks with those builtin function via indirect calls.

Thanks very much.

xiongji90 updated this revision to Diff 378136.Oct 8 2021, 2:20 AM

Ugh, wait. Can functions like __floatundisf be indirect call targets? Compilers generate direct calls to them and we don't expect users to indirectly call them.

I also don't find AArch64 bti notation.

Hi, @MaskRay
I agree that we don't expect users to call those builtin functions indirectly but compiler doesn't have any mechanism to forbid users to do that currently. As long as those builtin functions are exported, user can call them directly or indirectly and this may lead to CET IBT violation.

That will be pilot error. Unsupporting that use case is ok.

If there is legitimate use case (I doubt), we can add endbr as needed. The defensive approach is unnecessary.

I suggest to keep the "endbr" here, keeping the "endbr" in builtin functions just adds several endbr instructions into final binary and it can make CET enabling more unified, all functions will start with "endbr" when IBT is enabled, it will also avoid crash when some users are playing tricks with those builtin function via indirect calls.

I believe libgcc has much fewer endbr annotations.

Thanks very much.

Ugh, wait. Can functions like __floatundisf be indirect call targets? Compilers generate direct calls to them and we don't expect users to indirectly call them.

I also don't find AArch64 bti notation.

Hi, @MaskRay
I agree that we don't expect users to call those builtin functions indirectly but compiler doesn't have any mechanism to forbid users to do that currently. As long as those builtin functions are exported, user can call them directly or indirectly and this may lead to CET IBT violation.

That will be pilot error. Unsupporting that use case is ok.

If there is legitimate use case (I doubt), we can add endbr as needed. The defensive approach is unnecessary.

I suggest to keep the "endbr" here, keeping the "endbr" in builtin functions just adds several endbr instructions into final binary and it can make CET enabling more unified, all functions will start with "endbr" when IBT is enabled, it will also avoid crash when some users are playing tricks with those builtin function via indirect calls.

I believe libgcc has much fewer endbr annotations.

Thanks very much.

Hi, @MaskRay
I just "objdump" libgcc and find all builtin functions have endbr annotation. It seems that libgcc adopted the same strategy as we did in this patch. CET enabling for runtime libraries is transparent to users, if we remove those endbr annotations, I am afraid we may have to tell users indirectly calling those builtin functions in their source code in a CET enabled environment is forbidden which is a little confusing.
Thanks very much.

Hi, @MaskRay
I removed endbr in builtin .S file according to your suggestion, could you help review again?
Thanks very much.

If no customization is needed (like libc++/libc++abi D107560), why not just require explicit CXXFLAGS? I don't see a need for a new CMake option.

If no customization is needed (like libc++/libc++abi D107560), why not just require explicit CXXFLAGS? I don't see a need for a new CMake option.

Hi, @MaskRay
Compiler-rt does not have a way to pass custom flags like libc++ (LIBCXX_COMPILE_FLAGS), if we add CET flag to cxxflag, it will impact all components when we build compiler-rt with other components.
And adding an cmake option also provides us a way to run compiler-rt tests with CET enabled.
Thanks very much.

If no customization is needed (like libc++/libc++abi D107560), why not just require explicit CXXFLAGS? I don't see a need for a new CMake option.

Hi, @manojgupta
Do you think explicit CXXFLAGS is enough? If not, I am afraid we have to add CMake option.

Thanks very much.

Unfortunately, explicit CXXFLAGS is not enough. If we add CXXFLAGS, it will make all of llvm including clang to be built with CET which is not what we want.

compnerd requested changes to this revision.Oct 16 2021, 10:44 AM

I think that we might really want to subsequently rework the compiler-rt handling to use target_compile_options. This munging of flags manually is really rather onerous, but that is beyond the scope of this change.

compiler-rt/CMakeLists.txt
262–264

This is misrepresenting the state and is not really correct. Please change this. MSVC supports CET just as well with /CETCOMPAT. Please avoid using FATAL_ERROR, this allows any other configuration issues to be identified during the same run.

compiler-rt/lib/builtins/CMakeLists.txt
677

Please also ensure that the compiler supports the flag when adding it.

compiler-rt/lib/builtins/assembly.h
18

Please use the same thing we use elsewhere in compiler-rt:

#if !defined(__has_include)
#define __has_include(include) 0
#endif

Then you can unconditionally use __has_include.

compiler-rt/lib/crt/CMakeLists.txt
100

Similar - please ensure that the compiler supports the flag.

compiler-rt/test/builtins/CMakeLists.txt
53

Please use i?86|x86_64. Some targets use i686 and some use i386.

57

I think that it might be better to write this as:

if(COMPILER_RT_ENABLE_CET)
  if(NOT arch MATCHES "i?86|x86_64|AMD64")
    message(SEND_ERROR "${arch} does not support CET")
  endif()
  if(COMPILER_RT_HAS_FCF_PROTECTION_FLAG)
    list(APPEND BUILTINS_TEST_TARGET_CFLAGS -fcf-protection=full)
    string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS"${BUILTINS_TEST_TARGET_CFLAGS}")
  endif()
endif()
This revision now requires changes to proceed.Oct 16 2021, 10:44 AM
xiongji90 updated this revision to Diff 381925.Oct 25 2021, 3:59 AM
xiongji90 marked 5 inline comments as done.Oct 25 2021, 4:07 AM
xiongji90 added inline comments.
compiler-rt/lib/builtins/CMakeLists.txt
677

Done.

compiler-rt/lib/builtins/assembly.h
18

Done.

compiler-rt/lib/crt/CMakeLists.txt
100

Done.

compiler-rt/test/builtins/CMakeLists.txt
53

Done.

57

Done.

xiongji90 updated this revision to Diff 381927.Oct 25 2021, 4:10 AM
xiongji90 marked 5 inline comments as done.
xiongji90 added inline comments.
compiler-rt/CMakeLists.txt
262–264

Hi, @compnerd
I added "MSVC" check here to avoid impacting MSVC compiler and changed "FATAL_ERROR" to "SEND_ERROR", is it OK to you?

Thanks very much.

Hi, @compnerd , @MaskRay and @manojgupta
I have updated the patches according to previous comments, could you help review it?

Thanks very much.

@compnerd can you please check again.

Hi, @compnerd
Could you check again if the latest update meets your requirement?
Thanks very much.

compnerd accepted this revision.Dec 28 2021, 11:09 AM
compnerd added inline comments.
compiler-rt/CMakeLists.txt
53

I don't think that this needs to be mark_as_advanced.

MaskRay accepted this revision.Dec 28 2021, 11:25 AM
MaskRay added inline comments.
compiler-rt/lib/builtins/assembly.h
19

If __CET__ is defined (very new compiler), __has_include should be available, so you can simplify here a bit.

This revision is now accepted and ready to land.Dec 28 2021, 11:25 AM
xiongji90 updated this revision to Diff 397764.Jan 5 2022, 6:26 PM
xiongji90 set the repository for this revision to rG LLVM Github Monorepo.
xiongji90 marked 2 inline comments as done.Jan 5 2022, 6:28 PM

Hi, @MaskRay and @compnerd
I have updated the patch based on your comments and may need your approval again.
Thanks very much for the review.

compiler-rt/CMakeLists.txt
53

Done.
Thanks very much.

compiler-rt/lib/builtins/assembly.h
19

Done.
Thanks very much.

xiongji90 updated this revision to Diff 398472.Jan 9 2022, 5:30 PM
xiongji90 marked 2 inline comments as done.