Details

Reviewers

emaste
dim
brooks
kib
jrtc27
markj

Commits

rG7bc797e3f380: Add build system support for ASAN+UBSAN instrumentation

Summary

This adds two new options WITH_ASAN/WITH_UBSAN that can be set to
enable instrumentation of all binaries with AddressSanitizer and/or
UndefinedBehaviourSanitizer. This current patch is almost sufficient
to get a complete buildworld with sanitizer instrumentation but in
order to actually build and boot a system it depends on a few more
follow-up commits.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

arichardson created this revision.Jul 5 2021, 10:45 AM

Herald added subscribers: dab, bdragon, imp, bdrewery. · View Herald TranscriptJul 5 2021, 10:45 AM

arichardson requested review of this revision.Jul 5 2021, 10:45 AM

Harbormaster completed remote builds in B40289: Diff 91772.Jul 5 2021, 10:45 AM

Add one missing pattern

Harbormaster completed remote builds in B40291: Diff 91774.Jul 5 2021, 10:51 AM

arichardson added a child revision: D31044: Fix building rescue/rescue when sanitizers are enabled.Jul 5 2021, 10:51 AM

arichardson added a child revision: D31045: libc: Disable ASAN for certain string functions.

arichardson added a child revision: D31046: Don't instrument the rdtsc ifunc when building with ASAN/UBSAN.

arichardson added a child revision: D31047: Fix build of stand/ when building world with ASAN.Jul 5 2021, 11:33 AM

kib added inline comments.Jul 5 2021, 11:54 AM

libexec/rtld-elf/Makefile
17	I do not think rtld would be ever compatible.
share/mk/bsd.lib.mk
9	What does this mean, and what it does?
154	What does this mean? Are you hacking on the SHARED_CFLAGS value, removing -fstack-protector when sanitizers are enabled? Can it be done clean, for instance by not adding -fstack-protector at all to the variable? If my guess is right, such fight over the options should not be done.

arichardson added inline comments.Jul 5 2021, 11:58 AM

libexec/rtld-elf/Makefile
17	I was thinking that minimal UBSAN might work, but yeah will update the comment.
share/mk/bsd.lib.mk
9	This just creates an empty target so that bsd.sanitizer.mk can check whether it was included from bsd.lib.mk or bsd.prog.mk. Other makefiles use this pattern as an "include guard".
154	This is here to ensure that the .nosspico files don't have sanitizer instrumentation since they are used by RTLD. I reused the existing logic to remove -fstack-protector here, not sure if there is a cleaner solution.

arichardson added a child revision: D31009: usr.bin/sort: Avoid UBSan errors.Jul 5 2021, 12:17 PM

arichardson added a child revision: D31048: bin/ps: Avoid function name conflict with libc uname().Jul 5 2021, 12:27 PM

fix rtld comment

Harbormaster completed remote builds in B40300: Diff 91787.Jul 5 2021, 12:28 PM

arichardson added a child revision: D31049: Fix MK_TESTS build with MK_ASAN/MK_UBSAN.Jul 5 2021, 12:29 PM

arichardson added a child revision: D31050: Allow building usr.bin/vi with MK_ASAN.

arichardson added a reviewer: markj.Jul 5 2021, 1:02 PM

arichardson mentioned this in D31047: Fix build of stand/ when building world with ASAN.Jul 5 2021, 1:06 PM

emaste added inline comments.Jul 5 2021, 1:07 PM

lib/csu/Makefile.inc
9–10	This is a related but independent change?

arichardson added inline comments.Jul 5 2021, 1:08 PM

lib/csu/Makefile.inc
9–10	I originally had this as a separate commit but looks like I squashed it while rebasing. Will factor it out. I don't think it really matters since none of the functions should end up with SSP checks but in theory it could be a problem.

markj added inline comments.Jul 5 2021, 1:38 PM

share/mk/bsd.lib.mk
154	But we explicitly disable instrumentation for rtld, so this change is just for safety? I believe rtld is the only place where we generate this flavour of object files.
share/mk/bsd.sanitizer.mk
12	What does the linker do with -fsanitize? In my experiments this wasn't needed.

arichardson added inline comments.Jul 5 2021, 1:41 PM

share/mk/bsd.sanitizer.mk
12	This ensures that clang adds the require libraries to the linker command line: the ASAN runtime needs to intercept many libc functions (even when libc is built with ASAN instrumentation). And this runtime is linked as a static library if you pass -fsanitize=address.

arichardson added inline comments.Jul 5 2021, 1:42 PM

share/mk/bsd.lib.mk
154	RTLD pulls in some of these files from libc (and libc is built with ASAN instrumentation), so unless we remove -fsanitize=address from build command for the .nosspico files, rtld will end up using some object files that call `__asan_foo` functions.

markj added inline comments.Jul 5 2021, 4:13 PM

lib/libclang_rt/Makefile.inc
16	Is it necessary to use different assignment operators here for MK_PROFILE vs. MK_(A\|UB)SAN?
share/mk/bsd.lib.mk
154	Ah, I didn't realize that the rtld-libc build reuses libc object files. You might consider amending the description of .nossppico files in share/mk/bsd.README then.
share/mk/bsd.sanitizer.mk
12	I see, I was relying on the fact that we pass CFLAGS when linking. And I see now that -fsanitize* gets stripped from _LDFLAGS.

Fix running kyua by not instrumenting libunwind functions called during C++ exception unwinding. This results in false-positive ASAN reports.

Harbormaster completed remote builds in B40315: Diff 91813.Jul 5 2021, 5:42 PM

arichardson added a child revision: D31061: Enable MK_LLVM_BINUTILS if MK_ASAN is requested.Jul 5 2021, 5:47 PM

arichardson added a child revision: D31074: libthr: work around an ASAN false-positive.Jul 6 2021, 8:37 AM

arichardson marked 6 inline comments as done.Jul 6 2021, 9:47 AM

arichardson added inline comments.

lib/csu/Makefile.inc
9–10	Just noticed there is already a `SSP_CFLAGS=` above which should no longer be needed with MK_SSP=no.
lib/libclang_rt/Makefile.inc
16	Most other places use `:=` for MK_* assignments, I guess both work fine here.
share/mk/bsd.sanitizer.mk
12	It does seem like most (all?) linker invocation will get the CFLAGS but I feel that adding it to LDFLAGS as well is safer.

Drop unncessary MK_SSP=no

Harbormaster completed remote builds in B40349: Diff 91874.Jul 6 2021, 11:06 AM

Thanks, this looks reasonable to me. The change is missing src.conf.5 updates for the new knobs.

This revision is now accepted and ready to land.Jul 6 2021, 2:20 PM

kib accepted this revision.Jul 6 2021, 6:10 PM

emaste added inline comments.Jul 6 2021, 6:33 PM

Makefile.inc1
721–722	Do we have any documentation for `BOOTSTRAP_TOOLS_ASAN`/`BOOTSTRAP_TOOLS_UBSAN`?
lib/libgcc_eh/Makefile.inc
24	Is a bug, or a known limitation of asan?

brooks accepted this revision.Jul 6 2021, 7:42 PM

arichardson added inline comments.Jul 6 2021, 8:35 PM

lib/libgcc_eh/Makefile.inc
24	I haven't had time to debug this, my naive assumption is that asan expects functions that do stack unwinding to be uninstrumented.

jrtc27 added inline comments.Jul 6 2021, 8:55 PM

lib/libgcc_eh/Makefile.inc
24	https://github.com/llvm/llvm-project/commit/adf1561d6ce8af057127c65af863b3f0e1c77e60 seems relevant, don't know if there are other issues though, but it does look like upstream intends libunwind to work with asan
share/mk/bsd.sanitizer.mk
5	Tabs after your ='s
5	Style is probably to use yes/no not 1/0?
stand/Makefile.inc
4–5 ↗	(On Diff #91874)	Missing a tab after the =

jrtc27 added inline comments.Jul 6 2021, 8:58 PM

share/mk/bsd.sanitizer.mk
7
16	Ditto

arichardson added inline comments.Jul 6 2021, 10:34 PM

lib/libgcc_eh/Makefile.inc
24	Thanks, will try applying that tomorrow.

Address @jrtc27 comments

This revision now requires review to proceed.Jul 19 2021, 12:00 PM

Harbormaster completed remote builds in B40524: Diff 92417.Jul 19 2021, 12:00 PM

arichardson added inline comments.Jul 19 2021, 12:00 PM

Makefile.inc1
721–722	No, I added this as an experimental flag to try and add asan to the bootstrap tools. It won't work yet since there are still quite a few UBSan warnings. Happy to drop it from this commit and just hardcode it to `no`?

arichardson added inline comments.Jul 19 2021, 12:45 PM

lib/libgcc_eh/Makefile.inc

I tried applying that patch but it seems it's not sufficient to allow instrumenting libunwind (at least the version currently in contrib/).
I still get the following error with that patch applied:

root@freebsd-amd64:~ # kyua --help
=================================================================
==808==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7fffffffb500 at pc 0x0000019ae27c bp 0x7fffffffb2c0 sp 0x7fffffffaa80
WRITE of size 552 at 0x7fffffffb500 thread T0
    #0 0x19ae27b in memset /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:782:3
    #1 0x8029930a5 in libunwind::CFI_Parser<libunwind::LocalAddressSpace>::PrologInfo::PrologInfo(libunwind::CFI_Parser<libunwind::LocalAddressSpace>::PrologInfo::InitializeTime) /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/libunwind/src/DwarfParser.hpp:96:9
    #2 0x8029930a5 in libunwind::UnwindCursor<libunwind::LocalAddressSpace, libunwind::Registers_x86_64>::getInfoFromFdeCie(libunwind::CFI_Parser<libunwind::LocalAddressSpace>::FDE_Info const&, libunwind::CFI_Parser<libunwind::LocalAddressSpace>::CIE_Info const&, unsigned long, unsigned long) /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/libunwind/src/UnwindCursor.hpp:1510:38
    #3 0x8029930a5 in libunwind::UnwindCursor<libunwind::LocalAddressSpace, libunwind::Registers_x86_64>::getInfoFromDwarfSection(unsigned long, libunwind::UnwindInfoSections const&, unsigned int) /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/libunwind/src/UnwindCursor.hpp:1571:9
    #4 0x80298ff4f in libunwind::UnwindCursor<libunwind::LocalAddressSpace, libunwind::Registers_x86_64>::setInfoBasedOnIPRegister(bool) /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/libunwind/src/UnwindCursor.hpp:1958:17
    #5 0x80298ea39 in unw_init_local /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/libunwind/src/libunwind.cpp:79:7
    #6 0x80298cc5c in unwind_phase2 /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/libunwind/src/UnwindLevel1.c:133:3
    #7 0x80298d5ba in _Unwind_Resume /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/libunwind/src/UnwindLevel1.c:394:5
    #8 0x1a7ae39 in utils::cmdline::parse(int, char const* const*, std::__1::vector<utils::cmdline::base_option const*, std::__1::allocator<utils::cmdline::base_option const*> > const&) /local/scratch/alr48/cheri/freebsd/contrib/kyua/utils/cmdline/parser.cpp
    #9 0x1cefab6 in (anonymous namespace)::safe_main(utils::cmdline::ui*, int, char const* const*, std::__1::auto_ptr<utils::cmdline::base_command<utils::config::tree> >) /local/scratch/alr48/cheri/freebsd/contrib/kyua/cli/main.cpp:203:45
    #10 0x1ced089 in cli::main(utils::cmdline::ui*, int, char const* const*, std::__1::auto_ptr<utils::cmdline::base_command<utils::config::tree> >) /local/scratch/alr48/cheri/freebsd/contrib/kyua/cli/main.cpp:280:31
    #11 0x1cf1670 in cli::main(int, char const* const*) /local/scratch/alr48/cheri/freebsd/contrib/kyua/cli/main.cpp:353:27

Address 0x7fffffffb500 is located in stack of thread T0 at offset 0 in frame
    #0 0x27  (<unknown module>)

AddressSanitizer can't parse the stack frame descriptor: ||
SUMMARY: AddressSanitizer: stack-buffer-underflow /local/scratch/alr48/cheri/freebsd/contrib/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:782:3 in memset
Shadow bytes around the buggy address:
  0x4ffffffff650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4ffffffff660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4ffffffff670: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4ffffffff680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4ffffffff690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x4ffffffff6a0:[f1]f1 f1 f1 00 f2 f2 f2 00 00 00 00 00 00 00 00
  0x4ffffffff6b0: 00 f2 f2 f2 f2 f2 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x4ffffffff6c0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x4ffffffff6d0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x4ffffffff6e0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x4ffffffff6f0: f8 f8 f8 f8 f8 f8 f2 f2 f2 f2 f2 f2 f2 f2 f8 f2
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==808==ABORTING

lwhsu added a subscriber: lwhsu.Jul 19 2021, 2:24 PM

jrtc27 added inline comments.Jul 20 2021, 7:20 PM

Makefile.inc1
721–722	Yeah I'd leave them out of this
lib/csu/Makefile.inc
7–8	Tabs
lib/libgcc_s/Makefile
8	Tab
share/mk/bsd.sanitizer.mk
39	Tab

Style fixes + drop BOOTSTRAP_TOOLS_ASAN/BOOTSTRAP_TOOLS_UBSAN

Will commit this tomorrow after a final round of testing.

Harbormaster completed remote builds in B40587: Diff 92533.Jul 20 2021, 10:41 PM

This revision was not accepted when it landed; it landed in state Needs Review.Aug 2 2021, 1:35 PM

Closed by commit rG7bc797e3f380: Add build system support for ASAN+UBSAN instrumentation (authored by arichardson). · Explain Why

This revision was automatically updated to reflect the committed changes.

arichardson added a commit: rG7bc797e3f380: Add build system support for ASAN+UBSAN instrumentation.

Add build system support for ASAN+UBSAN instrumentation
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 93112

Makefile.inc1

Makefile.libcompat

lib/Makefile

lib/csu/Makefile.inc

lib/libclang_rt/Makefile.inc

lib/libgcc_eh/Makefile.inc

lib/libgcc_s/Makefile

libexec/rtld-elf/Makefile

share/mk/bsd.README

share/mk/bsd.compat.mk

share/mk/bsd.lib.mk

share/mk/bsd.opts.mk

share/mk/bsd.prog.mk

share/mk/bsd.progs.mk

share/mk/bsd.sanitizer.mk

share/mk/sys.mk

Add build system support for ASAN+UBSAN instrumentationClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 93112

Makefile.inc1

Makefile.libcompat

lib/Makefile

lib/csu/Makefile.inc

lib/libclang_rt/Makefile.inc

lib/libgcc_eh/Makefile.inc

lib/libgcc_s/Makefile

libexec/rtld-elf/Makefile

share/mk/bsd.README

share/mk/bsd.compat.mk

share/mk/bsd.lib.mk

share/mk/bsd.opts.mk

share/mk/bsd.prog.mk

share/mk/bsd.progs.mk

share/mk/bsd.sanitizer.mk

share/mk/sys.mk

Add build system support for ASAN+UBSAN instrumentation
ClosedPublic
Actions

Revision Contents
Changeset List