Page MenuHomePhabricator

[KernelAddressSanitizer] Make globals constructors compatible with kernel
ClosedPublic

Authored by melver on May 29 2020, 8:17 AM.

Details

Summary

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 emit constructors for globals *not* in explicit sections. The kernel uses sections for special globals, which we should not touch.

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

Tested:

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

    BUG: KASAN: global-out-of-bounds in kasan_global_oob+0xb3/0xba [test_kasan]

Diff Detail

Event Timeline

melver created this revision.May 29 2020, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 8:17 AM
melver edited the summary of this revision. (Show Details)May 29 2020, 8:22 AM
melver updated this revision to Diff 267253.May 29 2020, 8:26 AM

Add missing comment.

Could you please add an IR test for this?

Could you please add an IR test for this?

Was planning to -- sorry, I should have mentioned: before I go add tests, does this look sane to you?

The change looks good to me.

glider added a comment.Jun 2 2020, 5:41 AM

Could you please add an IR test for this?

Was planning to -- sorry, I should have mentioned: before I go add tests, does this look sane to you?

I thought we didn't have support for __attribute__((constructor)) in the kernel? Does GCC emit constructors for globals with KASAN?

Could you please add an IR test for this?

Was planning to -- sorry, I should have mentioned: before I go add tests, does this look sane to you?

I thought we didn't have support for __attribute__((constructor)) in the kernel? Does GCC emit constructors for globals with KASAN?

I don't think this has much to do with attribute((constructor)), since that's never used explicitly. But implicit constructors are emitted. GCC emits them with KASAN. The disassembled constructors for globals to initialize KASAN with both GCC and Clang are now pretty much identical (apart from symbol names).

The kernel calls them here: https://elixir.bootlin.com/linux/latest/source/init/main.c#L1046

Cool, didn't know that; then the change sure looks reasonable.

melver updated this revision to Diff 268796.Jun 5 2020, 7:39 AM

Update IR test.

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 7:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
melver edited the summary of this revision. (Show Details)Jun 5 2020, 7:40 AM
glider accepted this revision.Jun 5 2020, 10:13 AM

LGTM

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

Hi, this broke check-clang on mac: http://45.33.8.238/mac/14998/step_7.txt

Please take a look, and if it takes a while to fix please revert while you investigate.

nickdesaulniers added inline comments.Jun 5 2020, 1:31 PM
clang/test/CodeGen/asan-globals.cpp
14

/Users/thakis/src/llvm-project/clang/test/CodeGen/asan-globals.cpp:14:28: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
int attribute((section(".foo.bar"))) sectioned_global;

^

http://45.33.8.238/mac/14998/step_7.txt

Using an explicit Linux target triple would avoid this.

See also attribute-section-data-common.c for the OSX portable example.

thakis added inline comments.Jun 5 2020, 1:36 PM
clang/test/CodeGen/asan-globals.cpp
14

Using an explicit Linux target triple would avoid this.

It'd also make the test fewer things, since it currently tests asan on mac (when run on mac).

If the attribute-section-data-common.c approach works here, great. If not, maybe the linux-specific bits could go in a new test file?

Are you planning on fixing the test, or should we revert for now so we can figure things out without the tree being red?

melver added a comment.Jun 5 2020, 2:05 PM

Hi, this broke check-clang on mac: http://45.33.8.238/mac/14998/step_7.txt

Please take a look, and if it takes a while to fix please revert while you investigate.

Sent https://reviews.llvm.org/D81306