Page MenuHomePhabricator

hwasan: add -fsanitize=kernel-hwaddress
ClosedPublic

Authored by andreyknvl on Mar 29 2018, 7:36 AM.

Details

Summary

Add -fsanitize=kernel-hwaddress flag, that enables -hwasan-kernel=1 -hwasan-recover=1 -hwasan-match-all-tag=0xff.

Diff Detail

Repository
rC Clang

Event Timeline

andreyknvl created this revision.Mar 29 2018, 7:36 AM
eugenis added inline comments.Mar 29 2018, 1:16 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
732 ↗(On Diff #140292)

I'd prefer a CHECK(!CompileKernel || Recover).

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
345 ↗(On Diff #140292)

How about matchAllTag = ClMatchAllTag.getNumOccurrences() > 0 ? ClMatchAllTag : (CompileKernel ? 0xFF : -1) ?

In fact, the same logic could be applied to CompileKernel and Recover in the constructor.

llvm/test/Instrumentation/HWAddressSanitizer/X86/kernel.ll
16 ↗(On Diff #140292)

what happened here? Recover vs trap difference?

andreyknvl updated this revision to Diff 140777.Apr 3 2018, 7:04 AM

Addressed @eugenis comments.

andreyknvl updated this revision to Diff 140784.Apr 3 2018, 7:16 AM
andreyknvl marked 2 inline comments as done.
andreyknvl marked an inline comment as done.
andreyknvl added inline comments.
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
345 ↗(On Diff #140292)

getNumOccurrences is what I was looking for!

Done for ClMatchAllTag and ClRecover.

For ClRecover the behavior now is: when ClRecover is provided, use it's value; otherwise use True if CompileKernel and the value passed from CGOpts if not CompileKernel.

llvm/test/Instrumentation/HWAddressSanitizer/X86/kernel.ll
16 ↗(On Diff #140292)

Had to change that since I forced ClRecover when ClKhwasan. Fixed now.

eugenis added inline comments.Apr 3 2018, 11:39 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
345 ↗(On Diff #140292)

Looking at this:

this->Recover = ClRecover.getNumOccurrences() > 0 ? ClRecover : Recover;

Recover does not depend on CompileKernel at all.

Why not do the same getNumOccurrences() thing for CompileKernel?

What happens if the user asks for -fsanitize=kernel-hwaddress -fsanitize-recover=kernel-hwaddress? Looks like recover will be silently ignored. Could this be fixed by declaring it non-recoverable in SanitizerArgs?

andreyknvl updated this revision to Diff 140975.Apr 4 2018, 9:02 AM
andreyknvl marked an inline comment as done.

Check ClEnableK[hw]asan.getNumOccurrences().

andreyknvl added inline comments.Apr 4 2018, 9:09 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
345 ↗(On Diff #140292)

Why not do the same getNumOccurrences() thing for CompileKernel?

Done

What happens if the user asks for -fsanitize=kernel-hwaddress -fsanitize-recover=kernel-hwaddress? Looks like recover will be silently ignored. Could this be fixed by declaring it non-recoverable in SanitizerArgs?

Ah, so there's -fsanitize-recover and that's where those CodeGenOptions come from :)

You probably meant -fsanitize=kernel-hwaddress -fnosanitize-recover=kernel-hwaddress (-fsanitize=kernel-[hw]address always enables recover right now)? Yes, in this case it the latter flag will be ignored. Is there a way to mark -fsanitize=kernel-address non-unrecoverable?

There's a little chance that we might want to change that in the future and make KASAN/KHWASAN support the recover=0 mode.

eugenis added inline comments.Apr 4 2018, 2:39 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
345 ↗(On Diff #140292)

It would need to be done in SanitizerArgs.cpp (see ex. Unrecoverable). It appears that we don't have any sanitizers that are not-not-recoverable yet.

andreyknvl updated this revision to Diff 141329.Apr 6 2018, 6:34 AM

PTAL. Added AlwaysRecoverable to SanitizerArgs.cpp. Right now recovery is disallowed for both KASan and KHWASan, but all the appropriate flags seem to be passed properly to the instrumentation pass, so we can easily allow it in the future in we want.

andreyknvl edited the summary of this revision. (Show Details)Apr 6 2018, 6:35 AM

Also rebased onto the latest tip.

eugenis added inline comments.Apr 6 2018, 9:49 AM
clang/lib/Driver/SanitizerArgs.cpp
467 ↗(On Diff #141329)

Looks great, but this new logic needs a test.

Added a test.

eugenis accepted this revision.Apr 6 2018, 10:28 AM
This revision is now accepted and ready to land.Apr 6 2018, 10:28 AM

Thanks! Could you commit this as well?

the tests fail with:
clang: /code/llvm-project/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1064: llvm::FunctionPass *llvm::createAddressSanitizerFunctionPass(bool, bool, bool): Assertion `!CompileKernel || Recover' failed.

Hm, I don't see this failure. Just in case I rebased onto the latest tip.

How do you run the tests? I tried both make -j48 check-clang check-llvm and make -j48 check-all. I do see some tests failing, but they are not related to Sanitizers.

Simply ninja check-all. Maybe the patch got applied incorrectly. Phabricator is really bad with file copies.

eugenis added inline comments.Apr 10 2018, 12:58 PM
clang/test/CodeGen/address-safety-attr-flavors.cpp
16 ↗(On Diff #141841)

^^^ this line expands to this:
/code/build-llvm/bin/clang -cc1 -internal-isystem /code/build-llvm/lib/clang/7.0.0/include -nostdsysteminc -triple i386-unknown-linux -fsanitize=kernel-address -disable-O0-optnone -emit-llvm -o - /code/llvm-project/clang/test/CodeGen/address-safety-attr-flavors.cpp

and

clang: /code/llvm-project/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1064: llvm::FunctionPass *llvm::createAddressSanitizerFunctionPass(bool, bool, bool): Assertion `!CompileKernel || Recover' failed.
#0 0x00000000020af584 PrintStackTraceSignalHandler(void*) (/code/build-llvm/bin/clang+0x20af584)
#1 0x00000000020af8c6 SignalHandler(int) (/code/build-llvm/bin/clang+0x20af8c6)
#2 0x00007f53abd690c0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x110c0)
#3 0x00007f53aa8fafcf gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x32fcf)
#4 0x00007f53aa8fc3fa abort (/lib/x86_64-linux-gnu/libc.so.6+0x343fa)
#5 0x00007f53aa8f3e37 (/lib/x86_64-linux-gnu/libc.so.6+0x2be37)
#6 0x00007f53aa8f3ee2 (/lib/x86_64-linux-gnu/libc.so.6+0x2bee2)
#7 0x0000000002c5e817 (/code/build-llvm/bin/clang+0x2c5e817)
#8 0x000000000228b0e1 addKernelAddressSanitizerPasses(llvm::PassManagerBuilder const&, llvm::legacy::PassManagerBase&) (/code/build-llvm/bin/clang+0x228b0e1)
#9 0x0000000001c37c69 llvm::PassManagerBuilder::addExtensionsToPM(llvm::PassManagerBuilder::ExtensionPointTy, llvm::legacy::PassManagerBase&) const (/code/build-llvm/bin/clang+0x1c37c69)
#10 0x0000000001c38fb7 llvm::PassManagerBuilder::populateModulePassManager(llvm::legacy::PassManagerBase&) (/code/build-llvm/bin/clang+0x1c38fb7)
#11 0x000000000228093b clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/code/build-llvm/bin/clang+0x228093b)
#12 0x0000000002afb83c clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/code/build-llvm/bin/clang+0x2afb83c)
#13 0x0000000003368e24 clang::ParseAST(clang::Sema&, bool, bool) (/code/build-llvm/bin/clang+0x3368e24)
#14 0x0000000002758f38 clang::FrontendAction::Execute() (/code/build-llvm/bin/clang+0x2758f38)
#15 0x0000000002716451 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/code/build-llvm/bin/clang+0x2716451)
#16 0x000000000280d0c1 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/code/build-llvm/bin/clang+0x280d0c1)
#17 0x00000000008ff540 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/code/build-llvm/bin/clang+0x8ff540)

Fixed the assert. PTAL.

The reason tests didn't work for me was Release build mode. Tried Debug mode - apparently 64 GB RAM is not enough to build llvm =/ Ended up using Release build with asserts enabled.

I've also got commit access now, so I'll try to commit this myself.

Could be too many parallel links, try -DLLVM_PARALLEL_LINK_JOBS=3.

Anyway, Debug build is way too slow. I normally use Release with asserts, too.

LGTM (+missing *-commits MLs)

This revision was automatically updated to reflect the committed changes.