[sanitizer] Move recvmsg and recv interceptors to sanitizer_common.

[m.ostapenko]

Summary

This patch moves recv and recvfrom interceptors to sanitizer_common to enable them in ASan. This those, we would be able to catch CVE-2015-7547 with dynamic stack buffer overflow (https://groups.google.com/forum/#!topic/address-sanitizer/ttPxNhTK9TU).

[kcc]

I think this should go into sanitizer_common, to also cover tsan.
(msan has one, so merge msan's one into sanitizer_common).
also, would be nice to have a test in test/asan/TestCases/Linux

[m.ostapenko]

OK, let's try more. Moved recvfrom and recv interceptors from TSan and MSan to sanitizer_common. I'm adding Dmitry to reviewers because this change affects TSan. The testcase for ASan may look scary at first glance, but actually it's quite simple:

1. Create simple UDP server in thread 1 and wait for a message. The size of allocated destination buffer is less then expected message length.
2. Create simple UDP client in thread 0 and send the message.
3. Crash in recvfrom function due to buffer overflow.

[dvyukov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

5352 ↗

(On Diff #48907)

this is copied from recv, needs to be updated

5354 ↗

(On Diff #48907)

this var is unused

5357 ↗

(On Diff #48907)

res > 0

5359 ↗

(On Diff #48907)

this is different from what msan did
why?

5361 ↗

(On Diff #48907)

drop this
we add such synchronization lazily when somebody needs it, otherwise we will need to synchronize too much

[m.ostapenko]

lib/sanitizer_common/sanitizer_common_interceptors.inc

5359 ↗

(On Diff #48907)

You are right. This is msan specific code, so perhaps we could wrap the whole if to macro (say, COMMON_INTERCEPTOR_UNPOISON_COND) and define it like this:

#define COMMON_INTERCEPTOR_UNPOISON_COND(srcaddr, sz, srcaddr_sz)  \
  do {                                                             \
    if (srcaddr) {                                                 \
      SIZE_T sz = *addrlen;                                        \
        __msan_unpoison(srcaddr, Min(sz, srcaddr_sz));             \
    }                                                              \
  } while (false)

for msan and empty macros for others (just like COMMON_INTERCEPTOR_UNPOISON_PARAM)?

[dvyukov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

5359 ↗

(On Diff #48907)

__msan_unpoison(srcaddr, Min(sz, *addrlen)) looks simpler to me

[m.ostapenko]

Dmitry, thank you for your nits. Updating the diff.

[dvyukov]

Otherwise looks good.

lib/sanitizer_common/sanitizer_common_interceptors.inc

5364 ↗

(On Diff #48944)

I would just do:

if (srcaddr)
    __msan_unpoison(srcaddr, Min(srcaddr_sz, *addrlen));

The macro makes code more convoluted without adding significant value. Every time I read this function I will need to go and check macro definition.

[m.ostapenko]

lib/sanitizer_common/sanitizer_common_interceptors.inc

5364 ↗

(On Diff #48944)

Every time I read this function I will need to go and check macro definition.

Agree here, but wouldn't this lead to use of undeclared identifier '__msan_unpoison' errors in {a, t}san_interceptors.cc? I used the macro just to avoid such issues. Perhaps I can avoid introducing a new macro and write something like this:

if (srcaddr)
  COMMON_INTERCEPTOR_INITIALIZE_RANGE(srcaddr, Min(srcaddr_sz, (SIZE_T)*addrlen));

[dvyukov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

5364 ↗

(On Diff #48944)

Right.
Don't we already have it? I would expect that we should have something like this already. We already have COMMON_INTERCEPTOR_INITIALIZE_RANGE, maybe that's what you are looking for?
If we don't have it yet, then yes, introduce such macro.

[m.ostapenko]

Use COMMON_INTERCEPTOR_INITIALIZE_RANGE instead of introducing new macro for MSan.

[dvyukov]

LGTM
Thanks!

[m.ostapenko]

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc

5343 ↗

(On Diff #49020)

I've just noticed, that here and below in recvfrom we should pass res as third parameter, not len. Should I create a new revision on Phabricator?

5359 ↗

(On Diff #49020)

And here.

[dvyukov]

Yes, please.

[m.ostapenko]

I've also disabled recvfrom.cc testcase (http://reviews.llvm.org/rL261870) for now due to buildbot failure:

FAIL: AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc (524 of 1166)
******************** TEST 'AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m32 /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc -o /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp && not  /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp 2>&1 | FileCheck /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc
--
Exit Code: 2

Command Output (stderr):
--
FileCheck error: '-' is empty.

--

[eugenis]

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc

5343 ↗

(On Diff #49020)

I think you can submit such an obvious fix directly.

[m.ostapenko]

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc

5343 ↗

(On Diff #49020)

Thanks, I've already done it (approved by Dmitry): http://reviews.llvm.org/D17608.

lib/asan/asan_interceptors.cc

#define COMMON_SYSCALL_POST_READ_RANGE(p, s) \
   } while (false)
 #define COMMON_SYSCALL_POST_WRITE_RANGE(p, s) \
   do {                                        \
     (void)(p);                                \
     (void)(s);                                \
   } while (false)
 #include "sanitizer_common/sanitizer_common_syscalls.inc"
 
+
+INTERCEPTOR(SSIZE_T, recv, int fd, void *buf, SIZE_T len, int flags) {
+  ENSURE_ASAN_INITED();
+  ASAN_READ_RANGE(nullptr, buf, len);
+  SSIZE_T res = REAL(recv)(fd, buf, len, flags);
+  return res;
+}
+
+INTERCEPTOR(SSIZE_T, recvfrom, int fd, void *buf, SIZE_T len, int flags,
+            void *srcaddr, int *addrlen) {
+  ENSURE_ASAN_INITED();
+  SIZE_T srcaddr_sz;
+  if (srcaddr) srcaddr_sz = *addrlen;
+  ASAN_READ_RANGE(nullptr, buf, len);
+  SSIZE_T res = REAL(recvfrom)(fd, buf, len, flags, srcaddr, addrlen);
+  return res;
+}
+
 struct ThreadStartParam {
   atomic_uintptr_t t;
   atomic_uintptr_t is_registered;
 };
 
 static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
   ThreadStartParam *param = reinterpret_cast<ThreadStartParam *>(arg);
   AsanThread *t = nullptr;

 #if ASAN_INTERCEPT___CXA_ATEXIT
   ASAN_INTERCEPT_FUNC(__cxa_atexit);
 #endif
 
 #if ASAN_INTERCEPT_FORK
   ASAN_INTERCEPT_FUNC(fork);
 #endif
 
+  // Inrecept recv functions.
+  ASAN_INTERCEPT_FUNC(recv);
+  ASAN_INTERCEPT_FUNC(recvfrom);
+
   InitializePlatformInterceptors();
 
   VReport(1, "AddressSanitizer: libc interceptors initialized\n");
 }
 
 } // namespace __asan