[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

this is copied from recv, needs to be updated

5354

this var is unused

5357

res > 0

5359

this is different from what msan did
why?

5361

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

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

__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

5360

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

5360

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

5360

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/msan/msan_interceptors.cc

INTERCEPTOR(int, epoll_pwait, int epfd, void *events, int maxevents,
   }
   return res;
 }
 #define MSAN_MAYBE_INTERCEPT_EPOLL_PWAIT INTERCEPT_FUNCTION(epoll_pwait)
 #else
 #define MSAN_MAYBE_INTERCEPT_EPOLL_PWAIT
 #endif
 
-INTERCEPTOR(SSIZE_T, recv, int fd, void *buf, SIZE_T len, int flags) {
-  ENSURE_MSAN_INITED();
-  SSIZE_T res = REAL(recv)(fd, buf, len, flags);
-  if (res > 0)
-    __msan_unpoison(buf, res);
-  return res;
-}
-
-INTERCEPTOR(SSIZE_T, recvfrom, int fd, void *buf, SIZE_T len, int flags,
-            void *srcaddr, int *addrlen) {
-  ENSURE_MSAN_INITED();
-  SIZE_T srcaddr_sz;
-  if (srcaddr) srcaddr_sz = *addrlen;
-  SSIZE_T res = REAL(recvfrom)(fd, buf, len, flags, srcaddr, addrlen);
-  if (res > 0) {
-    __msan_unpoison(buf, res);
-    if (srcaddr) {
-      SIZE_T sz = *addrlen;
-      __msan_unpoison(srcaddr, Min(sz, srcaddr_sz));
-    }
-  }
-  return res;
-}
-
 INTERCEPTOR(void *, calloc, SIZE_T nmemb, SIZE_T size) {
   GET_MALLOC_STACK_TRACE;
   if (UNLIKELY(!msan_inited)) {
     // Hack: dlsym calls calloc before REAL(calloc) is retrieved from dlsym.
     const SIZE_T kCallocPoolSize = 1024;
     static uptr calloc_memory_for_dlsym[kCallocPoolSize];
     static SIZE_T allocated;
     SIZE_T size_in_words = ((nmemb * size) + kWordSize - 1) / kWordSize;
void InitializeInterceptors() {
   INTERCEPT_FUNCTION(getrlimit);
   MSAN_MAYBE_INTERCEPT_GETRLIMIT64;
   MSAN_MAYBE_INTERCEPT_PRLIMIT;
   MSAN_MAYBE_INTERCEPT_PRLIMIT64;
   MSAN_INTERCEPT_UNAME;
   INTERCEPT_FUNCTION(gethostname);
   MSAN_MAYBE_INTERCEPT_EPOLL_WAIT;
   MSAN_MAYBE_INTERCEPT_EPOLL_PWAIT;
-  INTERCEPT_FUNCTION(recv);
-  INTERCEPT_FUNCTION(recvfrom);
   INTERCEPT_FUNCTION(dladdr);
   INTERCEPT_FUNCTION(dlerror);
   INTERCEPT_FUNCTION(dl_iterate_phdr);
   INTERCEPT_FUNCTION(getrusage);
   INTERCEPT_FUNCTION(sigaction);
   INTERCEPT_FUNCTION(signal);
   INTERCEPT_FUNCTION(pthread_create);
   INTERCEPT_FUNCTION(pthread_key_create);

lib/sanitizer_common/sanitizer_common_interceptors.inc

INTERCEPTOR(char *, ctermid_r, char *s) {
   }
   return res;
 }
 #define INIT_CTERMID_R COMMON_INTERCEPT_FUNCTION(ctermid_r);
 #else
 #define INIT_CTERMID_R
 #endif
 
+#if SANITIZER_INTERCEPT_RECV_RECVFROM
+INTERCEPTOR(SSIZE_T, recv, int fd, void *buf, SIZE_T len, int flags) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, recv, fd, buf, len, flags);
+  COMMON_INTERCEPTOR_FD_ACCESS(ctx, fd);
+  SSIZE_T res = REAL(recv)(fd, buf, len, flags);
+  if (res > 0) {
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, len);
+  }
+  if (res >= 0 && fd >= 0) COMMON_INTERCEPTOR_FD_ACQUIRE(ctx, fd);
+  return res;
+}
+
+INTERCEPTOR(SSIZE_T, recvfrom, int fd, void *buf, SIZE_T len, int flags,
+            void *srcaddr, int *addrlen) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, recvfrom, fd, buf, len, flags, srcaddr,
+                           addrlen);
+  COMMON_INTERCEPTOR_FD_ACCESS(ctx, fd);
+  SIZE_T srcaddr_sz;
+  if (srcaddr) srcaddr_sz = *addrlen;
+  SSIZE_T res = REAL(recvfrom)(fd, buf, len, flags, srcaddr, addrlen);
+  if (res > 0) {
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, len);
+    if (srcaddr)
+      COMMON_INTERCEPTOR_INITIALIZE_RANGE(srcaddr,
+                                          Min((SIZE_T)*addrlen, srcaddr_sz));
+  }
+  return res;
+}
+#define INIT_RECV_RECVFROM          \
+  COMMON_INTERCEPT_FUNCTION(recv);  \
+  COMMON_INTERCEPT_FUNCTION(recvfrom);
+#else
+#define INIT_RECV_RECVFROM
+#endif
+
 static void InitializeCommonInterceptors() {
   static u64 metadata_mem[sizeof(MetadataHashMap) / sizeof(u64) + 1];
   interceptor_metadata_map = new((void *)&metadata_mem) MetadataHashMap();
 
   INIT_TEXTDOMAIN;
   INIT_STRCMP;
   INIT_STRNCMP;
   INIT_STRCASECMP;
static void InitializeCommonInterceptors() {
   INIT_MLOCKX;
   INIT_FOPENCOOKIE;
   INIT_SEM;
   INIT_PTHREAD_SETCANCEL;
   INIT_MINCORE;
   INIT_PROCESS_VM_READV;
   INIT_CTERMID;
   INIT_CTERMID_R;
+  INIT_RECV_RECVFROM;
 }

lib/sanitizer_common/sanitizer_platform_interceptors.h

 #define SANITIZER_INTERCEPT_SEM SI_LINUX || SI_FREEBSD
 #define SANITIZER_INTERCEPT_PTHREAD_SETCANCEL SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_MINCORE SI_LINUX
 #define SANITIZER_INTERCEPT_PROCESS_VM_READV SI_LINUX
 #define SANITIZER_INTERCEPT_CTERMID SI_LINUX || SI_MAC || SI_FREEBSD
 #define SANITIZER_INTERCEPT_CTERMID_R SI_MAC || SI_FREEBSD
 
 #define SANITIZER_INTERCEPTOR_HOOKS SI_LINUX
+#define SANITIZER_INTERCEPT_RECV_RECVFROM 1
 
 #endif  // #ifndef SANITIZER_PLATFORM_INTERCEPTORS_H

lib/tsan/rtl/tsan_interceptors.cc

TSAN_INTERCEPTOR(long_t, sendmsg, int fd, void *msg, int flags) {
   if (fd >= 0) {
     FdAccess(thr, pc, fd);
     FdRelease(thr, pc, fd);
   }
   int res = REAL(sendmsg)(fd, msg, flags);
   return res;
 }
 
-TSAN_INTERCEPTOR(long_t, recv, int fd, void *buf, long_t len, int flags) {
-  SCOPED_TSAN_INTERCEPTOR(recv, fd, buf, len, flags);
-  if (fd >= 0)
-    FdAccess(thr, pc, fd);
-  int res = REAL(recv)(fd, buf, len, flags);
-  if (res >= 0 && fd >= 0) {
-    FdAcquire(thr, pc, fd);
-  }
-  return res;
-}
-
 TSAN_INTERCEPTOR(int, unlink, char *path) {
   SCOPED_TSAN_INTERCEPTOR(unlink, path);
   Release(thr, pc, File2addr(path));
   int res = REAL(unlink)(path);
   return res;
 }
 
 TSAN_INTERCEPTOR(void*, tmpfile, int fake) {
#endif
   TSAN_INTERCEPT(close);
   TSAN_MAYBE_INTERCEPT___CLOSE;
   TSAN_MAYBE_INTERCEPT___RES_ICLOSE;
   TSAN_INTERCEPT(pipe);
   TSAN_INTERCEPT(pipe2);
 
   TSAN_INTERCEPT(send);
   TSAN_INTERCEPT(sendmsg);
-  TSAN_INTERCEPT(recv);
 
   TSAN_INTERCEPT(unlink);
   TSAN_INTERCEPT(tmpfile);
   TSAN_MAYBE_INTERCEPT_TMPFILE64;
   TSAN_INTERCEPT(fread);
   TSAN_INTERCEPT(fwrite);
   TSAN_INTERCEPT(abort);
   TSAN_INTERCEPT(puts);

test/asan/TestCases/Linux/recvfrom.cc

+// Test that ASan detects buffer overflow on read from socket via recvfrom.
+//
+// RUN: %clangxx_asan -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O1 %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O2 %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O3 %s -o %t && not %run %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <netdb.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <pthread.h>
+
+const int kPortNum = 1234;
+const int kBufSize = 10;
+
+static void *server_thread_udp(void *data) {
+  char buf[kBufSize / 2];
+  struct sockaddr_in serveraddr; // server's addr
+  int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
+  if (sockfd < 0)
+    fprintf(stderr, "ERROR opening socket\n");
+
+  memset((char *) &serveraddr, 0, sizeof(serveraddr));
+  serveraddr.sin_family = AF_INET;
+  serveraddr.sin_addr.s_addr = htonl(INADDR_ANY);
+  serveraddr.sin_port = htons(kPortNum);
+
+  if (bind(sockfd, (struct sockaddr *) &serveraddr, sizeof(serveraddr)) < 0)
+    fprintf(stderr, "ERROR on binding\n");
+
+  recvfrom(sockfd, buf, kBufSize, 0, NULL, NULL); // BOOM
+  // CHECK: {{WRITE of size 10 at 0x.* thread T1}}
+  // CHECK: {{    #1 0x.* in server_thread_udp.*recvfrom.cc:}}[[@LINE-2]]
+  // CHECK: {{Address 0x.* is located in stack of thread T1 at offset}}
+  // CHECK-NEXT: in{{.*}}server_thread_udp{{.*}}recvfrom.cc
+  return NULL;
+}
+
+int main() {
+  char buf[kBufSize] = "123456789";
+  struct sockaddr_in serveraddr; // server's addr
+
+  pthread_t server_thread;
+  if (pthread_create(&server_thread, NULL, server_thread_udp, NULL)) {
+    fprintf(stderr, "Error creating thread\n");
+    exit(1);
+  }
+
+  int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
+  struct hostent *server;
+  char hostname[] = "localhost";
+  if (sockfd < 0)
+    fprintf(stderr, "ERROR opening socket\n");
+
+  server = gethostbyname(hostname);
+  if (!server) {
+    fprintf(stderr,"ERROR, no such host as %s\n", hostname);
+    exit(1);
+  }
+
+  memset((char *) &serveraddr, 0, sizeof(serveraddr));
+  serveraddr.sin_family = AF_INET;
+  memcpy((char *)&serveraddr.sin_addr.s_addr, (char *)server->h_addr,
+          server->h_length);
+  serveraddr.sin_port = htons(kPortNum);
+  sendto(sockfd, buf, strlen(buf), 0, (struct sockaddr *) &serveraddr,
+         sizeof(serveraddr));
+
+  if (pthread_join(server_thread, NULL)) {
+    fprintf(stderr, "Error joining thread\n");
+    exit(1);
+  }
+
+  return 0;
+}