mailing list of musl libc
 help / color / mirror / Atom feed
* [musl] [PATCH] Add support for LLVM's Control Flow Integrity
@ 2020-12-27 17:53 Charlotte Delenk
  2020-12-27 23:05 ` Fangrui Song
  0 siblings, 1 reply; 10+ messages in thread
From: Charlotte Delenk @ 2020-12-27 17:53 UTC (permalink / raw)
  To: musl

Hi,

I have attempted to use musl HEAD together with clang's -fsanitize=cfi,
but currently it requires the main function to take all 3 arguments and
return an int.

After this patch is applied, clang will no longer try to add CFI
sanitization to the libc_start_main_stage2 function, allowing programs
to get to main().

I have tested CFI sanitization for both regular indirect functions
(qsort()) and thread creation and validly typed function pointers cause
no runtime aborts with CFI enabled for the whole program.

---

  src/env/__libc_start_main.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
index 8fbe5262..af61fb7c 100644
--- a/src/env/__libc_start_main.c
+++ b/src/env/__libc_start_main.c
@@ -85,6 +85,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;
-- 
2.29.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH] Add support for LLVM's Control Flow Integrity
  2020-12-27 17:53 [musl] [PATCH] Add support for LLVM's Control Flow Integrity Charlotte Delenk
@ 2020-12-27 23:05 ` Fangrui Song
  2020-12-28  0:56   ` Fangrui Song
  0 siblings, 1 reply; 10+ messages in thread
From: Fangrui Song @ 2020-12-27 23:05 UTC (permalink / raw)
  To: musl

On 2020-12-27, Charlotte Delenk wrote:
>Hi,
>
>I have attempted to use musl HEAD together with clang's -fsanitize=cfi,
>but currently it requires the main function to take all 3 arguments and
>return an int.
>
>After this patch is applied, clang will no longer try to add CFI
>sanitization to the libc_start_main_stage2 function, allowing programs
>to get to main().
>
>I have tested CFI sanitization for both regular indirect functions
>(qsort()) and thread creation and validly typed function pointers cause
>no runtime aborts with CFI enabled for the whole program.
>
>---
>
> src/env/__libc_start_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
>index 8fbe5262..af61fb7c 100644
>--- a/src/env/__libc_start_main.c
>+++ b/src/env/__libc_start_main.c
>@@ -85,6 +85,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;
>-- 
>2.29.2
>

tl;dr This is insufficient.


Some background about CFI for other readers

Clang -fsanitize=cfi includes several checks [0]. Most are C++ specific and I
think only -fsanitize=cfi-icall (forward-edge CFI for indirect function calls
[1]) is relevant to C. CFI requires -flto={thin,full}.

[0]: https://clang.llvm.org/docs/ControlFlowIntegrity.html
[1]: https://clang.llvm.org/docs/ControlFlowIntegrityDesign.html#forward-edge-cfi-for-indirect-function-calls

For libc, we can ignore all the other CFI checks.

ldso/ has several indirect function calls which are incompatible with -fsanitize=cfi-icall

ldso/dlstart.c
In _dlstart_c, dls2((void *)base, sp); The address is obtained via GETFUNCSYM (inline asm) - Clang does not know the original function type and thus the llvm.type.test check will fail. The codegen places functions of the same signature consecutively and checks whether the indirect function is a known function in this list - an unknown function will fail.

(Another problem: Clang tracks undefined symbols in module-level inline asm but does not know undefined symbols in function-scope inline asm.
If you build this file with LTO, the LTO backend will internalize __dls2 and cause a subsequent "undefined reference" error unless you specify -u __dls2
to mark __dls2 as "visibible to a regular object file" (a workaround))

ldso/dynlink.c
In __dls2, ((stage3_func)laddr(&ldso, dls2b_def.sym->st_value))(sp, auxv);
In do_init_fini, fpaddr(p, dyn[DT_INIT])();

src/env/__libc_start_main.c
In libc_start_main_stage2,
In libc_start_init, (

Adding this line to Makefile is a simple way to disable LTO and CFI

   obj/ldso/dlstart.lo obj/ldso/dynlink.lo: CFLAGS+=-fno-lto -fno-sanitize=cfi

(LTO has to be disabled, otherwise LTO will error "inconsistent LTO Unit splitting (recompile with -fsplit-lto-unit)")

With the above, I can build musl with CFI:

   mkdir -p out/lto
   cd out/lto
   ../../configure CC=clang CFLAGS='-g -flto=thin -fsanitize=cfi-icall' LDFLAGS='-fuse-ld=lld'

---

src/env/__libc_start_main.c:libc_start_main_stage2 has a problem.
Other functions with external hooks can also have problems as well, e.g. src/exit/atexit.c:call ((void (*)(void))(uintptr_t)p)(); may fail.

These functions may reside in either libc.so or libc.a. Let's discuss both.

## libc.so

-fsanitize=cfi-icall alone fundamentally does not work due to hooks.
So the experimental -fsanitize=cfi-icall -fsanitize-cfi-cross-dso is needed:
instead of trapping immediately, Clang emits a call to __cfi_slowpath to check
whether the function is defined externally.

However, -shared -fsanitize-cfi-cross-dso does not currently have a good link story.
clang -shared does not link in libclang_rt.cfi-*.a, so __cfi_slowpath call will be external.
One can still build musl with

   ../../configure CC=clang CFLAGS='-g -flto=thin -fsanitize=cfi-icall -fsanitize-cfi-cross-dso' LDFLAGS='-fuse-ld=lld -z undefs'

(libclang_rt.cfi-* call musl-unsupported dlvsym. This can be worked around with

--- i/src/ldso/x86_64/dlsym.s
+++ w/src/ldso/x86_64/dlsym.s
@@ -5,3 +5,10 @@
  dlsym:
         mov (%rsp),%rdx
         jmp __dlsym
+
+.global dlvsym
+.hidden __dlvsym
+.type dlvsym,@function
+dlvsym:
+       mov (%rsp),%rdx
+       jmp __dlsym
)

but such a built libc.so requires the main executable to link in libclang_rt.cfi-*.a, i.e. the main executable has to
be built with -fsanitize=cfi-icall (or another CFI check) -fsanitize-cfi-cross-dso.

(There is currently an unrelated llvm.ubsantrap bug (the intrinsic somehow is not lowered)
% ~/musl/out/cross-dso/obj/musl-clang a.c -fsanitize=cfi-icall -flto -fvisibility=default -fsanitize-cfi-cross-dso
ld: error: undefined symbol: llvm.ubsantrap
>>> referenced by crt1.c:0 (../../crt/crt1.c:0)
>>>               lto.tmp:(__cfi_check_fail)
clang-12: error: linker command failed with exit code 1 (use -v to see invocation)
)

% ~/musl/out/cross-dso/obj/musl-clang a.c -fsanitize=cfi-icall -flto -fvisibility=default -fsanitize-cfi-cross-dso -Wl,--defsym=llvm.ubsantrap=0
% ./a.out
Error relocating libc.so: __cfi_slowpath: symbol not found

The diagnostic is from __dls2.  The issue is that musl expects its relocations can be resolved by itself.

## libc.a

For a -static link, there are additional failures in:

src/env/__libc_start_main.c
In libc_start_init, (*(void (**)(void))a)();
a is linker synthesized __init_array_start

src/exit/exit.c
In libc_exit_fini, (*(void (**)())(a-sizeof(void(*)())))();

After annotating the above:

% ~/musl/out/cross-dso/obj/musl-clang a.c -fsanitize=cfi-icall -flto -fvisibility=default -fsanitize-cfi-cross-dso -static -Wl,--defsym=llvm.ubsantrap=0
% ./a.out   # succeeded

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH] Add support for LLVM's Control Flow Integrity
  2020-12-27 23:05 ` Fangrui Song
@ 2020-12-28  0:56   ` Fangrui Song
  2020-12-28  9:20     ` Charlotte Delenk
  0 siblings, 1 reply; 10+ messages in thread
From: Fangrui Song @ 2020-12-28  0:56 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 17106 bytes --]


On 2020-12-27, Fangrui Song wrote:
>On 2020-12-27, Charlotte Delenk wrote:
>>Hi,
>>
>>I have attempted to use musl HEAD together with clang's -fsanitize=cfi,
>>but currently it requires the main function to take all 3 arguments and
>>return an int.
>>
>>After this patch is applied, clang will no longer try to add CFI
>>sanitization to the libc_start_main_stage2 function, allowing programs
>>to get to main().
>>
>>I have tested CFI sanitization for both regular indirect functions
>>(qsort()) and thread creation and validly typed function pointers cause
>>no runtime aborts with CFI enabled for the whole program.
>>
>>---
>>
>> src/env/__libc_start_main.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>>diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
>>index 8fbe5262..af61fb7c 100644
>>--- a/src/env/__libc_start_main.c
>>+++ b/src/env/__libc_start_main.c
>>@@ -85,6 +85,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;
>>-- 
>>2.29.2
>>
>
>tl;dr This is insufficient.
>
>
>Some background about CFI for other readers
>
>Clang -fsanitize=cfi includes several checks [0]. Most are C++ specific and I
>think only -fsanitize=cfi-icall (forward-edge CFI for indirect function calls
>[1]) is relevant to C. CFI requires -flto={thin,full}.
>
>[0]: https://clang.llvm.org/docs/ControlFlowIntegrity.html
>[1]: https://clang.llvm.org/docs/ControlFlowIntegrityDesign.html#forward-edge-cfi-for-indirect-function-calls
>
>For libc, we can ignore all the other CFI checks.
>
>ldso/ has several indirect function calls which are incompatible with -fsanitize=cfi-icall
>
>ldso/dlstart.c
>In _dlstart_c, dls2((void *)base, sp); The address is obtained via GETFUNCSYM (inline asm) - Clang does not know the original function type and thus the llvm.type.test check will fail. The codegen places functions of the same signature consecutively and checks whether the indirect function is a known function in this list - an unknown function will fail.
>
>(Another problem: Clang tracks undefined symbols in module-level inline asm but does not know undefined symbols in function-scope inline asm.
>If you build this file with LTO, the LTO backend will internalize __dls2 and cause a subsequent "undefined reference" error unless you specify -u __dls2
>to mark __dls2 as "visibible to a regular object file" (a workaround))
>
>ldso/dynlink.c
>In __dls2, ((stage3_func)laddr(&ldso, dls2b_def.sym->st_value))(sp, auxv);
>In do_init_fini, fpaddr(p, dyn[DT_INIT])();
>
>src/env/__libc_start_main.c
>In libc_start_main_stage2,
>In libc_start_init, (
>
>Adding this line to Makefile is a simple way to disable LTO and CFI
>
>  obj/ldso/dlstart.lo obj/ldso/dynlink.lo: CFLAGS+=-fno-lto -fno-sanitize=cfi
>
>(LTO has to be disabled, otherwise LTO will error "inconsistent LTO Unit splitting (recompile with -fsplit-lto-unit)")
>
>With the above, I can build musl with CFI:
>
>  mkdir -p out/lto
>  cd out/lto
>  ../../configure CC=clang CFLAGS='-g -flto=thin -fsanitize=cfi-icall' LDFLAGS='-fuse-ld=lld'

Minor correction: AR=llvm-ar RANLIB=true is needed, otherwise the
archive index of libc.a will not include LLVM IR symbols - there will be
spurious undefined reference errors.

I wrote a clang-tidy check (attached) to find all indirect function
calls. Here is the list. cfi-icall can protect them. Very few like
__libc_start_main.c:64 may need __attribute__((__no_sanitize__("cfi"))).


../ldso/dlstart.c:147:2: warning: 'stage2_func' (aka 'void (*)(unsigned char *, unsigned long *)') [bugprone-find-icall]
         dls2((void *)base, sp);
         ^
../ldso/dynlink.c:2335:9: warning: 'int (*)(struct dl_phdr_info *, size_t, void *)' (aka 'int (*)(struct dl_phdr_info *, unsigned long, void *)') [bugprone-find-icall]
                 ret = (callback)(&info, sizeof (info), data);
                       ^
../src/aio/aio.c:201:3: warning: 'void (*)(union sigval)' [bugprone-find-icall]
                 sev.sigev_notify_function(sev.sigev_value);
                 ^
../src/aio/lio_listio.c:63:3: warning: 'void (*)(union sigval)' [bugprone-find-icall]
                 sev->sigev_notify_function(sev->sigev_value);
                 ^
../src/dirent/scandir.c:20:15: warning: 'int (*)(const struct dirent *)' [bugprone-find-icall]
                 if (sel && !sel(de)) continue;
                             ^
../src/env/__libc_start_main.c:64:3: warning: 'void (*)(void)' [bugprone-find-icall]
                 (*(void (**)(void))a)();
                 ^
../src/env/__libc_start_main.c:85:9: warning: 'lsm2_fn *' (aka 'int (*)(int (*)(int, char **, char **), int, char **)') [bugprone-find-icall]
         return stage2(main, argc, argv);
                ^
../src/env/__libc_start_main.c:94:7: warning: 'int (*)(int, char **, char **)' [bugprone-find-icall]
         exit(main(argc, argv, envp));
              ^
../src/exit/at_quick_exit.c:20:3: warning: 'void (*)(void)' [bugprone-find-icall]
                 func();
                 ^
../src/exit/atexit.c:34:3: warning: 'void (*)(void *)' [bugprone-find-icall]
                 func(arg);
                 ^
../src/exit/exit.c:21:3: warning: 'void (*)()' [bugprone-find-icall]
                 (*(void (**)())(a-sizeof(void(*)())))();
                 ^
../src/ldso/dl_iterate_phdr.c:43:9: warning: 'int (*)(struct dl_phdr_info *, size_t, void *)' (aka 'int (*)(struct dl_phdr_info *, unsigned long, void *)') [bugprone-find-icall]
         return (callback)(&info, sizeof (info), data);
                ^
../src/misc/nftw.c:75:33: warning: 'int (*)(const char *, const struct stat *, int, struct FTW *)' [bugprone-find-icall]
         if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
                                        ^
../src/misc/nftw.c:115:32: warning: 'int (*)(const char *, const struct stat *, int, struct FTW *)' [bugprone-find-icall]
         if ((flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev)))
                                       ^
../src/mq/mq_notify.c:28:3: warning: 'void (*)(union sigval)' [bugprone-find-icall]
                 func(val);
                 ^
../src/network/dns_parse.c:29:7: warning: 'int (*)(void *, int, const void *, int, const void *)' [bugprone-find-icall]
                 if (callback(ctx, p[1], p+10, len, r) < 0) return -1;
                     ^
../src/network/netlink.c:36:10: warning: 'int (*)(void *, struct nlmsghdr *)' [bugprone-find-icall]
                         ret = cb(ctx, h);
                               ^
../src/process/posix_spawn.c:153:2: warning: 'int (*)(const char *, char *const *, char *const *)' [bugprone-find-icall]
         exec(args->path, args->argv, args->envp);
         ^
../src/regex/glob.c:108:26: warning: 'int (*)(const char *, int)' [bugprone-find-icall]
                         if (errno!=ENOENT && (errfunc(buf, errno) || (flags & GLOB_ERR)))
                                               ^
../src/regex/glob.c:129:7: warning: 'int (*)(const char *, int)' [bugprone-find-icall]
                 if (errfunc(buf, errno) || (flags & GLOB_ERR))
                     ^
../src/regex/glob.c:169:18: warning: 'int (*)(const char *, int)' [bugprone-find-icall]
         if (readerr && (errfunc(buf, errno) || (flags & GLOB_ERR)))
                         ^
../src/sched/sched_getcpu.c:18:13: warning: 'getcpu_f' (aka 'long (*)(unsigned int *, unsigned int *, void *)') [bugprone-find-icall]
         return f ? f(cpu, node, unused) : -ENOSYS;
                    ^
../src/sched/sched_getcpu.c:33:7: warning: 'getcpu_f' (aka 'long (*)(unsigned int *, unsigned int *, void *)') [bugprone-find-icall]
                 r = f(&cpu, 0, 0);
                     ^
../src/search/lsearch.c:12:7: warning: 'int (*)(const void *, const void *)' [bugprone-find-icall]
                 if (compar(key, p[i]) == 0)
                     ^
../src/search/lsearch.c:26:7: warning: 'int (*)(const void *, const void *)' [bugprone-find-icall]
                 if (compar(key, p[i]) == 0)
                     ^
../src/search/tdelete.c:23:11: warning: 'int (*)(const void *, const void *)' [bugprone-find-icall]
                 int c = cmp(key, n->key);
                         ^
../src/search/tdestroy.c:14:15: warning: 'void (*)(void *)' [bugprone-find-icall]
         if (freekey) freekey((void *)r->key);
                      ^
../src/search/tfind.c:14:11: warning: 'int (*)(const void *, const void *)' [bugprone-find-icall]
                 int c = cmp(key, n->key);
                         ^
../src/search/tsearch.c:76:11: warning: 'int (*)(const void *, const void *)' [bugprone-find-icall]
                 int c = cmp(key, n->key);
                         ^
../src/search/twalk.c:9:3: warning: 'void (*)(const void *, VISIT, int)' [bugprone-find-icall]
                 action(r, leaf, d);
                 ^
../src/search/twalk.c:11:3: warning: 'void (*)(const void *, VISIT, int)' [bugprone-find-icall]
                 action(r, preorder, d);
                 ^
../src/search/twalk.c:13:3: warning: 'void (*)(const void *, VISIT, int)' [bugprone-find-icall]
                 action(r, postorder, d);
                 ^
../src/search/twalk.c:15:3: warning: 'void (*)(const void *, VISIT, int)' [bugprone-find-icall]
                 action(r, endorder, d);
                 ^
../src/stdio/__fclose_ca.c:5:9: warning: 'int (*)(FILE *)' (aka 'int (*)(struct _IO_FILE *)') [bugprone-find-icall]
         return f->close(f);
                ^
../src/stdio/__overflow.c:8:6: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
         if (f->write(f, &c, 1)!=1) return EOF;
             ^
../src/stdio/__stdio_exit.c:12:27: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
         if (f->wpos != f->wbase) f->write(f, 0, 0);
                                  ^
../src/stdio/__stdio_exit.c:13:26: warning: 'off_t (*)(FILE *, off_t, int)' (aka 'long (*)(struct _IO_FILE *, long, int)') [bugprone-find-icall]
         if (f->rpos != f->rend) f->seek(f, f->rpos-f->rend, SEEK_CUR);
                                 ^
../src/stdio/__toread.c:6:27: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
         if (f->wpos != f->wbase) f->write(f, 0, 0);
                                  ^
../src/stdio/__uflow.c:9:22: warning: 'size_t (*)(FILE *, unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, unsigned char *, unsigned long)') [bugprone-find-icall]
         if (!__toread(f) && f->read(f, &c, 1)==1) return c;
                             ^
../src/stdio/fclose.c:13:7: warning: 'int (*)(FILE *)' (aka 'int (*)(struct _IO_FILE *)') [bugprone-find-icall]
         r |= f->close(f);
              ^
../src/stdio/fflush.c:29:3: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
                 f->write(f, 0, 0);
                 ^
../src/stdio/fflush.c:37:26: warning: 'off_t (*)(FILE *, off_t, int)' (aka 'long (*)(struct _IO_FILE *, long, int)') [bugprone-find-icall]
         if (f->rpos != f->rend) f->seek(f, f->rpos-f->rend, SEEK_CUR);
                                 ^
../src/stdio/fopencookie.c:30:9: warning: 'cookie_read_function_t *' (aka 'long (*)(void *, char *, unsigned long)') [bugprone-find-icall]
                 ret = fc->iofuncs.read(fc->cookie, (char *) buf, len2);
                       ^
../src/stdio/fopencookie.c:40:8: warning: 'cookie_read_function_t *' (aka 'long (*)(void *, char *, unsigned long)') [bugprone-find-icall]
         ret = fc->iofuncs.read(fc->cookie, (char *) f->rpos, f->buf_size);
               ^
../src/stdio/fopencookie.c:64:8: warning: 'cookie_write_function_t *' (aka 'long (*)(void *, const char *, unsigned long)') [bugprone-find-icall]
         ret = fc->iofuncs.write(fc->cookie, (const char *) buf, len);
               ^
../src/stdio/fopencookie.c:85:8: warning: 'cookie_seek_function_t *' (aka 'int (*)(void *, long *, int)') [bugprone-find-icall]
         res = fc->iofuncs.seek(fc->cookie, &off, whence);
               ^
../src/stdio/fopencookie.c:94:32: warning: 'cookie_close_function_t *' (aka 'int (*)(void *)') [bugprone-find-icall]
         if (fc->iofuncs.close) return fc->iofuncs.close(fc->cookie);
                                       ^
../src/stdio/fread.c:27:25: warning: 'size_t (*)(FILE *, unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, unsigned char *, unsigned long)') [bugprone-find-icall]
                 k = __toread(f) ? 0 : f->read(f, dest, l);
                                       ^
../src/stdio/fseek.c:10:3: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
                 f->write(f, 0, 0);
                 ^
../src/stdio/fseek.c:18:6: warning: 'off_t (*)(FILE *, off_t, int)' (aka 'long (*)(struct _IO_FILE *, long, int)') [bugprone-find-icall]
         if (f->seek(f, off, whence) < 0) return -1;
             ^
../src/stdio/ftell.c:7:14: warning: 'off_t (*)(FILE *, off_t, int)' (aka 'long (*)(struct _IO_FILE *, long, int)') [bugprone-find-icall]
         off_t pos = f->seek(f, 0,
                     ^
../src/stdio/fwrite.c:10:36: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
         if (l > f->wend - f->wpos) return f->write(f, s, l);
                                           ^
../src/stdio/fwrite.c:16:15: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
                         size_t n = f->write(f, s, i);
                                    ^
../src/stdio/vfprintf.c:685:3: warning: 'size_t (*)(FILE *, const unsigned char *, size_t)' (aka 'unsigned long (*)(struct _IO_FILE *, const unsigned char *, unsigned long)') [bugprone-find-icall]
                 f->write(f, 0, 0);
                 ^
../src/stdlib/bsearch.c:9:10: warning: 'int (*)(const void *, const void *)' [bugprone-find-icall]
                 sign = cmp(key, try);
                        ^
../src/thread/pthread_atfork.c:21:20: warning: 'void (*)(void)' [bugprone-find-icall]
                         if (p->prepare) p->prepare();
                                         ^
../src/thread/pthread_atfork.c:26:27: warning: 'void (*)(void)' [bugprone-find-icall]
                         if (!who && p->parent) p->parent();
                                                ^
../src/thread/pthread_atfork.c:27:30: warning: 'void (*)(void)' [bugprone-find-icall]
                         else if (who && p->child) p->child();
                                                   ^
../src/thread/pthread_cleanup_push.c:19:11: warning: 'void (*)(void *)' [bugprone-find-icall]
         if (run) cb->__f(cb->__x);
                  ^
../src/thread/pthread_create.c:67:3: warning: 'void (*)(void *)' [bugprone-find-icall]
                 f(x);
                 ^
../src/thread/pthread_create.c:203:17: warning: 'void *(*)(void *)' [bugprone-find-icall]
         __pthread_exit(args->start_func(args->start_arg));
                        ^
../src/thread/pthread_create.c:211:36: warning: 'int (*)(void *)' [bugprone-find-icall]
         __pthread_exit((void *)(uintptr_t)start(args->start_arg));
                                           ^
../src/thread/pthread_key_create.c:82:5: warning: 'void (*)(void *)' [bugprone-find-icall]
                                 dtor(val);
                                 ^
../src/thread/pthread_once.c:22:3: warning: 'void (*)(void)' [bugprone-find-icall]
                 init();
                 ^
../src/thread/synccall.c:31:2: warning: 'void (*)(void *)' [bugprone-find-icall]
         callback(context);
         ^
../src/thread/synccall.c:105:2: warning: 'void (*)(void *)' [bugprone-find-icall]
         func(ctx);
         ^
../src/time/clock_gettime.c:49:13: warning: 'int (*)(clockid_t, struct timespec *)' (aka 'int (*)(int, struct timespec *)') [bugprone-find-icall]
         return f ? f(clk, ts) : -ENOSYS;
                    ^
../src/time/clock_gettime.c:64:7: warning: 'int (*)(clockid_t, struct timespec *)' (aka 'int (*)(int, struct timespec *)') [bugprone-find-icall]
                 r = f(clk, ts);
                     ^
../src/time/timer_create.c:51:4: warning: 'void (*)(union sigval)' [bugprone-find-icall]
                         notify(val);
                         ^


[-- Attachment #2: clang-tidy-bugprone-find-icall.patch --]
[-- Type: text/x-diff, Size: 5258 bytes --]

From 6b68dbf2e79df25f286bcd11805ae52093ba2b15 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i@maskray.me>
Date: Sun, 27 Dec 2020 16:54:29 -0800
Subject: [PATCH] WIP. find icall

---
 .../bugprone/BugproneTidyModule.cpp           |  2 +
 .../clang-tidy/bugprone/CMakeLists.txt        |  1 +
 .../clang-tidy/bugprone/FindICallCheck.cpp    | 43 +++++++++++++++++++
 .../clang-tidy/bugprone/FindICallCheck.h      | 30 +++++++++++++
 4 files changed, 76 insertions(+)
 create mode 100644 clang-tools-extra/clang-tidy/bugprone/FindICallCheck.cpp
 create mode 100644 clang-tools-extra/clang-tidy/bugprone/FindICallCheck.h

diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 7f4d40f9701..c9a8f250766 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "DanglingHandleCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "ExceptionEscapeCheck.h"
+#include "FindICallCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
@@ -161,6 +162,7 @@ public:
         "bugprone-suspicious-semicolon");
     CheckFactories.registerCheck<SuspiciousStringCompareCheck>(
         "bugprone-suspicious-string-compare");
+    CheckFactories.registerCheck<FindICallCheck>("bugprone-find-icall");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "bugprone-swapped-arguments");
     CheckFactories.registerCheck<TerminatingContinueCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index b3684a5c101..2e52673069f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -49,6 +49,7 @@ add_clang_library(clangTidyBugproneModule
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
+  FindICallCheck.cpp
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/FindICallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/FindICallCheck.cpp
new file mode 100644
index 00000000000..81a3e87e0f6
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/FindICallCheck.cpp
@@ -0,0 +1,43 @@
+//===--- FindICallCheck.cpp - clang-tidy ---------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "FindICallCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/FixIt.h"
+#include "llvm/ADT/SmallPtrSet.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void FindICallCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr().bind("call"), this);
+}
+
+static const Expr *ignoreNoOpCasts(const Expr *E) {
+  if (auto *Cast = dyn_cast<CastExpr>(E))
+    if (Cast->getCastKind() != CK_LValueToRValue &&
+        Cast->getCastKind() != CK_FunctionToPointerDecay)
+      return ignoreNoOpCasts(Cast->getSubExpr());
+  return E;
+}
+
+void FindICallCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+  if (auto *C = ignoreNoOpCasts(Call->getCallee()))
+    if (auto *Cast = dyn_cast<CastExpr>(C))
+      if (Cast->getCastKind() == CK_LValueToRValue)
+        diag(Call->getBeginLoc(), "%0") << C->getType();
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/bugprone/FindICallCheck.h b/clang-tools-extra/clang-tidy/bugprone/FindICallCheck.h
new file mode 100644
index 00000000000..8becedee82f
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/FindICallCheck.h
@@ -0,0 +1,30 @@
+//===--- SwappedArgumentsCheck.h - clang-tidy -------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FINDICALLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FINDICALLCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+class FindICallCheck : public ClangTidyCheck {
+public:
+  FindICallCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FINDICALLCHECK_H
-- 
2.29.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH] Add support for LLVM's Control Flow Integrity
  2020-12-28  0:56   ` Fangrui Song
@ 2020-12-28  9:20     ` Charlotte Delenk
  2020-12-28 13:17       ` [musl] [PATCH] Add support for LLVM's Control Flow Integrity (V2) Charlotte Delenk
  0 siblings, 1 reply; 10+ messages in thread
From: Charlotte Delenk @ 2020-12-28  9:20 UTC (permalink / raw)
  To: musl

Hi,

On 12/28/20 1:56 AM, Fangrui Song wrote:
>> tl;dr This is insufficient.
Yes, I'm aware that this only covers the few things that I have tested.
>> For libc, we can ignore all the other CFI checks.
>>
>> ldso/ has several indirect function calls which are incompatible with 
>> -fsanitize=cfi-icall
Use of CFI together with dynamic loading wasn't one of my priorities but 
it certainly might be for others
>>
>> ldso/dlstart.c
>> In _dlstart_c, dls2((void *)base, sp); The address is obtained via 
>> GETFUNCSYM (inline asm) - Clang does not know the original function 
>> type and thus the llvm.type.test check will fail.
I'm going to disable cfi for that one as well.
>>
>> (Another problem: Clang tracks undefined symbols in module-level 
>> inline asm but does not know undefined symbols in function-scope 
>> inline asm.
I think you can also add an __attribute__((used)) to the declaration; I 
have successfully used that hack in other low-level software that calls 
otherwise unused functions from inline asm.
> I wrote a clang-tidy check (attached) to find all indirect function
> calls. Here is the list. cfi-icall can protect them. Very few like
> __libc_start_main.c:64 may need __attribute__((__no_sanitize__("cfi"))).
Thank you for writing this! I'll look through the list and see if I spot 
other issues.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [musl] [PATCH] Add support for LLVM's Control Flow Integrity (V2)
  2020-12-28  9:20     ` Charlotte Delenk
@ 2020-12-28 13:17       ` Charlotte Delenk
  2020-12-28 17:01         ` Shiz
  0 siblings, 1 reply; 10+ messages in thread
From: Charlotte Delenk @ 2020-12-28 13:17 UTC (permalink / raw)
  To: musl

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(-)

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)
  {
      uintptr_t a = (uintptr_t)&__fini_array_end;
-- 
2.29.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH] Add support for LLVM's Control Flow Integrity (V2)
  2020-12-28 13:17       ` [musl] [PATCH] Add support for LLVM's Control Flow Integrity (V2) Charlotte Delenk
@ 2020-12-28 17:01         ` Shiz
  2020-12-29  1:26           ` Rich Felker
  2020-12-29 10:20           ` Charlotte Delenk
  0 siblings, 2 replies; 10+ messages in thread
From: Shiz @ 2020-12-28 17:01 UTC (permalink / raw)
  To: musl; +Cc: darkkirb

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH] Add support for LLVM's Control Flow Integrity (V2)
  2020-12-28 17:01         ` Shiz
@ 2020-12-29  1:26           ` Rich Felker
  2020-12-29 10:20           ` Charlotte Delenk
  1 sibling, 0 replies; 10+ messages in thread
From: Rich Felker @ 2020-12-29  1:26 UTC (permalink / raw)
  To: Shiz; +Cc: musl, darkkirb

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [musl] [PATCH] Add support for LLVM's Control Flow Integrity (V2)
  2020-12-28 17:01         ` Shiz
  2020-12-29  1:26           ` Rich Felker
@ 2020-12-29 10:20           ` Charlotte Delenk
  2020-12-29 11:56             ` [musl] [PATCH 1/2] Fix LTO shared library build on GCC and Clang Charlotte Delenk
  2020-12-29 11:59             ` [musl] [PATCH 2/2] Add support for LLVM's Control Flow Integrity Charlotte Delenk
  1 sibling, 2 replies; 10+ messages in thread
From: Charlotte Delenk @ 2020-12-29 10:20 UTC (permalink / raw)
  To: musl

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
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [musl] [PATCH 1/2] Fix LTO shared library build on GCC and Clang
  2020-12-29 10:20           ` Charlotte Delenk
@ 2020-12-29 11:56             ` Charlotte Delenk
  2020-12-29 11:59             ` [musl] [PATCH 2/2] Add support for LLVM's Control Flow Integrity Charlotte Delenk
  1 sibling, 0 replies; 10+ messages in thread
From: Charlotte Delenk @ 2020-12-29 11:56 UTC (permalink / raw)
  To: musl

Both GCC and Clang are currently unable to compile the musl dynamic
library with link time optimizations enabled, due to them overzealously
removing certain symbols used on program start which they deem are not
necessary.

This patch adds new link flags to the libc.so link that tell the linker
that a symbol is used externally, so that code for these functions is
always emitted.
---
  Makefile | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/Makefile b/Makefile
index e8cc4436..15190fb9 100644
--- a/Makefile
+++ b/Makefile
@@ -131,6 +131,9 @@ $(CRT_OBJS): CFLAGS_ALL += -DCRT

  $(LOBJS) $(LDSO_OBJS): CFLAGS_ALL += -fPIC

+# Work around LTO compiler bugs
+lib/libc.so: CFLAGS_ALL += -u_dlstart_c -u__dls2 -u__dls2b -u__dls3 
-u__stack_chk_guard -u_start_c
+
  CC_CMD = $(CC) $(CFLAGS_ALL) -c -o $@ $<

  # Choose invocation of assembler to be used
-- 
2.29.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [musl] [PATCH 2/2] Add support for LLVM's Control Flow Integrity
  2020-12-29 10:20           ` Charlotte Delenk
  2020-12-29 11:56             ` [musl] [PATCH 1/2] Fix LTO shared library build on GCC and Clang Charlotte Delenk
@ 2020-12-29 11:59             ` Charlotte Delenk
  1 sibling, 0 replies; 10+ messages in thread
From: Charlotte Delenk @ 2020-12-29 11:59 UTC (permalink / raw)
  To: musl

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 works by disabling CFI sanitization for these files:

ldso/dlstart.c
ldso/dynlink.c
src/env/__libc_start_main.c
src/exit/exit.c

These contain indirect function calls where the compiler is either
unable to find out the type of the function or where the actual function
type can be one of multiple equally valid ones.

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>

This patch depends on the previous patch labelled "Fix LTO shared 
library build on GCC and Clang"

---
  Makefile | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index 15190fb9..9d937b21 100644
--- a/Makefile
+++ b/Makefile
@@ -134,6 +134,14 @@ $(LOBJS) $(LDSO_OBJS): CFLAGS_ALL += -fPIC
  # Work around LTO compiler bugs
  lib/libc.so: CFLAGS_ALL += -u_dlstart_c -u__dls2 -u__dls2b -u__dls3 
-u__stack_chk_guard -u_start_c

+# Disable CFI for problematic source files
+ifneq (,$(findstring cfi,$(filter -fsanitize=%,$(CFLAGS))))
+obj/ldso/dlstart.lo: CFLAGS_ALL += -fno-sanitize=cfi
+obj/ldso/dynlink.lo: CFLAGS_ALL += -fno-sanitize=cfi
+obj/src/env/__libc_start_main.lo: CFLAGS_ALL += -fno-sanitize=cfi
+obj/src/exit/exit.lo: CFLAGS_ALL += -fno-sanitize=cfi
+endif
+
  CC_CMD = $(CC) $(CFLAGS_ALL) -c -o $@ $<

  # Choose invocation of assembler to be used
-- 
2.29.2



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-29 11:59 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-27 17:53 [musl] [PATCH] Add support for LLVM's Control Flow Integrity Charlotte Delenk
2020-12-27 23:05 ` Fangrui Song
2020-12-28  0:56   ` Fangrui Song
2020-12-28  9:20     ` Charlotte Delenk
2020-12-28 13:17       ` [musl] [PATCH] Add support for LLVM's Control Flow Integrity (V2) Charlotte Delenk
2020-12-28 17:01         ` Shiz
2020-12-29  1:26           ` Rich Felker
2020-12-29 10:20           ` Charlotte Delenk
2020-12-29 11:56             ` [musl] [PATCH 1/2] Fix LTO shared library build on GCC and Clang Charlotte Delenk
2020-12-29 11:59             ` [musl] [PATCH 2/2] Add support for LLVM's Control Flow Integrity Charlotte Delenk

mailing list of musl libc

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.vuxu.org/musl

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 musl musl/ http://inbox.vuxu.org/musl \
		musl@inbox.vuxu.org
	public-inbox-index musl

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.musl


code repositories for the project(s) associated with this inbox:

	https://git.vuxu.org/mirror/musl/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git