Page MenuHomePhabricator

[ASan] Do not misrepresent high value address dereferences as null dereferences
ClosedPublic

Authored by yln on Oct 8 2019, 5:58 PM.

Details

Summary

Dereferences with addresses above the 48-bit hardware addressable range
produce "invalid instruction" (instead of "invalid access") hardware
exceptions (there is no hardware address decoding logic for those bits),
and the address provided by this exception is the address of the
instruction (not the faulting address). The kernel maps the "invalid
instruction" to SEGV, but fails to provide the real fault address.

Because of this ASan lies and says that those cases are null
dereferences. This downgrades the severity of a found bug in terms of
security. In the ASan signal handler, we can not provide the real
faulting address, but at least we can try not to lie.

rdar://50366151

So far, I tested this on macOS and putting it up for feedback.
Before landing this, I will move the test to asan/Posix and run ninja check-asan on Linux.

Diff Detail

Event Timeline

yln created this revision.Oct 8 2019, 5:58 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
yln edited the summary of this revision. (Show Details)Oct 8 2019, 6:09 PM
vitalybuka added inline comments.Oct 9 2019, 1:32 AM
compiler-rt/lib/sanitizer_common/sanitizer_posix.cpp
302 ↗(On Diff #223966)

can you move it into darwin
on linux si_code is SI_KERNEL in similar cases so we will need different implementations

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_report.cpp
194–201

can you change it to

if (trueaddress)
  Print("Full error with address")
else
  Print("Full error without address")

I guess more readable and less problems when multiple processes write into same terminal

yln updated this revision to Diff 224188.Oct 9 2019, 3:15 PM

Address Vitaly's comments.

yln marked 2 inline comments as done.Oct 9 2019, 3:23 PM
vitalybuka accepted this revision.Oct 9 2019, 3:46 PM
This revision is now accepted and ready to land.Oct 9 2019, 3:46 PM
yln updated this revision to Diff 224213.Oct 9 2019, 5:14 PM

Move test to 'asan/Posix' and provide Linux implementation for 'SignalContext::IsTrueFaultingAddress'.

This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
949

This breaks a bunch of bots with /home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp:949: Missing username in TODO; it should look like "// TODO(my_username): Stuff." [readability/todo] [2]

I've reverted this to get the bots working again.

yln added a comment.Oct 10 2019, 10:17 AM

Apologies for the failing bots.