[PATCH v2] ARM: Add pointer guard support.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] ARM: Add pointer guard support.

Will Newton

Add support for pointer mangling in glibc internal structures in C
and assembler code.

ports/ChangeLog.arm:

2013-09-27  Will Newton  <[hidden email]>

        * sysdeps/arm/__longjmp.S (__longjmp): Demangle fp, sp
        and lr when restoring register values.
        * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Remove
        sp and lr from list and replace fp with a4.
        * sysdeps/arm/jmpbuf-unwind.h (_jmpbuf_sp): New function.
        (_JMPBUF_UNWINDS_ADJ): Call _jmpbuf_sp.
        * sysdeps/arm/setjmp.S (__sigsetjmp): Mangle fp, sp and lr
        before storing register values.
        * sysdeps/arm/sysdep.h (LDST_GLOBAL): New macro.
        * sysdeps/unix/sysv/linux/arm/sysdep.h (PTR_MANGLE): New macro.
        (PTR_DEMANGLE): Likewise. (PTR_MANGLE2): Likewise.
        (PTR_DEMANGLE2): Likewise.
---
 ports/sysdeps/arm/__longjmp.S              | 14 +++++++++++
 ports/sysdeps/arm/include/bits/setjmp.h    |  5 ++--
 ports/sysdeps/arm/jmpbuf-unwind.h          | 13 +++++++++-
 ports/sysdeps/arm/setjmp.S                 | 14 +++++++++++
 ports/sysdeps/arm/sysdep.h                 | 11 +++++++++
 ports/sysdeps/unix/sysv/linux/arm/sysdep.h | 39 +++++++++++++++++++++++++++---
 6 files changed, 90 insertions(+), 6 deletions(-)

Changes in v2:
 - Use a global instead of a thread local variable to store the guard value
 - The bit I don't like in this version of the patch is the test for !IS_IN_nscd,
   but without it I get undefined references to __pointer_chk_guard_local.

diff --git a/ports/sysdeps/arm/__longjmp.S b/ports/sysdeps/arm/__longjmp.S
index a5edede..2b1f7f4 100644
--- a/ports/sysdeps/arm/__longjmp.S
+++ b/ports/sysdeps/arm/__longjmp.S
@@ -34,10 +34,24 @@ ENTRY (__longjmp)
  sfi_breg ip, \
  ldr r4, [\B, #32] /* jmpbuf's sp */
  cfi_undefined (r4)
+#ifdef PTR_DEMANGLE
+ PTR_DEMANGLE (r4, r4, a3, a4)
+#endif
  CHECK_SP (r4)
 #endif
  sfi_sp sfi_breg ip, \
  ldmia \B!, JMP_BUF_REGLIST
+#ifdef PTR_DEMANGLE
+ PTR_DEMANGLE (fp, a4, a3, a2)
+ ldr a4, [ip], #4
+ PTR_DEMANGLE2 (sp, a4, a3)
+ ldr a4, [ip], #4
+ PTR_DEMANGLE2 (lr, a4, a3)
+#else
+ mov fp, a4
+ ldr sp, [ip], #4
+ ldr lr, [ip], #4
+#endif
  cfi_restore (v1)
  cfi_restore (v2)
  cfi_restore (v3)
diff --git a/ports/sysdeps/arm/include/bits/setjmp.h b/ports/sysdeps/arm/include/bits/setjmp.h
index 1559d7b..64505dc 100644
--- a/ports/sysdeps/arm/include/bits/setjmp.h
+++ b/ports/sysdeps/arm/include/bits/setjmp.h
@@ -26,8 +26,9 @@

 #ifndef _ISOMAC
 /* Register list for a ldm/stm instruction to load/store
-   the general registers from a __jmp_buf.  */
-# define JMP_BUF_REGLIST {v1-v6, sl, fp, sp, lr}
+   the general registers from a __jmp_buf. The a4 register
+   contains fp at this point.  */
+# define JMP_BUF_REGLIST {a4, v1-v6, sl}

 /* Index of __jmp_buf where the sp register resides.  */
 # define __JMP_BUF_SP 8
diff --git a/ports/sysdeps/arm/jmpbuf-unwind.h b/ports/sysdeps/arm/jmpbuf-unwind.h
index 0863540..1b0d020 100644
--- a/ports/sysdeps/arm/jmpbuf-unwind.h
+++ b/ports/sysdeps/arm/jmpbuf-unwind.h
@@ -17,6 +17,7 @@

 #include <setjmp.h>
 #include <stdint.h>
+#include <sysdep.h>
 #include <unwind.h>

 /* Test if longjmp to JMPBUF would unwind the frame
@@ -27,8 +28,18 @@
 #define _JMPBUF_CFA_UNWINDS_ADJ(_jmpbuf, _context, _adj) \
   _JMPBUF_UNWINDS_ADJ (_jmpbuf, (void *) _Unwind_GetCFA (_context), _adj)

+static inline uintptr_t __attribute__ ((unused))
+_jmpbuf_sp (__jmp_buf regs)
+{
+  uintptr_t sp = regs[__JMP_BUF_SP];
+#ifdef PTR_DEMANGLE
+  PTR_DEMANGLE (sp);
+#endif
+  return sp;
+}
+
 #define _JMPBUF_UNWINDS_ADJ(_jmpbuf, _address, _adj) \
-  ((uintptr_t) (_address) - (_adj) < (uintptr_t) (_jmpbuf)[__JMP_BUF_SP] - (_adj))
+  ((uintptr_t) (_address) - (_adj) < _jmpbuf_sp (_jmpbuf) - (_adj))

 /* We use the normal longjmp for unwinding.  */
 #define __libc_unwind_longjmp(buf, val) __libc_longjmp (buf, val)
diff --git a/ports/sysdeps/arm/setjmp.S b/ports/sysdeps/arm/setjmp.S
index a6c161d..b38b919 100644
--- a/ports/sysdeps/arm/setjmp.S
+++ b/ports/sysdeps/arm/setjmp.S
@@ -24,11 +24,25 @@
 #include <arm-features.h>

 ENTRY (__sigsetjmp)
+#ifdef PTR_MANGLE
+ PTR_MANGLE (a4, fp, a3, ip)
+#else
+ mov a4, fp
+#endif
  mov ip, r0

  /* Save registers */
  sfi_breg ip, \
  stmia \B!, JMP_BUF_REGLIST
+#ifdef PTR_MANGLE
+ PTR_MANGLE2 (a4, sp, a3)
+ str a4, [ip], #4
+ PTR_MANGLE2 (a4, lr, a3)
+ str a4, [ip], #4
+#else
+ str sp, [ip], #4
+ str lr, [ip], #4
+#endif

 #if !defined ARM_ASSUME_NO_IWMMXT || defined __SOFTFP__
 # define NEED_HWCAP 1
diff --git a/ports/sysdeps/arm/sysdep.h b/ports/sysdeps/arm/sysdep.h
index 5501597..60d26fd 100644
--- a/ports/sysdeps/arm/sysdep.h
+++ b/ports/sysdeps/arm/sysdep.h
@@ -171,6 +171,17 @@
 99: OP R, [pc, T]
 # endif

+# define LDST_GLOBAL(OP, R, T, EXPR) \
+ ldr T, 99f; \
+ ldr R, 100f; \
+98: add T, T, pc; \
+ ldr T, [T, R]; \
+ .subsection 2; \
+99: .word _GLOBAL_OFFSET_TABLE_ - 98b - PC_OFS; \
+100: .word EXPR##(GOT); \
+ .previous; \
+ OP R, [T]
+
 /* Cope with negative memory offsets, which thumb can't encode.
    Use NEGOFF_ADJ_BASE to (conditionally) alter the base register,
    and then NEGOFF_OFF1 to use 0 for thumb and the offset for arm,
diff --git a/ports/sysdeps/unix/sysv/linux/arm/sysdep.h b/ports/sysdeps/unix/sysv/linux/arm/sysdep.h
index b195d8e..7ef8374 100644
--- a/ports/sysdeps/unix/sysv/linux/arm/sysdep.h
+++ b/ports/sysdeps/unix/sysv/linux/arm/sysdep.h
@@ -435,8 +435,41 @@ __local_syscall_error: \

 #endif /* __ASSEMBLER__ */

-/* Pointer mangling is not yet supported for ARM.  */
-#define PTR_MANGLE(var) (void) (var)
-#define PTR_DEMANGLE(var) (void) (var)
+/* Pointer mangling support.  */
+#if (defined NOT_IN_libc && defined IS_IN_rtld) || (!defined SHARED && !defined IS_IN_nscd)
+# ifdef __ASSEMBLER__
+#  define PTR_MANGLE(dst, src, guard, tmp) \
+  LDST_PCREL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard_local)); \
+  PTR_MANGLE2(dst, src, guard)
+#  define PTR_MANGLE2(dst, src, guard) \
+  eor dst, src, guard
+#  define PTR_DEMANGLE(dst, src, guard, tmp) \
+  PTR_MANGLE (dst, src, guard, tmp)
+#  define PTR_DEMANGLE2(dst, src, guard) \
+  PTR_MANGLE2 (dst, src, guard)
+# else
+extern uintptr_t __pointer_chk_guard_local attribute_relro attribute_hidden;
+#  define PTR_MANGLE(var) \
+  (var) = (__typeof (var)) ((uintptr_t) (var) ^ __pointer_chk_guard_local)
+#  define PTR_DEMANGLE(var)     PTR_MANGLE (var)
+# endif
+#else
+# ifdef __ASSEMBLER__
+#  define PTR_MANGLE(dst, src, guard, tmp) \
+  LDST_GLOBAL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard)); \
+  PTR_MANGLE2(dst, src, guard)
+#  define PTR_MANGLE2(dst, src, guard) \
+  eor dst, src, guard
+#  define PTR_DEMANGLE(dst, src, guard, tmp) \
+  PTR_MANGLE (dst, src, guard, tmp)
+#  define PTR_DEMANGLE2(dst, src, guard) \
+  PTR_MANGLE2 (dst, src, guard)
+# else
+extern uintptr_t __pointer_chk_guard attribute_relro;
+#  define PTR_MANGLE(var) \
+  (var) = (__typeof (var)) ((uintptr_t) (var) ^ __pointer_chk_guard)
+#  define PTR_DEMANGLE(var)     PTR_MANGLE (var)
+# endif
+#endif

 #endif /* linux/arm/sysdep.h */
--
1.8.1.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] ARM: Add pointer guard support.

Joseph Myers
On Fri, 27 Sep 2013, Will Newton wrote:

>  - The bit I don't like in this version of the patch is the test for !IS_IN_nscd,
>    but without it I get undefined references to __pointer_chk_guard_local.

Please don't just state the symptom.  Please give a more detailed analysis
of the issue, explaining *why* those undefined references arise - what
code gets used where in nscd, what semantics it is meant to have there,
what the correct way is to access the relevant variable in that context,
and what it is specifically about nscd that makes IS_IN_nscd (rather than
some other conditional that would also cover other executables built with
glibc) the logically correct conditional, and whether your logic applies
also to --disable-shared builds of glibc when nscd would be statically
linked.

In general, neither this patch nor the previous iteration includes any
rationale - you state what you do, but not anything about why that's a
good thing to do in the first place, or what choices were made and the
basis on which they were made.  Is there some overall design document
(past mailing list message, etc.) explaining "pointer mangling in glibc
internal structures" and discussing what macros should be defined, with
what semantics, and used where?  If so, point to it in your submission.  
If not, your patch submission should include something of that explanation
itself (with reference to other architectures where appropriate) -
presumably you reverse-engineered this machinery yourself to write the
patch, if there wasn't such an existing document, in which case you should
write up the results of your reverse-engineering to benefit the reviewer
and people implementing this for other architectures, rather than
expecting people to repeat it again.  (An alternative to putting it all in
the patch submission is to create such detailed documentation for pointer
mangling on the wiki, then point to the wiki page in the patch submission
email.  But any ARM-specific rationale should still go in the email.)

You define a macro LDST_GLOBAL.  This needs a comment explaining its
semantics in detail, like the other nearby macros in sysdep.h.  You can
avoid such detailed comments on the mangling macros if they have
architecture-independent semantics documented somewhere appropriate, but
not for any new architecture-specific macro.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] ARM: Add pointer guard support.

Will Newton
On 27 September 2013 17:20, Joseph S. Myers <[hidden email]> wrote:

> On Fri, 27 Sep 2013, Will Newton wrote:
>
>>  - The bit I don't like in this version of the patch is the test for !IS_IN_nscd,
>>    but without it I get undefined references to __pointer_chk_guard_local.
>
> Please don't just state the symptom.  Please give a more detailed analysis
> of the issue, explaining *why* those undefined references arise - what
> code gets used where in nscd, what semantics it is meant to have there,
> what the correct way is to access the relevant variable in that context,
> and what it is specifically about nscd that makes IS_IN_nscd (rather than
> some other conditional that would also cover other executables built with
> glibc) the logically correct conditional, and whether your logic applies
> also to --disable-shared builds of glibc when nscd would be statically
> linked.

It appears that __pointer_chk_guard is used by libc.so, but
__pointer_chk_guard_local is used by ldso and libc.a. I guess it would
be nicer to change the logic to !SHARED && !IS_IN_libc or similar. I'm
reverse engineering this stuff so I was hoping someone with more
knowledge of this stuff than I could clear up what is the intended
behaviour.

> In general, neither this patch nor the previous iteration includes any
> rationale - you state what you do, but not anything about why that's a
> good thing to do in the first place, or what choices were made and the

I had rather assumed that argument had been made by every other port
including this feature.

> basis on which they were made.  Is there some overall design document
> (past mailing list message, etc.) explaining "pointer mangling in glibc
> internal structures" and discussing what macros should be defined, with
> what semantics, and used where?  If so, point to it in your submission.

AFAIK there is this blog post:

http://udrepper.livejournal.com/13393.html

It's a bit light on detail and doesn't go as far as specifying the macros used.

> If not, your patch submission should include something of that explanation
> itself (with reference to other architectures where appropriate) -
> presumably you reverse-engineered this machinery yourself to write the
> patch, if there wasn't such an existing document, in which case you should
> write up the results of your reverse-engineering to benefit the reviewer
> and people implementing this for other architectures, rather than
> expecting people to repeat it again.  (An alternative to putting it all in
> the patch submission is to create such detailed documentation for pointer
> mangling on the wiki, then point to the wiki page in the patch submission
> email.  But any ARM-specific rationale should still go in the email.)

Ok.

> You define a macro LDST_GLOBAL.  This needs a comment explaining its
> semantics in detail, like the other nearby macros in sysdep.h.  You can
> avoid such detailed comments on the mangling macros if they have
> architecture-independent semantics documented somewhere appropriate, but
> not for any new architecture-specific macro.

Ok. I'll update the patch and resubmit.

--
Will Newton
Toolchain Working Group, Linaro