This is an archive of the discontinued LLVM Phabricator instance.

[HWASAN] Implement munmap interceptor for HWASAN
ClosedPublic

Authored by kstoimenov on Jun 12 2023, 5:25 PM.

Diff Detail

Event Timeline

kstoimenov created this revision.Jun 12 2023, 5:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2023, 5:25 PM
Herald added a subscriber: Enna1. · View Herald Transcript
kstoimenov requested review of this revision.Jun 12 2023, 5:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2023, 5:25 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Updated comment.

Updated order.

thurston added inline comments.Jun 12 2023, 5:34 PM
compiler-rt/lib/hwasan/hwasan_interceptors.cpp
216

HWASan's mmap interceptor does not tag memory or allow MAP_FIXED with a tagged address, so there should be no need to zero-tag memory in the munmap interceptor. (To the extent that zero-tagging fixes anything, it means there is a bug elsewhere.)

thurston added inline comments.Jun 12 2023, 5:36 PM
compiler-rt/test/hwasan/TestCases/munmap.c
25

What is the purpose of testing that this works? Tagging the pointer and memory for an mmap'ed allocation is not something that should happen in practice.

kstoimenov added inline comments.Jun 13 2023, 9:16 AM
compiler-rt/test/hwasan/TestCases/munmap.c
25

Here is my understanding of what is going on. Feel free to double-check with vitalybuka@. While we investigated a bug we figured out that the area for the stack is mmaped. After that HWASAN tags that memory during execution from instrumented code to detect UAR. What happens after that the process uses vfork to spawn a child process where we get the tag mismatch error. The reason is that the memory is unmmaped in the child process without being untagged and then the next mmap request returns the same memory with tagged shadow memory.

thurston added inline comments.Jun 13 2023, 9:42 AM
compiler-rt/test/hwasan/TestCases/munmap.c
25

Wouldn't it be better to zero-tag the memory in the mmap interceptor, rather than the munmap interceptor? It would mean that un-mmap'ed pages would continue to be protected against illegal accesses, until such time that the memory is actually reused by mmap.

Fixed unmap.

we need the same for Asan, in a separate patch

compiler-rt/lib/hwasan/hwasan_interceptors.cpp
208

Let's do __hwasan::TagMemoryAligned(reinterpret_cast<uptr>(addr), length, 0); here as well

compiler-rt/test/hwasan/TestCases/munmap.c
25

pages are already protected as they are not mapped, so you'll get segv
TagMemoryAligned(...0) will also release unused physical pages

I guess it will not hurt to zero tag in mmap as well.
Reason is that some user code can mmap/munmap avoiding our interceptors, so these two checkpoints to fix them up

30

as this is hwasan test, you can use __hwasan_test_shadow, it's going to be simple:

__hwasan_tag_memory
mmap(fixed)
__hwasan_test_shadow

__hwasan_tag_memory
munmap
__hwasan_test_shadow
kstoimenov marked an inline comment as done.

Updated the test.

kstoimenov marked an inline comment as done.Jun 13 2023, 3:57 PM
kstoimenov added inline comments.
compiler-rt/test/hwasan/TestCases/munmap.c
30

Also added some extra checks. PTAL.

vitalybuka added inline comments.Jun 13 2023, 4:52 PM
compiler-rt/lib/hwasan/hwasan_interceptors.cpp
210

it need to be under
if (res != (void *)-1) {

compiler-rt/test/hwasan/TestCases/munmap.c
43

please add __hwasan_test_shadow(r) just after munmap

43

and then tag again before mmap(r, FIXED

LGTM with fixes above

vitalybuka accepted this revision.Jun 13 2023, 4:52 PM
This revision is now accepted and ready to land.Jun 13 2023, 4:52 PM
kstoimenov marked an inline comment as done.

Address comments.

kstoimenov marked 3 inline comments as done.Jun 13 2023, 5:11 PM
vitalybuka accepted this revision.Jun 13 2023, 7:29 PM

Updated to TagMem.

This revision was landed with ongoing or failed builds.Jun 13 2023, 10:37 PM
This revision was automatically updated to reflect the committed changes.
kstoimenov reopened this revision.Jun 14 2023, 7:25 AM

Breaks the build bot. Will fix forward.

This revision is now accepted and ready to land.Jun 14 2023, 7:25 AM

Switched to unaligned.

kstoimenov closed this revision.Jun 14 2023, 7:54 AM

Will submit as separate patch.