[Libunwind-devel] [PATCH] x86_64: fix mincore_validate and msync_validate

[Dave Watson]

The calls to mincore() or msync() are not checking for actual accessibility
this could lead to SIGSEGV if the address from a mapped page with the
PROT_NONE property occurs on the stack.
Hence an attempt to write one byte from the checked address to a pipe will
fail if the address is not readable.

V2 -> V3:

Use a nonblocking pipe to avoid setting a global SIGPIPE handler, and also
handle EINTR

Add a test

Also adds for x86 code

V1 -> V2:

Attempt to handle SIGPIPE, use CLOEXEC

https://github.com/libunwind/libunwind/pull/7

V1:

http://lists.nongnu.org/archive/html/libunwind-devel/2016-09/msg00001.html
---
 src/x86/Gglobal.c          |   2 +
 src/x86/Ginit.c            |  47 ++++++++++++++-
 src/x86_64/Ginit.c         |  50 +++++++++++++++-
 tests/Ltest-mem-validate.c | 143 +++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.am          |   4 +-
 5 files changed, 242 insertions(+), 4 deletions(-)
 create mode 100644 tests/Ltest-mem-validate.c

diff --git a/src/x86/Gglobal.c b/src/x86/Gglobal.c
index 132b8249..3625b5b5 100644
--- a/src/x86/Gglobal.c
+++ b/src/x86/Gglobal.c
@@ -57,6 +57,8 @@ tdep_init (void)
 
     dwarf_init ();
 
+    init_mem_validate ();
+
 #ifndef UNW_REMOTE_ONLY
     x86_local_addr_space_init ();
 #endif
diff --git a/src/x86/Ginit.c b/src/x86/Ginit.c
index 876990fe..01e5e410 100644
--- a/src/x86/Ginit.c
+++ b/src/x86/Ginit.c
@@ -78,6 +78,45 @@ get_dyn_info_list_addr (unw_addr_space_t as, unw_word_t 
*dyn_info_list_addr,
 #define PAGE_SIZE 4096
 #define PAGE_START(a)   ((a) & ~(PAGE_SIZE-1))
 
+static int validate_pipe[2];
+
+static void
+open_pipe (int needclose)
+{
+  if (needclose)
+    {
+      close(validate_pipe[1]);
+      close(validate_pipe[0]);
+    }
+
+  pipe2(validate_pipe, O_CLOEXEC | O_NONBLOCK);
+}
+
+ALWAYS_INLINE
+static int
+write_validate (void *addr)
+{
+  int i;
+  for (i = 0; i < 2; i++)
+    {
+      char buf;
+      int bytes = read (validate_pipe[0], &buf, 1);
+      if (bytes >= 0 || errno == EAGAIN || errno == EWOULDBLOCK)
+        {
+
+          bytes = write(validate_pipe[1], addr, 1);
+          if (bytes == 1)
+            {
+              return 0;
+            }
+        }
+
+      open_pipe(1);
+    }
+
+  return -1;
+}
+
 /* Cache of already validated addresses */
 #define NLGA 4
 static unw_word_t last_good_addr[NLGA];
@@ -129,7 +168,13 @@ validate_mem (unw_word_t addr)
   victim = (victim + 1) % NLGA;
   lga_victim = victim;
 
-  return 0;
+  return write_validate (addr);
+}
+
+HIDDEN void
+tdep_init_mem_validate (void)
+{
+  open_pipe(0);
 }
 
 static int
diff --git a/src/x86_64/Ginit.c b/src/x86_64/Ginit.c
index 2bb238e1..e7df19ef 100644
--- a/src/x86_64/Ginit.c
+++ b/src/x86_64/Ginit.c
@@ -72,10 +72,54 @@ get_dyn_info_list_addr (unw_addr_space_t as, unw_word_t 
*dyn_info_list_addr,
 #define PAGE_SIZE 4096
 #define PAGE_START(a)   ((a) & ~(PAGE_SIZE-1))
 
+static int validate_pipe[2];
+
+static void
+open_pipe (int needclose)
+{
+  if (needclose)
+    {
+      close(validate_pipe[1]);
+      close(validate_pipe[0]);
+    }
+
+  pipe2(validate_pipe, O_CLOEXEC | O_NONBLOCK);
+}
+
+ALWAYS_INLINE
+static int
+write_validate (void *addr)
+{
+  int i;
+  for (i = 0; i < 2; i++)
+    {
+      char buf;
+      int bytes = read (validate_pipe[0], &buf, 1);
+      if (bytes >= 0 || errno == EAGAIN || errno == EWOULDBLOCK)
+        {
+
+          bytes = write(validate_pipe[1], addr, 1);
+          if (bytes == 1)
+            {
+              return 0;
+            }
+        }
+
+      open_pipe(1);
+    }
+
+  return -1;
+}
+
 static int (*mem_validate_func) (void *addr, size_t len);
 static int msync_validate (void *addr, size_t len)
 {
-  return msync (addr, len, MS_ASYNC);
+  if (msync (addr, len, MS_ASYNC) != 0)
+    {
+      return -1;
+    }
+
+  return write_validate (addr);
 }
 
 #ifdef HAVE_MINCORE
@@ -96,7 +140,7 @@ static int mincore_validate (void *addr, size_t len)
       if (!(mvec[i] & 1)) return -1;
     }
 
-  return 0;
+  return write_validate (addr);
 }
 #endif
 
@@ -125,6 +169,8 @@ tdep_init_mem_validate (void)
       Debug(1, "using msync to validate memory\n");
       mem_validate_func = msync_validate;
     }
+
+  open_pipe(0);
 }
 
 /* Cache of already validated addresses */
diff --git a/tests/Ltest-mem-validate.c b/tests/Ltest-mem-validate.c
new file mode 100644
index 00000000..1cacb9f0
--- /dev/null
+++ b/tests/Ltest-mem-validate.c
@@ -0,0 +1,143 @@
+/* libunwind - a platform-independent unwind library
+   Copyright (C) 2003-2004 Hewlett-Packard Co
+       Contributed by David Mosberger-Tang <address@hidden>
+
+This file is part of libunwind.
+
+Copyright (c) 2003 Hewlett-Packard Co.
+
+Permission is hereby granted, free of charge, to any person obtaining
+a copy of this software and associated documentation files (the
+"Software"), to deal in the Software without restriction, including
+without limitation the rights to use, copy, modify, merge, publish,
+distribute, sublicense, and/or sell copies of the Software, and to
+permit persons to whom the Software is furnished to do so, subject to
+the following conditions:
+
+The above copyright notice and this permission notice shall be
+included in all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.  */
+
+#include "compiler.h"
+
+#include <libunwind.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <sys/resource.h>
+#include <sys/mman.h>
+
+#define panic(args...)                         \
+       { fprintf (stderr, args); exit (-1); }
+
+void * stack_start;
+
+#define PAGE_SIZE 4096
+
+void do_backtrace (void)
+{
+  void* buffer[1024];
+  int size = 1024;
+  mprotect((void*)((uintptr_t)stack_start & ~(PAGE_SIZE - 1)),
+           PAGE_SIZE, PROT_NONE);
+
+  unw_cursor_t cursor;
+  unw_word_t ip, sp;
+  unw_context_t uc;
+  int ret;
+  int steps = 0;
+
+  unw_getcontext (&uc);
+  if (unw_init_local (&cursor, &uc) < 0)
+    panic ("unw_init_local failed!\n");
+
+  do
+    {
+      unw_get_reg (&cursor, UNW_REG_IP, &ip);
+      unw_get_reg (&cursor, UNW_REG_SP, &sp);
+
+      ret = unw_step (&cursor);
+      if (ret < 0)
+       {
+         unw_get_reg (&cursor, UNW_REG_IP, &ip);
+       }
+      steps ++;
+    }
+  while (ret > 0);
+
+  if (steps < 5)
+    {
+      exit(-1);
+    }
+
+  mprotect((void*)((uintptr_t)stack_start & ~(PAGE_SIZE - 1)),
+           PAGE_SIZE, PROT_READ|PROT_WRITE);
+}
+
+void consume_and_run (int depth)
+{
+  unw_cursor_t cursor;
+  unw_context_t uc;
+  char string[1024];
+
+  sprintf (string, "hello %p %p\n", &cursor, &uc);
+  if (depth == 0) {
+    do_backtrace();
+  } else {
+    consume_and_run(depth - 1);
+  }
+}
+
+int
+main (int argc, char **argv UNUSED)
+{
+  int start;
+  unw_context_t uc;
+  unw_cursor_t cursor;
+
+  stack_start = &start;
+
+  // Initialize pipe mem validate check, opens file descriptors
+  unw_getcontext(&uc);
+  if (unw_init_local (&cursor, &uc) < 0)
+    panic ("unw_init_local failed!\n");
+
+  int i;
+  for (i = 3; i < 10; i++)
+    {
+
+      pid_t childpid = fork();
+      if (!childpid)
+        {
+          /* Close fds and make sure we still work */
+          int ret = close(i);
+        }
+
+      int status;
+      if (childpid)
+        {
+          wait(&status);
+          if (WIFEXITED(status))
+              return WEXITSTATUS(status);
+          else
+            return -1;
+        }
+      else
+        {
+          consume_and_run (10);
+
+          return 0;
+        }
+    }
+
+  return 0;
+}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c191c6bc..5d39a9e3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -45,7 +45,7 @@ endif #!ARCH_IA64
                        Gtest-resume-sig Ltest-resume-sig                \
                        Gtest-resume-sig-rt Ltest-resume-sig-rt          \
                        Gtest-trace Ltest-trace                          \
-                       Ltest-init-local-signal                          \
+                       Ltest-init-local-signal Ltest-mem-validate       \
                        test-async-sig test-flush-cache test-init-remote \
                        test-mem test-reg-state Ltest-varargs            \
                        Ltest-nomalloc Ltest-nocalloc Lrs-race
@@ -143,6 +143,7 @@ Ltest_init_SOURCES = Ltest-init.cxx
 Ltest_cxx_exceptions_SOURCES = Ltest-cxx-exceptions.cxx
 
 Ltest_init_local_signal_SOURCES = Ltest-init-local-signal.c 
Ltest-init-local-signal-lib.c
+Ltest_mem_validate_SOURCES = Ltest-mem-validate.c
 
 Gtest_dyn1_SOURCES = Gtest-dyn1.c flush-cache.S flush-cache.h
 Ltest_dyn1_SOURCES = Ltest-dyn1.c flush-cache.S flush-cache.h
@@ -186,6 +187,7 @@ test_strerror_LDADD = $(LIBUNWIND)
 Lrs_race_LDADD = $(LIBUNWIND_local) -lpthread
 Ltest_varargs_LDADD = $(LIBUNWIND_local)
 Ltest_init_local_signal_LDADD = $(LIBUNWIND) $(LIBUNWIND_local)
+Ltest_mem_validate_LDADD = $(LIBUNWIND) $(LIBUNWIND_local)
 
 Gtest_bt_LDADD = $(LIBUNWIND) $(LIBUNWIND_local)
 Gtest_concurrent_LDADD = $(LIBUNWIND) $(LIBUNWIND_local) -lpthread
-- 
2.13.5