This is an archive of the discontinued LLVM Phabricator instance.

[KernelAddressSanitizer] Make globals constructors compatible with kernel [v2]
ClosedPublic

Authored by melver on Jun 8 2020, 6:59 AM.

Details

Summary

Note: The first version of this feature was reverted due to modpost causing failures. This v2 fixes this. More info:

https://github.com/ClangBuiltLinux/linux/issues/1045#issuecomment-640381783

This makes -fsanitize=kernel-address emit the correct globals
constructors for the kernel. We had to do the following:

  • Disable generation of constructors that rely on linker features such as dead-global elimination.
  • Only instrument globals *not* in explicit sections. The kernel uses sections for special globals, which we should not touch.
  • Do not instrument globals that are prefixed with "" nor that are aliased by a symbol that is prefixed with "". For example, modpost relies on specially named aliases to find globals and checks their contents. Unfortunately modpost relies on size stored as ELF debug info and any padding of globals currently causes the debug info to cause size reported to be *with* redzone which throws modpost off.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203493

Tested:

  • With 'clang/test/CodeGen/asan-globals.cpp'.
  • With test_kasan.ko, we can see:

    BUG: KASAN: global-out-of-bounds in kasan_global_oob+0xb3/0xba [test_kasan]
  • allyesconfig, allmodconfig (x86_64)

Diff Detail

Event Timeline

melver created this revision.Jun 8 2020, 6:59 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
melver updated this revision to Diff 269229.Jun 8 2020, 7:46 AM

Also ignore non-alias globals prefixed by __.

melver edited the summary of this revision. (Show Details)Jun 8 2020, 7:54 AM
melver added a subscriber: nickdesaulniers.
melver edited the summary of this revision. (Show Details)Jun 8 2020, 1:28 PM

I do not know enough about KASAN enough to review this patch but I can say that against mainline at https://git.kernel.org/linus/af7b4801030c07637840191c69eb666917e4135d, there appear to be no visible regressions with this version of the patch on top of caa2fddce72f2e8ca3d6346cc2c8fe85252b91d8.

I do not know enough about KASAN enough to review this patch but I can say that against mainline at https://git.kernel.org/linus/af7b4801030c07637840191c69eb666917e4135d, there appear to be no visible regressions with this version of the patch on top of caa2fddce72f2e8ca3d6346cc2c8fe85252b91d8.

Thank you for testing this!

melver retitled this revision from [KernelAddressSanitizer] Make globals constructors compatible with kernel to [KernelAddressSanitizer] Make globals constructors compatible with kernel [v2].Jun 9 2020, 12:46 AM
melver removed a reviewer: nathanchance.
melver added a subscriber: nathanchance.
glider added inline comments.Jun 9 2020, 7:22 AM
clang/test/CodeGen/asan-globals.cpp
16

Need a comment explaining this case a bit.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
759

UseGlobalsGC assumes !this->CompileKernel already, doesn't it?

1796

Maybe it's better to call this function only if CompileKernel is true?
I don't think we need it except for the kernel.

1942

This change looks unrelated to the rest.

melver updated this revision to Diff 269578.Jun 9 2020, 9:53 AM
melver marked 6 inline comments as done.

Address comments.

melver added a comment.Jun 9 2020, 9:55 AM

Thanks! PTAL.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
759

I experimented a bit and found that even if one of them is enabled things break (say if you remove UseGlobalsGC here). This is more explicit and less error-prone in case somebody removes UseGlobalsGC from UseCtorComdat. And there is no real penalty here for having it on both.

1796

Done, though the previous version was cleaner. I guarded the call but now we need an assert and a comment to make sure the check is moved when people decide to use this for more than the kernel.

melver updated this revision to Diff 269582.Jun 9 2020, 9:58 AM

Fix test broken by linter.

melver updated this revision to Diff 269751.Jun 10 2020, 1:01 AM

Fix typos.

melver updated this revision to Diff 269780.Jun 10 2020, 3:35 AM

Fix test on windows.

glider accepted this revision.Jun 10 2020, 3:53 AM

LGTM assuming allyesconfig builds.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
759

That's strange, but ok.

This revision is now accepted and ready to land.Jun 10 2020, 3:53 AM
This revision was automatically updated to reflect the committed changes.