Page MenuHomePhabricator

[HWASAN] Instrument memory intrinsics
ClosedPublic

Authored by evgeny777 on Nov 30 2018, 4:07 AM.

Details

Summary

Patch replaces memory intrinsics with corresponding libc calls when specific option is set. The memset and friends can be either hooked by the runtime or libc itself can be sanitized

The patch lacks test case - I'll implement one if the whole thing makes sense.

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 created this revision.Nov 30 2018, 4:07 AM

Looks fine to me.

kcc added a comment.Dec 6 2018, 5:44 PM

asan replaces memset with __asan_memset (same for memcpy and memmove), see AddressSanitizer::instrumentMemIntrinsic.
Would it be more flexible to use the same approach here?

(and yes, we need to do this, so please proceed with a test)

Would it be more flexible to use the same approach here?

Since r340216 HWASAN stopped intercepting common libc routines. As far as I understand the goal of this change is to sanitize libc.
My understanding is that if we do sanitize the whole libc then we don't need special versions of memset and friends in HWASAN runtime.

Even if one doesn't sanitize libc then it is always possible to hook memset/memcpy/memmove like it was previously done with strcpy and stuff.

evgeny777 updated this revision to Diff 177164.Dec 7 2018, 3:13 AM

Added test case

kcc added a comment.Dec 7 2018, 12:46 PM

We do instrument libc on Android, but

  • memset&co are still not instrumented because they are written in asm (eugenis@, please confirm)
  • hwasan may be useful outside of Android in future. (we actually test it on arm64 linux, and even on x86_64 linux)

So, I still prefer __hwasan_memset (which means that we will also need a run-time test)
eugenis@, thoughts?

test/Instrumentation/HWAddressSanitizer/mem-intrinsics.ll
3 ↗(On Diff #177164)

please place the checks after their respective "call void @llvm..." lines

I think I prefer regular memset to hwasan_memset.
hwasan variants can be slightly faster because they don't need to check that the address is not a shadow address (from stack tagging), but that is probably not a big deal.
Also the hardware implementation will definitely not have (or need) __hwasan_memset.

kcc added a comment.Dec 7 2018, 1:38 PM

How will do do the actual checking then, given the memset is implemented in asm?
extra code in libc?

Yes, I think so.

kcc added a comment.Dec 7 2018, 1:50 PM

ok, let's do it this way, I'll leave it to eugenis@ to stamp.
Also, at some point we'll need to flip the flag.

In fact, I changed my mind - let's do __hwasan_memset.
Tag checking in memset is part of software emulation; as such, it belongs in libclang_rt.hwasan, and not in any system library.

In fact, I changed my mind - let's do __hwasan_memset.

What about kernel compilation mode?

We should do the same thing in kernel mode, I think. Either way memintrinsic checking will require changes on the kernel side. @andreyknvl

evgeny777 updated this revision to Diff 177689.Dec 11 2018, 4:58 AM

Addressed review comments. Now the patch uses approach identical to ASAN.
ASAN doesn't add prefix in kernel compilation mode (my guess is that memset and friends are part of Linux kernel and are always instrumented in such scenario)

See also patch to runtime: D55554

kcc accepted this revision.Dec 11 2018, 3:08 PM

LGTM
Next step is to add run-time tests and enable by default

This revision is now accepted and ready to land.Dec 11 2018, 3:08 PM
This revision was automatically updated to reflect the committed changes.