Page MenuHomePhabricator

hwasan: Compatibility fixes for short granules.
ClosedPublic

Authored by pcc on Sep 25 2019, 3:49 PM.

Details

Summary

We can't use short granules with stack instrumentation when targeting older
API levels because the rest of the system won't understand the short granule
tags stored in shadow memory.

Moreover, we need to be able to let old binaries (which won't understand
short granule tags) run on a new system that supports short granule
tags. Such binaries will call the hwasan_tag_mismatch function when their
outlined checks fail. We can compensate for the binary's lack of support
for short granules by implementing the short granule part of the check in
the
hwasan_tag_mismatch function. Unfortunately we can't do anything about
inline checks, but I don't believe that we can generate these by default on
aarch64, nor did we do so when the ABI was fixed.

A new function, __hwasan_tag_mismatch_v2, is introduced that lets code
targeting the new runtime avoid redoing the short granule check. Because tag
mismatches are rare this isn't important from a performance perspective; the
main benefit is that it introduces a symbol dependency that prevents binaries
targeting the new runtime from running on older (i.e. incompatible) runtimes.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Sep 25 2019, 3:49 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 25 2019, 3:49 PM
Herald added subscribers: Restricted Project, hiraditya, kristof.beyls, srhines. · View Herald Transcript
eugenis added inline comments.Sep 25 2019, 6:10 PM
compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S
55 ↗(On Diff #221852)

This comment is wrong, isn't it? X1 is the encoded access info, which includes size as well as a read/write bit and more. Could you fix this please?

And x0 is not the address of an instruction. It's the data address.

compiler-rt/test/hwasan/TestCases/stack-oob.c
3 ↗(On Diff #221852)

Would it be useful to test values of SIZE near 0?

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
623 ↗(On Diff #221852)

Do you think this is better expressed as a new intrinsic? I don't like UseShortGranules mixed into AccessInfo, which has lots of stuff encoded in it, but at least it has the same format everywhere (compiler & runtime library).

644 ↗(On Diff #221852)

Is this condition right? If not using short granules, an access can be bad from the point of view of this code, but good from the runtime library POV and we might get control back.

Should this be UseShortGranules && !Recover ?

llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
11 ↗(On Diff #221852)

I'm not sure why the number in __hwasan_check_* went down when you added more stuff into it. Are you matching partial string?

pcc updated this revision to Diff 222051.Sep 26 2019, 4:56 PM
pcc marked 8 inline comments as done.
  • Address review comments
pcc added inline comments.Sep 26 2019, 4:56 PM
compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S
55 ↗(On Diff #221852)

Yes, it's wrong, I've fixed it.

compiler-rt/test/hwasan/TestCases/stack-oob.c
3 ↗(On Diff #221852)

I added a test for SIZE=2. (SIZE=1 failed to detect an error to my surprise, but I realised that it was because deterministic tagging was giving us a tag of 1 which was equal to the short tag.)

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
623 ↗(On Diff #221852)

Yes, adding a new intrinsic seems like the best option if we want to keep the AccessInfo format the same everywhere. Done.

644 ↗(On Diff #221852)

The condition here is based on the diff from D63908. Recovery is always based on the Recover flag, but in the short granule case the unreachable block is created on line 652 instead.

That said, maybe it would be better to not change this part of the code at all, as this would let short granules work in code compiled with new compilers targeting old API levels running on new systems, even when we end up emitting an inline check. That's what I've now done.

llvm/test/CodeGen/AArch64/hwasan-check-memaccess.ll
11 ↗(On Diff #221852)

It's because I changed the value of AccessInfo so that I could test the emission of the check both with and without short granules. I ended up adjusting this code again for the new intrinsic.

eugenis accepted this revision.Sep 26 2019, 5:16 PM

LGTM

This revision is now accepted and ready to land.Sep 26 2019, 5:16 PM
This revision was automatically updated to reflect the committed changes.