Add support for LLVM's Control Flow Integrity (V2)

Submitted by Charlotte Delenk on Dec. 28, 2020, 1:17 p.m.

Details

Message ID 865734a3-e4c6-eb06-5855-db74c00fb071@darkkirb.de
State New
Series "Add support for LLVM's Control Flow Integrity"
Headers show

Commit Message

Charlotte Delenk Dec. 28, 2020, 1:17 p.m.
Control Flow Integrity is a sanitization option found in clang which
attempts to prevent exploits and bugs that divert the control flow to an
unintended path. For more information about it, refer to clang's
documentation[1].

While there are many different schemes currently implemented, the only
one that is enabled for C code is the cfi-icall scheme, which attempts
to prevent indirect calls to function with the wrong type. In most of
musl's code this works without issues, however there are a few cases
where it does not work, or at least won't work without breaking a
considerable amount of applications.

This patch force-disables CFI for the following functions:

libc_start_main_stage2: Requries main() to take all 3 arguments and
return an int. Standard C defines a 0 and 2 argument variant which are
used by most applications.

libc_start_init: This function calls static initializers with the type
void(*)(void). The actual function signature for static initializers are
undefined and it is not the same type as the one used by clang for c++
static initializers.

libc_exit_fini: Same as libc_start_init, just with static destructors.

_dlstart_c: The compiler can't determine the type of the __dls2
function. I am not sure why exactly, because it can do that with the
ctors, dtors and main. Might be because of the inline assembly.

__dls2: The compiler can't determine the type of the __dls2b function
that is called indirectly

Additionally the patch changes the following functions:

__dls2: LTO (a requirement for CFI) will errorneously determine that
this symbol is unused. We have to tell the compiler that the symbol is
in fact used.

__stack_chk_guard: Same as __dls2
__dls2b: Same as __dls2
__dls3: Same as __dls2

Despite making changes to the dynamic loader portion, this patch
currently does not allow to use CFI in dynamically linked applications,
as it would either require a runtime library or break every function
that uses a non-libc function pointer.

I have checked all of the places with indirect function calls using the
output of Fangrui's clang tidy patch and only found the aforementioned
functions.

How to test: In addition to the -fsanitize=cfi flag, you also need to pass
-flto=thin and -fvisibility=default (or hidden in a static build). The
application has to be compiled and linked with the same flags as well.
You might need to set the environment variables AR=llvm-ar and RANLIB=
llvm-ranlib for musl or the software you are compiling.

[1]: https://clang.llvm.org/docs/ControlFlowIntegrity.html
Special thanks to Fangrui Song <i@maskray.me>

---
  ldso/dlstart.c              |  5 +++++
  ldso/dynlink.c              | 15 ++++++++++++---
  src/env/__libc_start_main.c |  9 +++++++++
  src/env/__stack_chk_fail.c  |  3 +++
  src/exit/exit.c             |  4 ++++
  5 files changed, 33 insertions(+), 3 deletions(-)

  {
      uintptr_t a = (uintptr_t)&__fini_array_end;

Patch hide | download patch | download mbox

diff --git a/ldso/dlstart.c b/ldso/dlstart.c
index 20d50f2c..f32177f4 100644
--- a/ldso/dlstart.c
+++ b/ldso/dlstart.c
@@ -18,6 +18,11 @@ 
      *(fp) = static_func_ptr; } while(0)
  #endif

+
+#ifdef __clang__
+/* function is incompatible with CFI */
+__attribute__((no_sanitize("cfi")))
+#endif
  hidden void _dlstart_c(size_t *sp, size_t *dynv)
  {
      size_t i, aux[AUX_CNT], dyn[DYN_CNT];
diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 6b868c84..f9c77c66 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1637,7 +1637,12 @@  static void install_new_tls(void)
   * symbols. Its job is to perform symbolic relocations on the dynamic
   * linker itself, but some of the relocations performed may need to be
   * replaced later due to copy relocations in the main program. */
-
+#ifdef __clang__
+/* workaround for compiling ldso with LTO with clang. This forces the
+ * linker to emit this function, even though it is "seemingly" unused
+ */
+__attribute__((used))
+#endif
  hidden void __dls2(unsigned char *base, size_t *sp)
  {
      size_t *auxv;
@@ -1702,7 +1707,9 @@  hidden void __dls2(unsigned char *base, size_t *sp)
   * This is done as a separate stage, with symbolic lookup as a barrier,
   * so that loads of the thread pointer and &errno can be pure/const and
   * thereby hoistable. */
-
+#ifdef __clang__
+__attribute__((used))
+#endif
  void __dls2b(size_t *sp, size_t *auxv)
  {
      /* Setup early thread pointer in builtin_tls for ldso/libc itself to
@@ -1725,7 +1732,9 @@  void __dls2b(size_t *sp, size_t *auxv)
   * fully functional. Its job is to load (if not already loaded) and
   * process dependencies and relocations for the main application and
   * transfer control to its entry point. */
-
+#ifdef __clang__
+__attribute__((used))
+#endif
  void __dls3(size_t *sp, size_t *auxv)
  {
      static struct dso app, vdso;
diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
index 8fbe5262..9eaeda17 100644
--- a/src/env/__libc_start_main.c
+++ b/src/env/__libc_start_main.c
@@ -56,6 +56,12 @@  void __init_libc(char **envp, char *pn)
      libc.secure = 1;
  }

+#ifdef __clang__
+/* Disable CFI sanitization as the constructors don't necessarily
+ * have the correct void(void) type (didn't work with c++ static
+ * constructors */
+__attribute__((no_sanitize("cfi")))
+#endif
  static void libc_start_init(void)
  {
      _init();
@@ -85,6 +91,9 @@  int __libc_start_main(int (*main)(int,char **,char 
**), int argc, char **argv)
      return stage2(main, argc, argv);
  }

+#ifdef __clang__
+__attribute__((no_sanitize("cfi")))
+#endif
  static int libc_start_main_stage2(int (*main)(int,char **,char **), 
int argc, char **argv)
  {
      char **envp = argv+argc+1;
diff --git a/src/env/__stack_chk_fail.c b/src/env/__stack_chk_fail.c
index bf5a280a..adbac9b7 100644
--- a/src/env/__stack_chk_fail.c
+++ b/src/env/__stack_chk_fail.c
@@ -2,6 +2,9 @@ 
  #include <stdint.h>
  #include "pthread_impl.h"

+#ifdef __clang__
+__attribute__((used))
+#endif
  uintptr_t __stack_chk_guard;

  void __init_ssp(void *entropy)
diff --git a/src/exit/exit.c b/src/exit/exit.c
index a6869b37..3be952e6 100644
--- a/src/exit/exit.c
+++ b/src/exit/exit.c
@@ -14,6 +14,10 @@  weak_alias(dummy, _fini);

  extern weak hidden void (*const __fini_array_start)(void), (*const 
__fini_array_end)(void);

+#ifdef __clang__
+/* static destructor might not be of the correct type, just like the 
initializer */
+__attribute__((no_sanitize("cfi")))
+#endif
  static void libc_exit_fini(void)

Comments

Shiz Dec. 28, 2020, 5:01 p.m.
Hi,

It seems to me guarding all of those with #ifdef __clang__ is not the optimal approach.
What if another compilers decides to pick up the spec, or an older non-CFI
Clang is used to compile? I think it's more logical to check the no_sanitize attribute
with a functional test (like __has_attribute), and to just apply the used attribute
unconditionally.

- Shiz
Rich Felker Dec. 29, 2020, 1:26 a.m.
On Mon, Dec 28, 2020 at 06:01:45PM +0100, Shiz wrote:
> Hi,
> 
> It seems to me guarding all of those with #ifdef __clang__ is not the optimal approach.
> What if another compilers decides to pick up the spec, or an older non-CFI
> Clang is used to compile? I think it's more logical to check the no_sanitize attribute
> with a functional test (like __has_attribute), and to just apply the used attribute
> unconditionally.

Regardless none of these changes belong in source files.

Assuming it actually can be made to work to begin with, the nocfi
stuff should be done on a file-granularity basis with per-target
CFLAGS += ...

The "used" attributes are working around some other problem, either a
bug in clang (sounds most likely) or lack of semantic usedness in musl
source, and should be fixed wherever the bug is not papered over.

Rich
Charlotte Delenk Dec. 29, 2020, 10:20 a.m.
On 12/28/20 6:01 PM, Shiz wrote:
> Hi,
>
> It seems to me guarding all of those with #ifdef __clang__ is not the optimal approach.
> What if another compilers decides to pick up the spec, or an older non-CFI
> Clang is used to compile? I think it's more logical to check the no_sanitize attribute
> with a functional test (like __has_attribute),
As Rich Felker wrote in an email i haven't yet received in my mailbox, 
he thinks it's better to put the exceptions in the Makefile. I'm going 
to update the patch accordingly (and split in two, one for GCC and LLVM 
LTO fixes and one for CFI)
> and to just apply the used attribute unconditionally.
The used attribute is a workaround for a LLVM bug that has existed for 
quite some time. GCC does not have a problem with the functions marked 
used, but instead requires _dlstart_c to be marked as such.
>
> - Shiz
>