[RFC][PR 67336][PING^2] Verify pointers during stack unwind

[RFC][PR 67336][PING^2] Verify pointers during stack unwind

[Yuri Gribov]

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]

Hi all,

Libgcc unwinder currently does not do any verification of pointers
which it chases on stack. In practice this not so rarely causes
segfaults when unwinding on corrupted stacks (e.g. when when trying to
print diagnostic on
fatal error) [1]. Ironically this usually happens in error reporting
code which puzzles users.

I've attached one motivating example - with current libgcc it will
abort inside unwinder when trying to access invalid address
(0x0a0a0a0a).

There is an old request to provide a safer version of
_Unwind_Backtrace (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67336)
that would check pointer validity prior to dereferencing. I've
attached a draft patch to see if this approach is viable and hopefully
get some advice.

The change is rather straightforward: I add a new
_Unwind_Backtrace_Checked which checks return value of msync on every
potentially unsafe access (libunwind does something like this as well,
although in a very incomplete manner).
To avoid paying for syscalls too often, I cache the last checked
memory page. Potentially parsing /proc/$$/maps would allow for much
faster verification but I didn't bother too much as new APIs are
intended for reporting fatal errors where speed isn't an issue.

The code is only implemented for DW2 unwinder (probably used on most targets).

So my questions now are:
1) Would this feature considered useful i.e. will it be accepted for
trunk once implementation is polished/tested?
2) Should I strive to implement it for all possible targets or DW2
would do for now? I don't have easy access to other platforms (ARM,
C6x, etc.) so this may delay implementation.
3) Any suggestions/comments on this attached draft implementation?
E.g. alternative syscalls to use (Andrew suggested futex), how many
verified addresses to cache, whether I should verify unwind table
accesses in addition to stack accesses, etc.

-Y

[1] The issue has been reported in the past e.g. "Adventure with Stack
Smashing Protector" (http://site.pi3.com.pl/papers/ASSP.pdf).

[-- Attachment #2: repro.c --]
[-- Type: text/x-csrc, Size: 949 bytes --]

#include <string.h>
#include <stdio.h>

struct _Unwind_Context;

typedef int (*_Unwind_Trace_Fn)(struct _Unwind_Context *, void *vdata);

extern int _Unwind_Backtrace(_Unwind_Trace_Fn trace, void * trace_argument);
extern int _Unwind_Backtrace_Checked(_Unwind_Trace_Fn trace, void * trace_argument);

extern void *_Unwind_GetIP (struct _Unwind_Context *context);

int simple_unwind (struct _Unwind_Context *context, void *vdata) {
  printf("Next frame: ");
  void *pc = _Unwind_GetIP(context);
  printf("%p\n", pc);
  return 0;
}

#define noinline __attribute__((noinline))

noinline int foo() {
  // Clobber stack to provoke errors in unwinder
  int x;
  void *p = &x;
  asm("" :: "r"(p));
  memset(p, 0xa, 128);

  printf("After clobbering stack\n");

  int ret = _Unwind_Backtrace(simple_unwind, 0);
  printf("After unwind: %d\n", ret);

  return 0;
}

noinline int bar() {
  int x = foo();
  return x + 1;
}

int main() {
  bar();
  return 0;
}

[-- Attachment #3: safe-unwind-1.patch --]
[-- Type: application/octet-stream, Size: 24124 bytes --]

2017-01-12  Yury Gribov  <tetra2005@gmail.com>

	Added safe version of stack-unwinding routines.
	* config/cr16/unwind-cr16.c (uw_set_check_ptrs): New function.
	(uw_update_context, uw_update_context_1, uw_advance_context): Update type.
	* config/ia64/unwind-ia64.c (uw_set_check_ptrs): New function.
	(uw_update_context, uw_advance_context): Update type.
	* unwind-sjlj.c (uw_set_check_ptrs): New function.
	(uw_update_context, uw_advance_context): Update type.
	* config/xtensa/unwind-dw2-xtensa.c (uw_set_check_ptrs): New
	function.
	(uw_update_context, uw_update_context_1, uw_advance_context):
	Update type.
	* libgcc-std.ver.in (_Unwind_Backtrace_Checked): New function.
	* unwind-dw2.c (uw_set_check_ptrs, is_valid_memory_access,
	check_memory_access, _Unwind_GetGR_Checked,
	_Unwind_GetPtr_Checked, _Unwind_SetGR_Checked): New function.
	(_Unwind_Context): New flag.
	(_Unwind_GetGR, _Unwind_GetPtr, _Unwind_SetGR): Call helper.
	(execute_stack_op, uw_update_context_1): Verify memory
	accesses.
	(uw_update_context, uw_init_context_1, uw_init_context,
	uw_advance_context): Update type.
	* unwind_generic.h (_Unwind_Backtrace_Checked): New function.
	* unwind.inc (_Unwind_ForcedUnwind_Phase2): Check memory
	accesses.
	(_Unwind_Backtrace_1, _Unwind_Backtrace_Checked): New function.
	(_Unwind_Backtrace): Call helper.

diff -rup /home/yugr/src/gcc-6.2.0/libgcc/config/cr16/unwind-cr16.c /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/config/cr16/unwind-cr16.c
--- /home/yugr/src/gcc-6.2.0/libgcc/config/cr16/unwind-cr16.c	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/config/cr16/unwind-cr16.c	2017-01-12 11:21:05.509012708 +0300
@@ -135,7 +135,7 @@ union unaligned
   signed s8 __attribute__ ((mode (DI)));
 } __attribute__ ((packed));
 
-static void uw_update_context (struct _Unwind_Context *, _Unwind_FrameState *);
+static int uw_update_context (struct _Unwind_Context *, _Unwind_FrameState *);
 static _Unwind_Reason_Code uw_frame_state_for (struct _Unwind_Context *,
 					       _Unwind_FrameState *);
 
@@ -1320,7 +1320,7 @@ _Unwind_SetSpColumn (struct _Unwind_Cont
   _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), tmp_sp);
 }
 
-static void
+static int
 uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
   struct _Unwind_Context orig_context = *context;
@@ -1436,6 +1436,8 @@ uw_update_context_1 (struct _Unwind_Cont
 #ifdef MD_FROB_UPDATE_CONTEXT
   MD_FROB_UPDATE_CONTEXT (context, fs);
 #endif
+
+  return 0;  /* TODO */
 }
 
 /* CONTEXT describes the unwind state for a frame, and FS describes the FDE
@@ -1443,10 +1445,11 @@ uw_update_context_1 (struct _Unwind_Cont
    that the args_size and lsda members are not updated here, but later in
    uw_frame_state_for.  */
 
-static void
+static int
 uw_update_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
-  uw_update_context_1 (context, fs);
+  if (__builtin_expect (uw_update_context_1 (context, fs), 0))
+    return 1;
 
   /* In general this unwinder doesn't make any distinction between
      undefined and same_value rule.  Call-saved registers are assumed
@@ -1467,12 +1470,14 @@ uw_update_context (struct _Unwind_Contex
 #if defined( __CR16C__ )
   context->ra = (void*)( ( (unsigned)context->ra ) << 1 ) ;
 #endif
+
+  return 0;
 }
 
-static void
+static int
 uw_advance_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
-  uw_update_context (context, fs);
+  return uw_update_context (context, fs);
 }
 \f
 /* Fill in CONTEXT for top-of-stack.  The only valid registers at this
@@ -1490,6 +1495,12 @@ uw_advance_context (struct _Unwind_Conte
   while (0)
 
 static inline void
+uw_set_check_ptrs (struct _Unwind_Context *context)
+{
+  /* TODO */
+}
+
+static inline void
 init_dwarf_reg_size_table (void)
 {
   __builtin_init_dwarf_reg_size_table (dwarf_reg_size_table);
diff -rup /home/yugr/src/gcc-6.2.0/libgcc/config/ia64/unwind-ia64.c /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/config/ia64/unwind-ia64.c
--- /home/yugr/src/gcc-6.2.0/libgcc/config/ia64/unwind-ia64.c	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/config/ia64/unwind-ia64.c	2017-01-12 11:20:59.333012708 +0300
@@ -2051,7 +2051,7 @@ uw_update_reg_address (struct _Unwind_Co
     }
 }
 
-static void
+static int
 uw_update_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
   int i;
@@ -2102,12 +2102,14 @@ uw_update_context (struct _Unwind_Contex
       context->bsp = (unw_word)
 	ia64_rse_skip_regs ((unw_word *) context->bsp, -sol);
     }
+
+  return 0;  /* TODO */
 }
 
-static void
+static int
 uw_advance_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
-  uw_update_context (context, fs);
+  return uw_update_context (context, fs);
 }
 
 /* Fill in CONTEXT for top-of-stack.  The only valid registers at this
@@ -2161,6 +2163,12 @@ uw_init_context_1 (struct _Unwind_Contex
   uw_update_context (context, &fs);
 }
 
+static inline void
+uw_set_check_ptrs (struct _Unwind_Context *context)
+{
+  /* TODO */
+}
+
 /* Install (i.e. longjmp to) the contents of TARGET.  */
 
 static void __attribute__((noreturn))
diff -rup /home/yugr/src/gcc-6.2.0/libgcc/config/xtensa/unwind-dw2-xtensa.c /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/config/xtensa/unwind-dw2-xtensa.c
--- /home/yugr/src/gcc-6.2.0/libgcc/config/xtensa/unwind-dw2-xtensa.c	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/config/xtensa/unwind-dw2-xtensa.c	2017-01-12 11:20:52.137012708 +0300
@@ -90,7 +90,7 @@ union unaligned
   void *p;
 } __attribute__ ((packed));
 
-static void uw_update_context (struct _Unwind_Context *, _Unwind_FrameState *);
+static int uw_update_context (struct _Unwind_Context *, _Unwind_FrameState *);
 static _Unwind_Reason_Code uw_frame_state_for (struct _Unwind_Context *,
 					       _Unwind_FrameState *);
 
@@ -378,7 +378,7 @@ uw_frame_state_for (struct _Unwind_Conte
   return _URC_NO_REASON;
 }
 \f
-static void
+static int
 uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
   struct _Unwind_Context orig_context = *context;
@@ -417,6 +417,8 @@ uw_update_context_1 (struct _Unwind_Cont
   context->cfa = next_cfa;
 
   _Unwind_SetSignalFrame (context, fs->signal_frame);
+
+  return 0;  /* TODO */
 }
 
 /* CONTEXT describes the unwind state for a frame, and FS describes the FDE
@@ -424,10 +426,11 @@ uw_update_context_1 (struct _Unwind_Cont
    that the lsda member is not updated here, but later in
    uw_frame_state_for.  */
 
-static void
+static int
 uw_update_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
-  uw_update_context_1 (context, fs);
+  if (__builtin_expect (uw_update_context_1 (context, fs), 0))
+    return 1;
 
   /* Compute the return address now, since the return address column
      can change from frame to frame.  */
@@ -436,12 +439,14 @@ uw_update_context (struct _Unwind_Contex
   else
     context->ra = (void *) ((_Unwind_GetGR (context, fs->retaddr_column)
 			     & XTENSA_RA_FIELD_MASK) | context->ra_high_bits);
+
+  return 0;
 }
 
-static void
+static int
 uw_advance_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
-  uw_update_context (context, fs);
+  return uw_update_context (context, fs);
 }
 \f
 /* Fill in CONTEXT for top-of-stack.  The only valid registers at this
@@ -478,6 +483,12 @@ uw_init_context_1 (struct _Unwind_Contex
   context->ra = outer_ra;
 }
 
+static inline void
+uw_set_check_ptrs (struct _Unwind_Context *context)
+{
+  /* TODO */
+}
+
 
 /* Install TARGET into CURRENT so that we can return to it.  This is a
    macro because __builtin_eh_return must be invoked in the context of
diff -rup /home/yugr/src/gcc-6.2.0/libgcc/libgcc-std.ver.in /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/libgcc-std.ver.in
--- /home/yugr/src/gcc-6.2.0/libgcc/libgcc-std.ver.in	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/libgcc-std.ver.in	2017-01-06 09:19:30.427445201 +0300
@@ -1938,3 +1938,8 @@ GCC_4.7.0 {
 %inherit GCC_4.8.0 GCC_4.7.0
 GCC_4.8.0 {
 }
+
+%inherit GCC_6.2.0 GCC_4.8.0
+GCC_6.2.0 {
+  _Unwind_Backtrace_Checked
+}
Only in /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc: .Makefile.in.swp
Only in /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc: tags
diff -rup /home/yugr/src/gcc-6.2.0/libgcc/unwind-dw2.c /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind-dw2.c
--- /home/yugr/src/gcc-6.2.0/libgcc/unwind-dw2.c	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind-dw2.c	2017-01-12 11:21:59.437012708 +0300
@@ -41,6 +41,9 @@
 #include <sys/sdt.h>
 #endif
 
+/* TODO: check whether msync is present in configure? */
+#include <sys/mman.h>
+
 #ifndef __USING_SJLJ_EXCEPTIONS__
 
 #ifndef __LIBGCC_STACK_GROWS_DOWNWARD__
@@ -132,15 +135,19 @@ struct _Unwind_Context
   void *ra;
   void *lsda;
   struct dwarf_eh_bases bases;
+#define MSB_MASK(i) ((~(_Unwind_Word) 0 >> (i + 1)) + 1)
   /* Signal frame context.  */
-#define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
+#define SIGNAL_FRAME_BIT MSB_MASK(0)
   /* Context which has version/args_size/by_value fields.  */
-#define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
+#define EXTENDED_CONTEXT_BIT MSB_MASK(1)
+  /* Context which checks whether pointers are valid prior chasing them.  */
+#define CHECK_PTRS_BIT MSB_MASK(2)
   _Unwind_Word flags;
   /* 0 for now, can be increased when further fields are added to
      struct _Unwind_Context.  */
   _Unwind_Word version;
   _Unwind_Word args_size;
+  _Unwind_Ptr last_checked_page;
   char by_value[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
 };
 
@@ -161,7 +168,7 @@ union unaligned
   signed s8 __attribute__ ((mode (DI)));
 } __attribute__ ((packed));
 
-static void uw_update_context (struct _Unwind_Context *, _Unwind_FrameState *);
+static int uw_update_context (struct _Unwind_Context *, _Unwind_FrameState *);
 static _Unwind_Reason_Code uw_frame_state_for (struct _Unwind_Context *,
 					       _Unwind_FrameState *);
 
@@ -192,6 +199,32 @@ read_8u (const void *p) { const union un
 static inline unsigned long
 read_8s (const void *p) { const union unaligned *up = p; return up->s8; }
 \f
+static _Unwind_Word
+is_valid_memory_access (void *ptr, _Unwind_Word size, struct _Unwind_Context *context) {
+  if (__builtin_expect (!(context->flags & CHECK_PTRS_BIT), 1))
+    return 1;
+
+/* TODO: this should come from config.h.  */
+#define PAGE_SIZE 4096
+  _Unwind_Ptr page = (_Unwind_Ptr) ptr & ~((_Unwind_Ptr) PAGE_SIZE - 1);
+  _Unwind_Word len = (_Unwind_Ptr) ptr + size < page + PAGE_SIZE ? PAGE_SIZE : 2 * PAGE_SIZE;
+  if (__builtin_expect (len == PAGE_SIZE && page == context->last_checked_page, 1))
+    return 1;
+
+  if (__builtin_expect (msync ((void *)page, len, MS_ASYNC), 0))
+    return 0;
+
+  /* TODO: keep more than one page in cache?  */
+  context->last_checked_page = page;
+
+  return 1;
+}
+
+#define check_memory_access(ptr, size, status, rv, ctx) do { \
+  if (__builtin_expect (!is_valid_memory_access ((void *) ptr, size, ctx), 0)) \
+    return status = 1, rv; \
+  } while (0)
+\f
 static inline _Unwind_Word
 _Unwind_IsSignalFrame (struct _Unwind_Context *context)
 {
@@ -216,12 +249,15 @@ _Unwind_IsExtendedContext (struct _Unwin
 \f
 /* Get the value of register INDEX as saved in CONTEXT.  */
 
-inline _Unwind_Word
-_Unwind_GetGR (struct _Unwind_Context *context, int index)
+static _Unwind_Word
+_Unwind_GetGR_Checked (struct _Unwind_Context *context, int index, int *status)
 {
   int size;
   _Unwind_Context_Reg_Val val;
 
+  if (__builtin_expect (status != NULL, 0))
+    *status = 0;
+
 #ifdef DWARF_ZERO_REG
   if (index == DWARF_ZERO_REG)
     return 0;
@@ -235,6 +271,9 @@ _Unwind_GetGR (struct _Unwind_Context *c
   if (_Unwind_IsExtendedContext (context) && context->by_value[index])
     return _Unwind_Get_Unwind_Word (val);
 
+  if (__builtin_expect (status != NULL, 0))
+    check_memory_access (val, size, *status, 0, context);
+
   /* This will segfault if the register hasn't been saved.  */
   if (size == sizeof(_Unwind_Ptr))
     return * (_Unwind_Ptr *) (_Unwind_Internal_Ptr) val;
@@ -245,10 +284,22 @@ _Unwind_GetGR (struct _Unwind_Context *c
     }
 }
 
+_Unwind_Word
+_Unwind_GetGR (struct _Unwind_Context *context, int index)
+{
+  return _Unwind_GetGR_Checked (context, index, 0);
+}
+
+static inline void *
+_Unwind_GetPtr_Checked (struct _Unwind_Context *context, int index, int *status)
+{
+  return (void *)(_Unwind_Ptr) _Unwind_GetGR_Checked (context, index, status);
+}
+
 static inline void *
 _Unwind_GetPtr (struct _Unwind_Context *context, int index)
 {
-  return (void *)(_Unwind_Ptr) _Unwind_GetGR (context, index);
+  return (void *)(_Unwind_Ptr) _Unwind_GetGR_Checked (context, index, 0);
 }
 
 /* Get the value of the CFA as saved in CONTEXT.  */
@@ -261,12 +312,15 @@ _Unwind_GetCFA (struct _Unwind_Context *
 
 /* Overwrite the saved value for register INDEX in CONTEXT with VAL.  */
 
-inline void
-_Unwind_SetGR (struct _Unwind_Context *context, int index, _Unwind_Word val)
+static void
+_Unwind_SetGR_Checked (struct _Unwind_Context *context, int index, _Unwind_Word val, int *status)
 {
   int size;
   void *ptr;
 
+  if (__builtin_expect (status != NULL, 0))
+    *status = 0;
+
   index = DWARF_REG_TO_UNWIND_COLUMN (index);
   gcc_assert (index < (int) sizeof(dwarf_reg_size_table));
   size = dwarf_reg_size_table[index];
@@ -279,6 +333,9 @@ _Unwind_SetGR (struct _Unwind_Context *c
 
   ptr = (void *) (_Unwind_Internal_Ptr) context->reg[index];
 
+  if (__builtin_expect (status != NULL, 0))
+    check_memory_access (ptr, size, *status, (void) 0, context);
+
   if (size == sizeof(_Unwind_Ptr))
     * (_Unwind_Ptr *) ptr = val;
   else
@@ -288,6 +345,12 @@ _Unwind_SetGR (struct _Unwind_Context *c
     }
 }
 
+void
+_Unwind_SetGR (struct _Unwind_Context *context, int index, _Unwind_Word val)
+{
+  return _Unwind_SetGR_Checked (context, index, val, 0);
+}
+
 /* Get the pointer to a register INDEX as saved in CONTEXT.  */
 
 static inline void *
@@ -508,7 +571,8 @@ extract_cie_info (const struct dwarf_cie
 
 static _Unwind_Word
 execute_stack_op (const unsigned char *op_ptr, const unsigned char *op_end,
-		  struct _Unwind_Context *context, _Unwind_Word initial)
+		  struct _Unwind_Context *context, _Unwind_Word initial,
+		  int *status)
 {
   _Unwind_Word stack[64];	/* ??? Assume this is enough.  */
   int stack_elt;
@@ -516,6 +580,8 @@ execute_stack_op (const unsigned char *o
   stack[0] = initial;
   stack_elt = 1;
 
+  *status = 0;
+
   while (op_ptr < op_end)
     {
       enum dwarf_location_atom op = *op_ptr++;
@@ -756,6 +822,7 @@ execute_stack_op (const unsigned char *o
 	    case DW_OP_deref:
 	      {
 		void *ptr = (void *) (_Unwind_Ptr) result;
+		check_memory_access (ptr, sizeof (_Unwind_Ptr), *status, 0, context);
 		result = (_Unwind_Ptr) read_pointer (ptr);
 	      }
 	      break;
@@ -766,15 +833,19 @@ execute_stack_op (const unsigned char *o
 		switch (*op_ptr++)
 		  {
 		  case 1:
+		    check_memory_access (ptr, 1, *status, 0, context);
 		    result = read_1u (ptr);
 		    break;
 		  case 2:
+		    check_memory_access (ptr, 2, *status, 0, context);
 		    result = read_2u (ptr);
 		    break;
 		  case 4:
+		    check_memory_access (ptr, 4, *status, 0, context);
 		    result = read_4u (ptr);
 		    break;
 		  case 8:
+		    check_memory_access (ptr, 8, *status, 0, context);
 		    result = read_8u (ptr);
 		    break;
 		  default:
@@ -1246,6 +1317,9 @@ uw_frame_state_for (struct _Unwind_Conte
       /* Couldn't find frame unwind info for this function.  Try a
 	 target-specific fallback mechanism.  This will necessarily
 	 not provide a personality routine or LSDA.  */
+      /* TODO: should use more precise platform-specific checks inside fallback code.  */
+      int dummy __attribute__((unused));
+      check_memory_access (context->ra, sizeof (void *), dummy, _URC_FATAL_PHASE1_ERROR, context);
       return MD_FALLBACK_FRAME_STATE_FOR (context, fs);
 #else
       return _URC_END_OF_STACK;
@@ -1373,12 +1447,13 @@ _Unwind_SetSpColumn (struct _Unwind_Cont
   _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), tmp_sp);
 }
 
-static void
+static int
 uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
   struct _Unwind_Context orig_context = *context;
   void *cfa;
   long i;
+  int status;
 
 #ifdef __LIBGCC_EH_RETURN_STACKADJ_RTX__
   /* Special handling here: Many machines do not use a frame pointer,
@@ -1418,7 +1493,9 @@ uw_update_context_1 (struct _Unwind_Cont
 
 	exp = read_uleb128 (exp, &len);
 	cfa = (void *) (_Unwind_Ptr)
-	  execute_stack_op (exp, exp + len, &orig_context, 0);
+	  execute_stack_op (exp, exp + len, &orig_context, 0, &status);
+	if (__builtin_expect (status, 0))
+	  return status;
 	break;
       }
 
@@ -1459,7 +1536,9 @@ uw_update_context_1 (struct _Unwind_Cont
 
 	  exp = read_uleb128 (exp, &len);
 	  val = execute_stack_op (exp, exp + len, &orig_context,
-				  (_Unwind_Ptr) cfa);
+				  (_Unwind_Ptr) cfa, &status);
+	  if (__builtin_expect (status, 0))
+	    return status;
 	  _Unwind_SetGRPtr (context, i, (void *) val);
 	}
 	break;
@@ -1478,7 +1557,9 @@ uw_update_context_1 (struct _Unwind_Cont
 
 	  exp = read_uleb128 (exp, &len);
 	  val = execute_stack_op (exp, exp + len, &orig_context,
-				  (_Unwind_Ptr) cfa);
+				  (_Unwind_Ptr) cfa, &status);
+	  if (__builtin_expect (status, 0))
+	    return status;
 	  _Unwind_SetGRValue (context, i, val);
 	}
 	break;
@@ -1489,6 +1570,8 @@ uw_update_context_1 (struct _Unwind_Cont
 #ifdef MD_FROB_UPDATE_CONTEXT
   MD_FROB_UPDATE_CONTEXT (context, fs);
 #endif
+
+  return 0;
 }
 
 /* CONTEXT describes the unwind state for a frame, and FS describes the FDE
@@ -1496,10 +1579,11 @@ uw_update_context_1 (struct _Unwind_Cont
    that the args_size and lsda members are not updated here, but later in
    uw_frame_state_for.  */
 
-static void
+static int
 uw_update_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
-  uw_update_context_1 (context, fs);
+  if (__builtin_expect (uw_update_context_1 (context, fs), 0))
+    return 1;
 
   /* In general this unwinder doesn't make any distinction between
      undefined and same_value rule.  Call-saved registers are assumed
@@ -1517,12 +1601,14 @@ uw_update_context (struct _Unwind_Contex
        can change from frame to frame.  */
     context->ra = __builtin_extract_return_addr
       (_Unwind_GetPtr (context, fs->retaddr_column));
+
+  return 0;
 }
 
-static void
+static int
 uw_advance_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
-  uw_update_context (context, fs);
+  return uw_update_context (context, fs);
 }
 \f
 /* Fill in CONTEXT for top-of-stack.  The only valid registers at this
@@ -1545,7 +1631,7 @@ init_dwarf_reg_size_table (void)
   __builtin_init_dwarf_reg_size_table (dwarf_reg_size_table);
 }
 
-static void __attribute__((noinline))
+static int __attribute__((noinline))
 uw_init_context_1 (struct _Unwind_Context *context,
 		   void *outer_cfa, void *outer_ra)
 {
@@ -1580,12 +1666,21 @@ uw_init_context_1 (struct _Unwind_Contex
   fs.regs.cfa_reg = __builtin_dwarf_sp_column ();
   fs.regs.cfa_offset = 0;
 
-  uw_update_context_1 (context, &fs);
+  if (__builtin_expect (uw_update_context_1 (context, &fs), 0))
+    return 1;
 
   /* If the return address column was saved in a register in the
      initialization context, then we can't see it in the given
      call frame data.  So have the initialization context tell us.  */
   context->ra = __builtin_extract_return_addr (outer_ra);
+
+  return 0;
+}
+
+static inline void
+uw_set_check_ptrs (struct _Unwind_Context *context)
+{
+  context->flags |= CHECK_PTRS_BIT;
 }
 
 static void _Unwind_DebugHook (void *, void *)
diff -rup /home/yugr/src/gcc-6.2.0/libgcc/unwind-generic.h /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind-generic.h
--- /home/yugr/src/gcc-6.2.0/libgcc/unwind-generic.h	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind-generic.h	2017-01-06 09:28:12.903445201 +0300
@@ -162,6 +162,12 @@ typedef _Unwind_Reason_Code (*_Unwind_Tr
 extern _Unwind_Reason_Code LIBGCC2_UNWIND_ATTRIBUTE
 _Unwind_Backtrace (_Unwind_Trace_Fn, void *);
 
+/* @@@ Same as _Unwind_Backtrace but checks pointers
+   before following them, potentially avoiding accesses to
+   invalid memory when unwinding corrupted stacks.  */
+extern _Unwind_Reason_Code LIBGCC2_UNWIND_ATTRIBUTE
+_Unwind_Backtrace_Checked (_Unwind_Trace_Fn, void *);
+
 /* These functions are used for communicating information about the unwind
    context (i.e. the unwind descriptors and the user register state) between
    the unwind library and the personality routine and landing pad.  Only
diff -rup /home/yugr/src/gcc-6.2.0/libgcc/unwind.inc /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind.inc
--- /home/yugr/src/gcc-6.2.0/libgcc/unwind.inc	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind.inc	2017-01-12 11:18:20.989012708 +0300
@@ -182,7 +182,8 @@ _Unwind_ForcedUnwind_Phase2 (struct _Unw
 
       /* Update cur_context to describe the same frame as fs, and discard
 	 the previous context if necessary.  */
-      uw_advance_context (context, &fs);
+      if (uw_advance_context (context, &fs))
+	return _URC_FATAL_PHASE2_ERROR;
     }
 
   return code;
@@ -272,16 +273,20 @@ _Unwind_DeleteException (struct _Unwind_
 }
 
 
-/* Perform stack backtrace through unwind data.  */
+/* Common code to backtrace through unwind data.  */
 
-_Unwind_Reason_Code LIBGCC2_UNWIND_ATTRIBUTE
-_Unwind_Backtrace(_Unwind_Trace_Fn trace, void * trace_argument)
+static _Unwind_Reason_Code
+_Unwind_Backtrace_1 (_Unwind_Trace_Fn trace, void * trace_argument, int check_ptrs)
 {
   struct _Unwind_Context context;
   _Unwind_Reason_Code code;
 
+  /* TODO: check status of uw_init_context.  */
   uw_init_context (&context);
 
+  if (__builtin_expect (check_ptrs, 0))
+    uw_set_check_ptrs (&context);
+
   while (1)
     {
       _Unwind_FrameState fs;
@@ -300,8 +305,26 @@ _Unwind_Backtrace(_Unwind_Trace_Fn trace
 	break;
 
       /* Update context to describe the same frame as fs.  */
-      uw_update_context (&context, &fs);
+      if (__builtin_expect (uw_update_context (&context, &fs), 0))
+	return _URC_FATAL_PHASE1_ERROR;
     }
 
   return code;
 }
+
+/* Perform stack backtrace through unwind data.  */
+
+_Unwind_Reason_Code LIBGCC2_UNWIND_ATTRIBUTE
+_Unwind_Backtrace (_Unwind_Trace_Fn trace, void * trace_argument)
+{
+  return _Unwind_Backtrace_1 (trace, trace_argument, 0);
+}
+
+/* Perform stack backtrace through unwind data
+   but watch out for unsafe pointers in the way.  */
+
+_Unwind_Reason_Code LIBGCC2_UNWIND_ATTRIBUTE
+_Unwind_Backtrace_Checked (_Unwind_Trace_Fn trace, void * trace_argument)
+{
+  return _Unwind_Backtrace_1 (trace, trace_argument, 1);
+}
diff -rup /home/yugr/src/gcc-6.2.0/libgcc/unwind-sjlj.c /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind-sjlj.c
--- /home/yugr/src/gcc-6.2.0/libgcc/unwind-sjlj.c	2016-01-04 17:30:50.000000000 +0300
+++ /home/yugr/src/gcc-6.2.0-safe-unwind/libgcc/unwind-sjlj.c	2017-01-12 11:20:43.285012708 +0300
@@ -278,18 +278,19 @@ uw_frame_state_for (struct _Unwind_Conte
     }
 }
 
-static inline void
+static inline int
 uw_update_context (struct _Unwind_Context *context,
 		   _Unwind_FrameState *fs __attribute__((unused)) )
 {
   context->fc = context->fc->prev;
+  return 0;
 }
 
-static void
+static int
 uw_advance_context (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
   _Unwind_SjLj_Unregister (context->fc);
-  uw_update_context (context, fs);
+  return uw_update_context (context, fs, 0);
 }
 
 static inline void
@@ -298,6 +299,12 @@ uw_init_context (struct _Unwind_Context
   context->fc = _Unwind_SjLj_GetContext ();
 }
 
+static inline void
+uw_set_check_ptrs (struct _Unwind_Context *context)
+{
+  /* TODO */
+}
+
 static void __attribute__((noreturn))
 uw_install_context (struct _Unwind_Context *current __attribute__((unused)),
                     struct _Unwind_Context *target)

Re: [RFC][PR 67336][PING^2] Verify pointers during stack unwind

[Andrew Pinski]

On Sun, Jun 25, 2017 at 12:08 PM, Yuri Gribov <tetra2005@gmail.com> wrote:
> Hi all,
>
> Libgcc unwinder currently does not do any verification of pointers
> which it chases on stack. In practice this not so rarely causes
> segfaults when unwinding on corrupted stacks (e.g. when when trying to
> print diagnostic on
> fatal error) [1]. Ironically this usually happens in error reporting
> code which puzzles users.
>
> I've attached one motivating example - with current libgcc it will
> abort inside unwinder when trying to access invalid address
> (0x0a0a0a0a).
>
> There is an old request to provide a safer version of
> _Unwind_Backtrace (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67336)
> that would check pointer validity prior to dereferencing. I've
> attached a draft patch to see if this approach is viable and hopefully
> get some advice.
>
> The change is rather straightforward: I add a new
> _Unwind_Backtrace_Checked which checks return value of msync on every
> potentially unsafe access (libunwind does something like this as well,
> although in a very incomplete manner).
> To avoid paying for syscalls too often, I cache the last checked
> memory page. Potentially parsing /proc/$$/maps would allow for much
> faster verification but I didn't bother too much as new APIs are
> intended for reporting fatal errors where speed isn't an issue.
>
> The code is only implemented for DW2 unwinder (probably used on most targets).
>
> So my questions now are:
> 1) Would this feature considered useful i.e. will it be accepted for
> trunk once implementation is polished/tested?
> 2) Should I strive to implement it for all possible targets or DW2
> would do for now? I don't have easy access to other platforms (ARM,
> C6x, etc.) so this may delay implementation.
> 3) Any suggestions/comments on this attached draft implementation?
> E.g. alternative syscalls to use (Andrew suggested futex), how many
> verified addresses to cache, whether I should verify unwind table
> accesses in addition to stack accesses, etc.

The version script should be using GCC_8.0.0 since 6.2.0 has already
shipped months ago.
Also all patches should be submitted against the trunk and not a
released version.

Thanks,
Andrew

>
> -Y
>
> [1] The issue has been reported in the past e.g. "Adventure with Stack
> Smashing Protector" (http://site.pi3.com.pl/papers/ASSP.pdf).

Re: [RFC][PR 67336][PING^2] Verify pointers during stack unwind

[Yuri Gribov]

On Sun, Jun 25, 2017 at 8:13 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Sun, Jun 25, 2017 at 12:08 PM, Yuri Gribov <tetra2005@gmail.com> wrote:
>> Hi all,
>>
>> Libgcc unwinder currently does not do any verification of pointers
>> which it chases on stack. In practice this not so rarely causes
>> segfaults when unwinding on corrupted stacks (e.g. when when trying to
>> print diagnostic on
>> fatal error) [1]. Ironically this usually happens in error reporting
>> code which puzzles users.
>>
>> I've attached one motivating example - with current libgcc it will
>> abort inside unwinder when trying to access invalid address
>> (0x0a0a0a0a).
>>
>> There is an old request to provide a safer version of
>> _Unwind_Backtrace (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67336)
>> that would check pointer validity prior to dereferencing. I've
>> attached a draft patch to see if this approach is viable and hopefully
>> get some advice.
>>
>> The change is rather straightforward: I add a new
>> _Unwind_Backtrace_Checked which checks return value of msync on every
>> potentially unsafe access (libunwind does something like this as well,
>> although in a very incomplete manner).
>> To avoid paying for syscalls too often, I cache the last checked
>> memory page. Potentially parsing /proc/$$/maps would allow for much
>> faster verification but I didn't bother too much as new APIs are
>> intended for reporting fatal errors where speed isn't an issue.
>>
>> The code is only implemented for DW2 unwinder (probably used on most targets).
>>
>> So my questions now are:
>> 1) Would this feature considered useful i.e. will it be accepted for
>> trunk once implementation is polished/tested?
>> 2) Should I strive to implement it for all possible targets or DW2
>> would do for now? I don't have easy access to other platforms (ARM,
>> C6x, etc.) so this may delay implementation.
>> 3) Any suggestions/comments on this attached draft implementation?
>> E.g. alternative syscalls to use (Andrew suggested futex), how many
>> verified addresses to cache, whether I should verify unwind table
>> accesses in addition to stack accesses, etc.
>
> The version script should be using GCC_8.0.0 since 6.2.0 has already
> shipped months ago.
> Also all patches should be submitted against the trunk and not a
> released version.

Well, that's an RFC so I thought maybe original patch might be enough
for general ok/not ok decision...

-Y