This is an archive of the discontinued LLVM Phabricator instance.

[KernelAddressSanitizer] Fix globals exclusion for indirect aliases
ClosedPublic

Authored by melver on Dec 8 2020, 7:18 AM.

Details

Summary

GlobalAlias::getAliasee() may not always point directly to a
GlobalVariable. In such cases, try to find the canonical GlobalVariable
that the alias refers to.

Link: https://github.com/ClangBuiltLinux/linux/issues/1208

Diff Detail

Unit TestsFailed

Event Timeline

melver created this revision.Dec 8 2020, 7:18 AM
melver requested review of this revision.Dec 8 2020, 7:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 7:18 AM
melver updated this revision to Diff 310214.Dec 8 2020, 7:48 AM

Fix recursive case

dvyukov added a subscriber: dvyukov.Dec 8 2020, 8:25 AM

Have you checked if there is already a function that does this? Frequently there is :)
Some functions that look similar based on names:
stripPointerCasts
stripPointerCastsAndOffsets
stripPointerCastsAndAliases
canonicalizeAlias

Harbormaster completed remote builds in B81455: Diff 310191.
melver updated this revision to Diff 310488.Dec 9 2020, 3:47 AM

Add test

Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2020, 3:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
melver updated this revision to Diff 310492.Dec 9 2020, 4:07 AM

Simplify using stripPointerCastsAndAliases()

melver added a comment.Dec 9 2020, 4:07 AM

Have you checked if there is already a function that does this? Frequently there is :)
Some functions that look similar based on names:
stripPointerCasts
stripPointerCastsAndOffsets
stripPointerCastsAndAliases
canonicalizeAlias

Yup, stripPointerCastsAndAliases() works.

melver updated this revision to Diff 310495.Dec 9 2020, 4:15 AM

Revert unnecessary reformat

dvyukov accepted this revision.Dec 9 2020, 4:28 AM

The code looks reasonable to me. I see it only affects kernel, so assuming you booted kernel, we should be fine.
I can rubber-stamp it, but if you want more meaningful review, please wait for Alex or Nick.

This revision is now accepted and ready to land.Dec 9 2020, 4:28 AM
melver added a comment.Dec 9 2020, 9:22 AM

The code looks reasonable to me. I see it only affects kernel, so assuming you booted kernel, we should be fine.
I can rubber-stamp it, but if you want more meaningful review, please wait for Alex or Nick.

Added Nick.

I suppose all I'd like a 2nd confirmation on is if stripPointerCastsAndAliases() is the right thing to use here.

Nick or Alex, if you could have a brief look would be great -- thanks!

Thanks,

  • Marco
nickdesaulniers accepted this revision.Dec 10 2020, 1:35 PM

Thanks for the patch.

clang/test/CodeGen/asan-globals-alias.cpp
30

Do we want to add checks for the aliases: global_alias_2, __global_alias_2_alias, and __mod_joydev_ids_device_table?

melver added inline comments.Dec 10 2020, 3:28 PM
clang/test/CodeGen/asan-globals-alias.cpp
30

I found that depending on optimization level, what the aliases end up aliasing varies (e.g. for the alias-of-alias at -O2 it turns it into an alias to the global_alias_2). Not sure how to best capture this in the test without it becoming flaky. I suppose we could check that the alias somehow references either what was specified directly or the final global, but I don't think that's the job of this test since it's not directly related to ASan or KASAN.

Or did you mean to just check they exist?

clang/test/CodeGen/asan-globals-alias.cpp
30

Existence check. If it doesn't add anything, feel free to skip the suggestion.

melver updated this revision to Diff 311157.Dec 11 2020, 3:19 AM

Check aliases exist in test

This revision was landed with ongoing or failed builds.Dec 11 2020, 3:30 AM
This revision was automatically updated to reflect the committed changes.