This is an archive of the discontinued LLVM Phabricator instance.

[hwasan] print exact mismatch offset for short granules.
ClosedPublic

Authored by fmayer on Jun 17 2021, 7:27 AM.

Diff Detail

Event Timeline

fmayer requested review of this revision.Jun 17 2021, 7:27 AM
fmayer created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2021, 7:27 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

The reason it was done this way was that what looks like a short granule might not be one - sometimes a low tag value is simply a low tag value, and the last byte of the granule is user memory that can match by chance.

On the other hand, the chance of that happening is very low and perhaps this is a good tradeoff for better report quality.

@pcc @hctim WDYT?

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c
15

why offset 10 and not 5?

fmayer added inline comments.Jun 17 2021, 12:36 PM
compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c
15

The first 5 bytes, [5, 10) are the valid access, and the invalid one is [10, 26).

The reason it was done this way was that what looks like a short granule might not be one - sometimes a low tag value is simply a low tag value, and the last byte of the granule is user memory that can match by chance.

On the other hand, the chance of that happening is very low and perhaps this is a good tradeoff for better report quality.

@pcc @hctim WDYT?

isn't the chance of this lower than a normal tag matching by accident (also producing an incorrect report)? The chance of this should be (16 / 256) * (1 / 256).

fmayer marked an inline comment as not done.Jun 17 2021, 12:41 PM
fmayer added inline comments.
compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c
15

Sorry, my mistake, replied too fast. Will look tomorrow.

isn't the chance of this lower than a normal tag matching by accident (also producing an incorrect report)? The chance of this should be (16 / 256) * (1 / 256).

Right, we can find an unrelated alloc/dealloc stack trace because of an accidental tag match. That happens.

fmayer updated this revision to Diff 352945.Jun 18 2021, 2:21 AM
fmayer marked an inline comment as not done.

Fixed logic.

fmayer added inline comments.Jun 18 2021, 2:22 AM
compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c
15

Fixed. Sorry about that, added second test as well.

eugenis added inline comments.Jun 18 2021, 11:31 AM
compiler-rt/lib/hwasan/hwasan_report.cpp
635

Is this really different from ShadowToMem(tag_ptr) ?
If you prefer this form, please put (untagged_addr + offset) in brackets to make operator precedence clear.

But if I read __hwasan_test_shadow correctly, (untagged_addr + offset) must be granule-aligned already.

fmayer updated this revision to Diff 353333.Jun 21 2021, 4:14 AM

Improve readability.

fmayer updated this revision to Diff 353338.Jun 21 2021, 4:37 AM

Add test for aligned (untagged_addr + offset).

fmayer added inline comments.Jun 21 2021, 4:39 AM
compiler-rt/lib/hwasan/hwasan_report.cpp
635

Is this really different from ShadowToMem(tag_ptr) ?

I wrote it this way so it's easy to see I am separating the low from the high bits from this and the line below.

If you prefer this form, please put (untagged_addr + offset) in brackets to make operator precedence clear.

Done.

But if I read __hwasan_test_shadow correctly, (untagged_addr + offset) must be granule-aligned already.

There is the edge case where offset == 0, then untagged_addr + offset is not necessarily aligned.
I added more test cases to cover both the aligned and non-aligned scenarios.

eugenis added inline comments.Jun 21 2021, 3:45 PM
compiler-rt/lib/hwasan/hwasan_report.cpp
641

This line assumes that the first bad access has to be 1 byte past the end.

What if ex. the short granule is 5 byte in size, and the access is for offsets [10, 12) ?

fmayer updated this revision to Diff 353641.Jun 22 2021, 7:13 AM

Correctly handle accesses out of range of granule.

fmayer marked 2 inline comments as done.Jun 22 2021, 7:18 AM

Please add a test where the bad access starts after the end of the allocation.

LGTM with that and the other comments.

compiler-rt/lib/hwasan/hwasan_report.cpp
634

call it "granule_ptr" or something like that.

638

"this is the offset of the leftmost accessed byte within the bad granule" or something more literate. I.e. it's not the bad address, it's the start of the accessed ranged clamped to the granule bounds.

643

"if the access starts after the end of the short granule, then the first bad byte is the first byte of the access; otherwise it is the first byte past the end of the short granule" or something like that in your own words. This is not really obvious from the code.

compiler-rt/test/hwasan/TestCases/heap-buffer-overflow-into.c
22

I was really confused why the size is 10 until I realized that in "%t 5 2>&1" 2 is the file descriptor and not the argument. Please make all arguments explicit in the command lines and remove the argc checks (or rather turn them into asserts).

fmayer updated this revision to Diff 353916.Jun 23 2021, 3:35 AM
fmayer marked 4 inline comments as done.

Update comments and tests.

eugenis accepted this revision.Jun 23 2021, 1:30 PM

LGTM

This revision is now accepted and ready to land.Jun 23 2021, 1:30 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.Jun 24 2021, 5:33 PM
This revision is now accepted and ready to land.Jun 24 2021, 5:33 PM
fmayer updated this revision to Diff 354845.Jun 28 2021, 4:53 AM

Fix test on aarch64.

Tested on Android and x86.

This revision was automatically updated to reflect the committed changes.
fmayer reopened this revision.Jun 29 2021, 4:12 AM
This revision is now accepted and ready to land.Jun 29 2021, 4:12 AM
fmayer updated this revision to Diff 382887.Oct 27 2021, 7:21 PM

do not check short granule if mem tag is 0.

fmayer marked an inline comment as done.Oct 27 2021, 7:21 PM
This revision was landed with ongoing or failed builds.Oct 27 2021, 7:31 PM
This revision was automatically updated to reflect the committed changes.