This is an archive of the discontinued LLVM Phabricator instance.

[HWASan] Ignore shortgranules for global tag selection
ClosedPublic

Authored by hctim on May 16 2023, 5:47 PM.

Details

Summary

Tag selection for global variables is sequential, starting at a
pseduo-ish seed that's based on the hash of the file name.

Previously, it was possible for a global to be assigned a tag in the
range [1,15]. If the global's size was not a multiple of granules (i.e.
size % 16 != 0), then the last granule of the global would be assigned
a short granule tag as well.

If the real memory tag of the global (e.g. '04') happened to collide
with the short granule tag (e.g. '04'), then __hwasan_check would
see that the memory tag matched the short granule tag, and dutifully
assume (in this fast check) that everthing is okay.

Unfortunately, if you tried to access the [5,15]th byte, you never get
to the short granule check. This means you miss intra-granule overflows
on the last granule of a global, if said global was assigned a real
memory tag in the range [1,15].

This causes flakiness in certain global tests, if the SHA of the
filename changes between runs.

This problem also exists for heap and stack allocations as well, but
there's a concern that if we exclude the [1,15] range for heap and stack
that it's an unhappy tradeoff. On Android, this would mean that a 1/255
chance of false positive becomes 1/240. On other platforms though (that
have a less-than-8-bit tag space), this may be unacceptable. We can
think about potential fixes for that in other places, but globals are
fine to reduce the space, as really the only thing that matters is
catching sequential overflow. The false-negative scenario is much, much
more common in use-after-free and use-after-scope, which globals don't
have.

Diff Detail

Event Timeline

hctim created this revision.May 16 2023, 5:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 5:47 PM
hctim requested review of this revision.May 16 2023, 5:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 5:47 PM

Can you update some test?
Maybe it will trigger some test without ClUseShortGranules

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
1590–1591

Tag &= TagMaskByte;
is effectively
Tag = Tag % (TagMaskByte + 1);

so to map them above 15:
Tag = 16 + Tag % (TagMaskByte + 1 - 16);

1590–1597
1595

let's remove conditions and do that all the time

hctim marked 3 inline comments as done.May 17 2023, 5:05 PM

Can you update some test?

Added a new test.

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
1590–1597

I think I've got a simpler approach:

if (Tag < 16 || Tag > TagMaskByte)
  Tag = 16;
hctim updated this revision to Diff 523221.May 17 2023, 5:05 PM
hctim marked an inline comment as done.

Update from Vitaly's comments.

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 5:05 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.May 18 2023, 5:39 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
1588

We are here only if it's our mistake in this file, this is assert()

vitalybuka accepted this revision.May 18 2023, 5:41 PM
This revision is now accepted and ready to land.May 18 2023, 5:41 PM
This revision was landed with ongoing or failed builds.May 18 2023, 5:49 PM
This revision was automatically updated to reflect the committed changes.