On Wed, Oct 11, 2017 at 07:55:44AM +0200, Martin Liška wrote:
> > Conceptually, these two instrumentations rely on address sanitization,
> > not really sure if we should supporting them for kernel sanitization (but I
> > bet it is just going to be too costly for kernel).
> > So, we also need to make sure at least parts of SANITIZE_ADDRESS is enabled
> > when these options are on.
> > That can be done by erroring out if -fsanitize=pointer-compare is requested
> > without -fsanitize=address, or by implicitly enabling -fsanitize=address for
> > these, or by adding yet another SANITIZE_* bit which would cover
> > sanitization of memory accesses for asan, that bit would be set by
> > -fsanitize={address,kernel-address} in addition to the current 2 bits, but
> > pointer-{subtract,compare} would set its own bit and SANITIZE_ADDRESS and
> > SANITIZE_USER_ADDRESS only. Without the new bit we'd emit red zones,
> > function prologue/epilogue asan changes, registraction of global variables,
> > but not actual instrumentation of memory accesses (and probably not
> > instrumentation of C++ ctor ordering).
>
> Agree, would be much easier to just enable SANITIZE_ADDRESS with there 2
> options.
> Question is how to make it also possible with -fsanitize=kernel-address:
>
> $ ./xgcc -B.
> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pointer-compare-2.c
> -fsanitize=pointer-compare,kernel-address
> cc1: error: ‘-fsanitize=address’ is incompatible with
> ‘-fsanitize=kernel-address’
>
> Ideas?
If we want to make it usable for both user and kernel address, then either
we'll let pointer-compare/pointer-subtract implicitly enable user address,
unless kernel-address has been enabled (that would mean set SANITIZE_ADDRESS
bit in pointer-*, but not SANITIZE_USER_ADDRESS, and at the point where we
diagnose option incompatibilities like -fsanitize=address,kernel-address
check for the case (SANITIZE_ADDRESS bit set, none of SANITIZE_USER_ADDRESS
nor SANITIZE_KERNEL_ADDRESS, and one of SANITIZE_POINTER_*) and set
implicitly SANITIZE_USER_ADDRESS, or simply require that the user chooses,
by erroring out if pointer-* is used without explicit address or
kernel-address. In any case, I think this should be also something
discussed with the upstream sanitizer folks, so that LLVM (if it ever
decides to actually implement it) behaves compatibly.
> --- a/libsanitizer/asan/asan_descriptions.cc
> +++ b/libsanitizer/asan/asan_descriptions.cc
> @@ -220,6 +220,15 @@ bool GetStackAddressInformation(uptr addr, uptr
> access_size,
> return true;
> }
>
> +bool GetStackVariableBeginning(uptr addr, uptr *shadow_addr)
> +{
> + AsanThread *t = FindThreadByStackAddress(addr);
> + if (!t) return false;
> +
> + *shadow_addr = t->GetStackFrameVariableBeginning (addr);
> + return true;
> +}
> +
> static void PrintAccessAndVarIntersection(const StackVarDescr &var, uptr
> addr,
> uptr access_size, uptr
> prev_var_end,
> uptr next_var_beg) {
> diff --git a/libsanitizer/asan/asan_descriptions.h
> b/libsanitizer/asan/asan_descriptions.h
> index 584b9ba6491..b7f23b1a71b 100644
> --- a/libsanitizer/asan/asan_descriptions.h
> +++ b/libsanitizer/asan/asan_descriptions.h
> @@ -138,6 +138,7 @@ struct StackAddressDescription {
>
> bool GetStackAddressInformation(uptr addr, uptr access_size,
> StackAddressDescription *descr);
> +bool GetStackVariableBeginning(uptr addr, uptr *shadow_addr);
>
> struct GlobalAddressDescription {
> uptr addr;
> diff --git a/libsanitizer/asan/asan_globals.cc
> b/libsanitizer/asan/asan_globals.cc
> index f2292926e6a..ed707c0ca01 100644
> --- a/libsanitizer/asan/asan_globals.cc
> +++ b/libsanitizer/asan/asan_globals.cc
> @@ -122,6 +122,31 @@ int GetGlobalsForAddress(uptr addr, Global *globals, u32
> *reg_sites,
> return res;
> }
>
> +bool AreGlobalVariablesSame(uptr addr1, uptr addr2)
> +{
> + if (addr1 > addr2)
> + {
> + uptr tmp = addr1;
> + addr1 = addr2;
> + addr2 = tmp;
std::swap(addr1, addr2); ? I don't see it used in any of libsanitizer
though, so not sure if the corresponding STL header is included.
> + }
> +
> + BlockingMutexLock lock(&mu_for_globals);
Why do you need a mutex for checking if there are no red zones in between?
> + uptr aligned_addr1 = addr1 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr.
> + uptr aligned_addr2 = addr2 & ~(SANITIZER_WORDSIZE/8 - 1); // align addr.
> +
> + u8 *shadow_ptr1 = (u8*)MemToShadow(aligned_addr1);
> + u8 *shadow_ptr2 = (u8*)MemToShadow(aligned_addr2);
> +
> + while (shadow_ptr1 <= shadow_ptr2
> + && *shadow_ptr1 != kAsanGlobalRedzoneMagic) {
> + shadow_ptr1++;
> + }
There are many kinds of shadow memory markings. My thought was that it
would start with a quick check, perhaps vectorized by hand (depending on if
the arch has unaligned loads maybe without or with a short loop for
alignment) where say unsigned long (perhaps may_alias?) pointer would be
used to read 4/8 shadow bytes at a time, and just check if any of them is
non-zero. And, if it found a non-zero byte, deal with it after the loop,
if after the "vectorized" loop, find which byte it was, and in any case
deal e.g. with the various special cases like when the shadow byte is 1-7
and stands for how many bytes are accessible, etc.
> --- a/libsanitizer/asan/asan_report.cc
> +++ b/libsanitizer/asan/asan_report.cc
> @@ -344,14 +344,52 @@ static INLINE void CheckForInvalidPointerPair(void *p1,
> void *p2) {
> if (!flags()->detect_invalid_pointer_pairs) return;
> uptr a1 = reinterpret_cast<uptr>(p1);
> uptr a2 = reinterpret_cast<uptr>(p2);
> - AsanChunkView chunk1 = FindHeapChunkByAddress(a1);
> - AsanChunkView chunk2 = FindHeapChunkByAddress(a2);
> - bool valid1 = chunk1.IsAllocated();
> - bool valid2 = chunk2.IsAllocated();
> - if (!valid1 || !valid2 || !chunk1.Eq(chunk2)) {
> - GET_CALLER_PC_BP_SP;
> - return ReportInvalidPointerPair(pc, bp, sp, a1, a2);
> +
> + if (a1 == a2)
> + return;
My thought was that you'd do the difference <= 2048 test first
without any lock, and only for the larger stuff take locks and look stuff
up.
> +
> + uptr shadow_offset1, shadow_offset2;
> + bool valid1, valid2;
> + {
> + ThreadRegistryLock l(&asanThreadRegistry());
> +
> + valid1 = GetStackVariableBeginning(a1, &shadow_offset1);
> + valid2 = GetStackVariableBeginning(a2, &shadow_offset2);
> + }
> +
> + if (valid1 && valid2) {
> + if (shadow_offset1 == shadow_offset2)
> + return;
> }
> + else if (!valid1 && !valid2) {
> + AsanChunkView chunk1 = FindHeapChunkByAddress(a1);
> + AsanChunkView chunk2 = FindHeapChunkByAddress(a2);
> + valid1 = chunk1.IsAllocated();
> + valid2 = chunk2.IsAllocated();
> +
> + if (valid1 && valid2) {
> + if (chunk1.Eq(chunk2))
> + return;
> + }
> + else if (!valid1 && !valid2) {
> + uptr offset = a1 < a2 ? a2 - a1 : a1 - a2;
> + if (offset <= 2048) {
> + if (AreGlobalVariablesSame (a1, a2))
> + return;
> + }
> +
> + GlobalAddressDescription gdesc1, gdesc2;
> + valid1 = GetGlobalAddressInformation(a1, 1, &gdesc1);
> + valid2 = GetGlobalAddressInformation(a2, 1, &gdesc2);
> +
> + if (valid1 && valid2
> + && gdesc1.globals[0].beg == gdesc2.globals[0].beg)
> + return;
> + }
> + }
> +
> + GET_CALLER_PC_BP_SP;
> + ReportInvalidPointerPair(pc, bp, sp, a1, a2);
> }
> // ----------------------- Mac-specific reports ----------------- {{{1
>
> diff --git a/libsanitizer/asan/asan_report.h b/libsanitizer/asan/asan_report.h
> index 111b8400153..2b4c9d29fda 100644
> --- a/libsanitizer/asan/asan_report.h
> +++ b/libsanitizer/asan/asan_report.h
> @@ -27,6 +27,7 @@ struct StackVarDescr {
> // them to "globals" array.
> int GetGlobalsForAddress(uptr addr, __asan_global *globals, u32 *reg_sites,
> int max_globals);
> +bool AreGlobalVariablesSame(uptr addr1, uptr addr2);
>
> const char *MaybeDemangleGlobalName(const char *name);
> void PrintGlobalNameIfASCII(InternalScopedString *str, const __asan_global
> &g);
> diff --git a/libsanitizer/asan/asan_thread.cc
> b/libsanitizer/asan/asan_thread.cc
> index 818e1261400..d6bd051a493 100644
> --- a/libsanitizer/asan/asan_thread.cc
> +++ b/libsanitizer/asan/asan_thread.cc
> @@ -322,6 +322,29 @@ bool AsanThread::GetStackFrameAccessByAddr(uptr addr,
> return true;
> }
>
> +uptr AsanThread::GetStackFrameVariableBeginning(uptr addr)
> +{
> + uptr bottom = 0;
> + if (AddrIsInStack(addr)) {
> + bottom = stack_bottom();
> + } else if (has_fake_stack()) {
> + bottom = fake_stack()->AddrIsInFakeStack(addr);
> + CHECK(bottom);
> + }
> + uptr aligned_addr = addr & ~(SANITIZER_WORDSIZE/8 - 1); // align addr.
> + u8 *shadow_ptr = (u8*)MemToShadow(aligned_addr);
> + u8 *shadow_bottom = (u8*)MemToShadow(bottom);
> +
> + while (shadow_ptr >= shadow_bottom &&
> + (*shadow_ptr != kAsanStackLeftRedzoneMagic
> + && *shadow_ptr != kAsanStackMidRedzoneMagic
> + && *shadow_ptr != kAsanStackRightRedzoneMagic)) {
> + shadow_ptr--;
> + }
> +
> + return (uptr)shadow_ptr;
> +}
> +
> bool AsanThread::AddrIsInStack(uptr addr) {
> const auto bounds = GetStackBounds();
> return addr >= bounds.bottom && addr < bounds.top;
> diff --git a/libsanitizer/asan/asan_thread.h b/libsanitizer/asan/asan_thread.h
> index c51a58ad0bb..c5adecacad4 100644
> --- a/libsanitizer/asan/asan_thread.h
> +++ b/libsanitizer/asan/asan_thread.h
> @@ -80,6 +80,7 @@ class AsanThread {
> const char *frame_descr;
> };
> bool GetStackFrameAccessByAddr(uptr addr, StackFrameAccess *access);
> + uptr GetStackFrameVariableBeginning(uptr addr);
>
> bool AddrIsInStack(uptr addr);
>
> --
> 2.14.2
>
Jakub
