This is an archive of the discontinued LLVM Phabricator instance.

[HWASAN] Improve tag-mismatch diagnostics
ClosedPublic

Authored by evgeny777 on Jan 11 2019, 8:33 AM.

Details

Summary

This patch improves tag-mismatch report in the following ways:

  • SigTrap explicitly sets X0 register so fault address and tags are correctly shown on AArch64
  • Access sizes not equal to power of 2 are correctly shown on both AArch64 and X86_64
  • ptr and mem tags are displayed correctly when SigTrap is invoked from CheckAddressSized

Diff Detail

Event Timeline

evgeny777 created this revision.Jan 11 2019, 8:33 AM
eugenis added inline comments.Jan 11 2019, 9:01 AM
lib/hwasan/hwasan_checks.h
98

This would report an error with the size of the memory access being equal to the distance from the first bad granule to the end of the memset() buffer. This sounds very confusing.

I suggest doing the sized version of SigTrap with the starting address and the full size of the buffer, and then adding extra analysis in reporting code. The result could be a note saying that the first bad byte is somewhere at offset [A, A+16] from the start of the accessed range.

evgeny777 updated this revision to Diff 181508.Jan 14 2019, 1:54 AM

Addressed review comments

eugenis added inline comments.Jan 14 2019, 4:48 PM
lib/hwasan/hwasan_checks.h
27–28

this can be done better with register asm:

register uptr x0 asm("x0") = p;

and then use "r"(x0) in asm constraints.

It would also tell the compiler that x0 is clobbered, which might matter in the -fsanitize-recover mode.

lib/hwasan/hwasan_report.cc
411

Reuse __hwasan_test_shadow.

evgeny777 marked an inline comment as done.Jan 15 2019, 6:56 AM
evgeny777 added inline comments.
lib/hwasan/hwasan_report.cc
411

This function looks a bit strange to me. For instance it can return -1 either in case of error or in case of a tag mismatch when address is equal to start of granule plus one. It would make much more sense if this function returned pointer instead of offset

evgeny777 updated this revision to Diff 181784.Jan 15 2019, 7:41 AM

Changed way X0 and X1 are assigned on AArch64

eugenis added inline comments.Jan 15 2019, 12:56 PM
lib/hwasan/hwasan_report.cc
411

Oh, that's a good catch! I'd prefer the interface to stay as it is for consistency with __msan_test_shadow, but fix the return offset to be within [0, sz] is case of an error.

Also, this function assumes that tag 0 is match-all, which is not the case. The ptr_tag==0 check could be removed.

eugenis added inline comments.Jan 15 2019, 12:59 PM
lib/hwasan/hwasan_checks.h
28

brk %1

eugenis accepted this revision.Jan 17 2019, 3:53 PM
eugenis added a subscriber: pcc.

LGTM

lib/hwasan/hwasan_report.cc
415

This phrase sounds awkward. There should probably be a "the" before "first", at least... @pcc

This revision is now accepted and ready to land.Jan 17 2019, 3:53 PM
eugenis requested changes to this revision.Jan 17 2019, 3:55 PM

And we should probably skip this note in the case where it is obvious - for example, for an 8 byte, 8-aligned access!

This revision now requires changes to proceed.Jan 17 2019, 3:55 PM

assume that case is not "obvious" when offset is not zero.

eugenis accepted this revision.Jan 18 2019, 10:40 AM

LGTM with comments.

Please add a -NOT test line somewhere to check that the new note is not printed in "obvious" cases.

test/hwasan/TestCases/mem-intrinsics.c
27

I think this could be flaky - there is a chance that PTR_TAG will the be last one on its line, and MEM_TAG - the first one on the following line. Align the buffer to at least 256 byte to prevent this from happening. Perhaps simply mmap a page of memory?

This revision is now accepted and ready to land.Jan 18 2019, 10:40 AM
This revision was automatically updated to reflect the committed changes.