public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2] ARM: Add pointer guard support.
@ 2013-09-27 15:49 Will Newton
  2013-09-27 16:20 ` Joseph S. Myers
  0 siblings, 1 reply; 3+ messages in thread
From: Will Newton @ 2013-09-27 15:49 UTC (permalink / raw)
  To: libc-ports; +Cc: patches


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

ports/ChangeLog.arm:

2013-09-27  Will Newton  <will.newton@linaro.org>

	* 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ARM: Add pointer guard support.
  2013-09-27 15:49 [PATCH v2] ARM: Add pointer guard support Will Newton
@ 2013-09-27 16:20 ` Joseph S. Myers
  2013-09-27 19:46   ` Will Newton
  0 siblings, 1 reply; 3+ messages in thread
From: Joseph S. Myers @ 2013-09-27 16:20 UTC (permalink / raw)
  To: Will Newton; +Cc: libc-ports, patches

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
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ARM: Add pointer guard support.
  2013-09-27 16:20 ` Joseph S. Myers
@ 2013-09-27 19:46   ` Will Newton
  0 siblings, 0 replies; 3+ messages in thread
From: Will Newton @ 2013-09-27 19:46 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: libc-ports, Patch Tracking

On 27 September 2013 17:20, Joseph S. Myers <joseph@codesourcery.com> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2013-09-27 19:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-27 15:49 [PATCH v2] ARM: Add pointer guard support Will Newton
2013-09-27 16:20 ` Joseph S. Myers
2013-09-27 19:46   ` Will Newton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).