This is an archive of the discontinued LLVM Phabricator instance.

[ASan] Initial support for Kernel AddressSanitizer
ClosedPublic

Authored by glider on Jun 12 2015, 5:05 AM.

Details

Reviewers
dvyukov
samsonov
Summary

This patch adds initial support for the -fsanitize=kernel-address flag to Clang.
Right now it's quite restricted: only out-of-line instrumentation is supported, globals are not instrumented, some GCC kasan flags are not supported.
Using this patch I am able to build and boot the KASan tree with LLVMLinux patches from github.com/ramosian-glider/kasan/tree/kasan_llvmlinux.
To disable KASan instrumentation for a certain function attribute((no_sanitize("kernel-address"))) can be used.

Diff Detail

Event Timeline

glider updated this revision to Diff 27571.Jun 12 2015, 5:05 AM
glider retitled this revision from to [ASan] Initial support for Kernel AddressSanitizer.
glider updated this object.
glider edited the test plan for this revision. (Show Details)
glider added reviewers: samsonov, dvyukov.
glider added subscribers: kcc, Unknown Object (MLST), Unknown Object (MLST).
glider updated this revision to Diff 27577.Jun 12 2015, 8:08 AM

Added some tests

samsonov edited edge metadata.Jun 12 2015, 11:04 AM

Cool, happy to see it coming. I would also add tests for:

a) presence of sanitize_address function attribute
b) actual KASan instrumentation with correct shadow offset/scale.
c) missing module ctors/callbacks in KASan mode.
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1362

Wait a second.... Do you do *anything* in AddressSanitizerModule pass in CompileKernel mode? You don't add module constructor, and you don't instrument globals. Maybe, we should just avoid creating this pass in the first place?

1412

Hm... So you don't have __asan_memset() in the kernel. Maybe, we shouldn't instrument mem intrinsics in this case at all, not replace intrinisics with call to plain memset()?

1448

You will access uninitialized memory in AddressSanitizer::runOnFunction (potentially in AddressSanitizer::maybeInsertAsanInitAtFunctionEntry as well, but the latter is relevant only for Mac).

1535–1536
bool UseCalls = CompileKernel || (ClInstrumentationWithCallsThreshold >= 0 && ...);
tools/clang/include/clang/Basic/Sanitizers.def
44

What is the "correct" way to capitalize it? KASAN? KASan?

tools/clang/lib/AST/Decl.cpp
3684–3685

Wow, that's hard to parse. I'd vote for adding a method to SanitizerSet

if (!Context.getLangOpts().Sanitize.hasOneOf(SanitizerKind::Address, SanitizerKind::KernelAddress)
  ...

That would also be useful in several places below.

tools/clang/lib/CodeGen/BackendUtil.cpp
217

See note above - I'm not sure we need a module pass for that.

ygribov added inline comments.
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1362

Or perhaps instrumentation of globals will follow shortly so it's ok to have this pass at hand.

1383

Perhaps use "_noabort" suffix to match gcc?

Addressed Alexey's comments.
Regarding the missing tests, I think it's best to add them at opt level, but I can't do that now because there's no command line switch that enables KASan (this mode only depends on the bool passed to the pass factory). I can add such a switch, but then it can be used to work around the "ASan xor KASan" restriction. WDYT?

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1362

Or perhaps instrumentation of globals will follow shortly so it's ok to have this pass at hand.

Exactly, my intention was to add it soon.

1383

Looks like I've been comparing to the wrong gcc version, 4.9 didn't have the 'noabort' suffixes. Fixed.

1412

mm/kasan/kasan.c redefines memset() et al. making them call __asan_{load,store}N (see https://github.com/ramosian-glider/kasan/blob/kasan_llvmlinux/mm/kasan/kasan.c#L263), so we just replace the intrinsics with calls to memset().

1448

In fact I didn't fully understand this piece before. I've looked std::tie up and it's more clear to me now.
Moved the assignment outside the condition (or shall we NULL-initialize these fields instead?)

1535–1536

Done.

tools/clang/include/clang/Basic/Sanitizers.def
44

It hasn't been established yet, but Dima says we prefer "KASan" (which surprised me). Fixed here and in tools/clang/lib/CodeGen/CodeGenModule.cpp

tools/clang/lib/AST/Decl.cpp
3684–3685

I've added SanitizerSet::hasAsanOrKasan() -- did you mean that?

tools/clang/lib/CodeGen/BackendUtil.cpp
217

Can we keep it provided that we'll have module-level instrumentation soon?

tools/clang/lib/CodeGen/CodeGenFunction.cpp
619

Note I'm using the same attribute for both userspace and kernel instrumentation. I think we don't need to distinguish between them.

glider updated this revision to Diff 27667.Jun 15 2015, 6:53 AM
glider edited edge metadata.

Actually attaching the diff.

I think it's fine to add whatever commandline flags are needed for testing - they are not user-visible anyway, and we should have *some* means to test KASan instrumentation without a frontend.

Looks mostly good.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1448

I would actually prefer a null initialization, as it's weird to create functions that would never be called in KASan mode.

tools/clang/include/clang/Basic/Sanitizers.h
56

Um, no, I mean to add hasOneOf method that would take ArrayRef of sanitizer kinds and return true iff at least one of them is enabled. Then you could call it as...

tools/clang/lib/AST/Decl.cpp
3684–3685
Context.getLangOpts().Sanitize.hasOneOf({SanitizerKind::Address, SanitizerKind::KernelAddress})
tools/clang/lib/CodeGen/BackendUtil.cpp
203–204

Pass false here.

glider updated this revision to Diff 27761.Jun 16 2015, 7:06 AM

Added a couple of tests, addressed Alexey's comments.

I've added the tests for -fsanitize=kernel-address

  • check that it instruments memory accesses
  • check that the sanitize_address attributes are emitted

Regarding the offset/scale, these aren't emitted now, as I've only implemented the out-of-line instrumentation.
Regarding the missing module ctors, these will be added once we've globals instrumentation - not sure we need to test that they're absent now.

I've also added the -enable-kasan flag, but the existing tests do not use it yet.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1448

Done.

tools/clang/include/clang/Basic/Sanitizers.h
56

This is basically the has() method without the llvm::countPopulation(K) == 1 check.
Why not use a mask instead of an ArrayRef?

tools/clang/lib/CodeGen/BackendUtil.cpp
203–204

Done

dvyukov added inline comments.Jun 16 2015, 7:38 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1384

we also need to make them actually noabort

samsonov added inline comments.Jun 16 2015, 11:07 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
120

Um, asan-kernel? Looks like most of the flags in this file start with asan-, it makes sense to keep it.

394

nullptr
You can also consider using brace-or-equal initializer here.

tools/clang/include/clang/Basic/Sanitizers.h
56

Yeah, using the mask is also ok. But let's make the name different, so that has() method would still always accept a single sanitizer.

samsonov added inline comments.Jun 16 2015, 11:09 AM
tools/clang/test/Driver/kasan.c
1 ↗(On Diff #27761)

Please use linux as a target (http://reviews.llvm.org/D10467). You can also merge it into asan.c to keep a single test case (I'm fine either way).

glider updated this revision to Diff 27852.Jun 17 2015, 11:38 AM

Addressed comments by Alexey and Dmitry

lib/Transforms/Instrumentation/AddressSanitizer.cpp
120

Done

394

Added them for the ctor/init functions. What's the policy regarding those in the code?

1384

Removed the noabort suffixes for error reporting functions.
Per patch description, only out-of-line instrumentation is supported anyway.

tools/clang/test/Driver/kasan.c
1 ↗(On Diff #27761)

Done, united the tests.

samsonov added inline comments.Jun 17 2015, 12:27 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1366

Wait, this is dead code, right? CompileKernel is known to be false here, as you've checked it in the outer if.

tools/clang/lib/CodeGen/CGDeclCXX.cpp
270–271

hasOneOf

tools/clang/lib/CodeGen/SanitizerMetadata.cpp
28–29

(here and below)
I think you don't do anything with globals in KASan. Or you want to add it in the following changes soon?

tools/clang/test/CodeGen/address-safety-attr-kasan.cpp
5

Please specify triple (or merge this into address-safety-attr.cpp somehow)

11

Function Attrs: nounwind sanitize_address
would match this CHECK-NOASAN line

glider updated this revision to Diff 27928.Jun 18 2015, 5:47 AM

Addressed Alexey's comments

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1366

Removed the dead code for now. This block will be added back once we enable globals instrumentation.

tools/clang/lib/CodeGen/CGDeclCXX.cpp
270–271

Done

tools/clang/lib/CodeGen/SanitizerMetadata.cpp
28–29

Yes, I'm about to add globals support soon, it's just not working properly yet.

tools/clang/test/CodeGen/address-safety-attr-kasan.cpp
5

Done.

11

Added a $ after "nounwind"

samsonov accepted this revision.Jun 18 2015, 4:30 PM
samsonov edited edge metadata.

LGTM

tools/clang/lib/AST/Decl.cpp
3684–3685

remove extra parens around hasOneOf call

tools/clang/test/CodeGen/address-safety-attr-kasan.cpp
11

Please check this syntax. I thought that [[]] is used to define FileCheck variables, while {{}} are used for regular expressions.

tools/clang/test/Driver/asan.c
1–9

while you're here, please add linux to original RUN-lines as well.

This revision is now accepted and ready to land.Jun 18 2015, 4:30 PM
ygribov added inline comments.Jun 18 2015, 11:39 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1387

Do you mean to add noabort variants to compiler-rt? That would be nice, especially if you could add -fsanitize-recover to enable those in userspace as well.

glider updated this revision to Diff 28013.Jun 19 2015, 2:46 AM
glider edited edge metadata.

Addressed the comments by Alexey and Yury

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1387

No, I meant just emitting _noabort calls under KASan. I've updated the comment to reflect that.
-fsanitize-recover is kind of orthogonal to this patch.

tools/clang/lib/AST/Decl.cpp
3684–3685

Indeed.

tools/clang/test/CodeGen/address-safety-attr-kasan.cpp
11

Yeah, that's right.

tools/clang/test/Driver/asan.c
1–9

Done

Now there's a problem with handleNoSanitizeSpecificAttr() - looks like attribute((no_sanitize_address)) can only works with either ASan or KASan.
Looking.

glider updated this revision to Diff 28016.Jun 19 2015, 5:12 AM
glider updated this object.

Updated the test to use attribute((no_sanitize("kernel-address"))) as a workaround. We'll still need to decide which of the three attributes:

  • attribute((no_sanitize_address))
  • attribute((no_sanitize("address")))
  • attribute((no_sanitize("kernel-address")))

shall affect KASan instrumentation.

glider closed this revision.Jun 19 2015, 5:49 AM

Landed in r240131.