Page MenuHomePhabricator

[MSan] another take at instrumenting inline assembly - now with calls
AcceptedPublic

Authored by glider on Oct 29 2018, 5:01 AM.

Details

Summary

Turns out it's not always possible to figure out whether an asm() statement argument points to a valid memory region.
One example would be per-CPU objects in the Linux kernel, for which the addresses are calculated using the FS register and a small offset in the .data..percpu section.
To avoid pulling all sorts of checks into the instrumentation, we replace actual checking/unpoisoning code with calls to msan_instrument_asm_load(ptr, size) and msan_instrument_asm_store(ptr, size) functions in the runtime.

This patch doesn't implement the runtime hooks in compiler-rt, as there's been no demand in assembly instrumentation for userspace apps so far.

Diff Detail

Event Timeline

glider created this revision.Oct 29 2018, 5:01 AM

Otherwise looks good to me.

lib/Transforms/Instrumentation/MemorySanitizer.cpp
3554

After offline discussion, it's better to move stores _before_ the asm call because the asm call can publish some memory to other threads (also consistent with the way we instrument atomic ops).
But loads should precede stores because some args can be in/out.
After we move stores to before the call, we could combine the 2 loops, but it's unclear if it's a win code-wise or not.

glider updated this revision to Diff 171498.Oct 29 2018, 7:34 AM

Moved __msan_instrument_asm_store() before the asm call.

eugenis added inline comments.Oct 29 2018, 1:11 PM
lib/Transforms/Instrumentation/MemorySanitizer.cpp
3118

Why test for InsertCheck here?
It is done in insertShadowCheck already, and unpoisoning of stores should be done even in blacklisted functions.

glider added inline comments.Oct 30 2018, 2:45 AM
lib/Transforms/Instrumentation/MemorySanitizer.cpp
3118

Fair enough, will remove.

glider updated this revision to Diff 171649.Oct 30 2018, 2:46 AM

Removed the check for InsertChecks.

eugenis accepted this revision.Oct 30 2018, 3:54 PM

LGTM

This revision is now accepted and ready to land.Oct 30 2018, 3:54 PM

Landed r345702, thank you!