public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] ARM: Don't apply pointer encryption to the frame pointer
@ 2013-12-10 17:28 Will Newton
  2013-12-10 18:07 ` Joseph S. Myers
  2013-12-13 15:57 ` Carlos O'Donell
  0 siblings, 2 replies; 6+ messages in thread
From: Will Newton @ 2013-12-10 17:28 UTC (permalink / raw)
  To: libc-ports; +Cc: Patch Tracking


The frame pointer register is rarely used for that purpose on ARM and
applications that look at the contents of the jmp_buf may be relying
on reading the value. Ruby uses the contents of jmp_buf to find the
root set for garbage collection so relies on this pointer value being
unencrypted.

ports/ChangeLog.arm:

2013-12-10  Will Newton  <will.newton@linaro.org>

	* sysdeps/arm/__longjmp.S: Don't apply pointer encryption
	to fp register.
	* sysdeps/arm/setjmp.S: Likewise.
 	* sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add
	fp to register list, remove a4.
	* sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD):
	New macro.
---
 ports/sysdeps/arm/__longjmp.S              | 4 +---
 ports/sysdeps/arm/include/bits/setjmp.h    | 5 ++---
 ports/sysdeps/arm/setjmp.S                 | 4 +---
 ports/sysdeps/unix/sysv/linux/arm/sysdep.h | 8 ++++++--
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/ports/sysdeps/arm/__longjmp.S b/ports/sysdeps/arm/__longjmp.S
index 894c121..aaa2d3d 100644
--- a/ports/sysdeps/arm/__longjmp.S
+++ b/ports/sysdeps/arm/__longjmp.S
@@ -41,14 +41,12 @@ ENTRY (__longjmp)
 	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 (a4, a4, a3)
+	PTR_DEMANGLE (a4, a4, a3, a2)
 	mov	sp, a4
 	ldr	a4, [ip], #4
 	PTR_DEMANGLE2 (lr, a4, a3)
 #else
-	mov	fp, a4
 	ldr	sp, [ip], #4
 	ldr	lr, [ip], #4
 #endif
diff --git a/ports/sysdeps/arm/include/bits/setjmp.h b/ports/sysdeps/arm/include/bits/setjmp.h
index 64505dc..7bb4f00 100644
--- a/ports/sysdeps/arm/include/bits/setjmp.h
+++ b/ports/sysdeps/arm/include/bits/setjmp.h
@@ -26,9 +26,8 @@

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

 /* Index of __jmp_buf where the sp register resides.  */
 # define __JMP_BUF_SP		8
diff --git a/ports/sysdeps/arm/setjmp.S b/ports/sysdeps/arm/setjmp.S
index fedd994..803591e 100644
--- a/ports/sysdeps/arm/setjmp.S
+++ b/ports/sysdeps/arm/setjmp.S
@@ -23,9 +23,7 @@

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

diff --git a/ports/sysdeps/unix/sysv/linux/arm/sysdep.h b/ports/sysdeps/unix/sysv/linux/arm/sysdep.h
index 6cfe4e0..ccab57e 100644
--- a/ports/sysdeps/unix/sysv/linux/arm/sysdep.h
+++ b/ports/sysdeps/unix/sysv/linux/arm/sysdep.h
@@ -439,8 +439,10 @@ __local_syscall_error:						\
 #if (defined NOT_IN_libc && defined IS_IN_rtld) || \
   (!defined SHARED && (!defined NOT_IN_libc || defined IS_IN_libpthread))
 # ifdef __ASSEMBLER__
+#  define PTR_MANGLE_LOAD(guard, tmp)					\
+  LDST_PCREL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard_local));
 #  define PTR_MANGLE(dst, src, guard, tmp)				\
-  LDST_PCREL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard_local)); \
+  PTR_MANGLE_LOAD(guard, tmp);						\
   PTR_MANGLE2(dst, src, guard)
 /* Use PTR_MANGLE2 for efficiency if guard is already loaded.  */
 #  define PTR_MANGLE2(dst, src, guard)		\
@@ -457,8 +459,10 @@ extern uintptr_t __pointer_chk_guard_local attribute_relro attribute_hidden;
 # endif
 #else
 # ifdef __ASSEMBLER__
+#  define PTR_MANGLE_LOAD(guard, tmp)					\
+  LDST_GLOBAL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard));
 #  define PTR_MANGLE(dst, src, guard, tmp)				\
-  LDST_GLOBAL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard));	\
+  PTR_MANGLE_LOAD(guard, tmp);						\
   PTR_MANGLE2(dst, src, guard)
 /* Use PTR_MANGLE2 for efficiency if guard is already loaded.  */
 #  define PTR_MANGLE2(dst, src, guard)		\
-- 
1.8.1.4

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

* Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer
  2013-12-10 17:28 [PATCH] ARM: Don't apply pointer encryption to the frame pointer Will Newton
@ 2013-12-10 18:07 ` Joseph S. Myers
  2013-12-10 18:57   ` Carlos O'Donell
  2013-12-13 15:57 ` Carlos O'Donell
  1 sibling, 1 reply; 6+ messages in thread
From: Joseph S. Myers @ 2013-12-10 18:07 UTC (permalink / raw)
  To: Will Newton; +Cc: libc-ports, Patch Tracking

On Tue, 10 Dec 2013, Will Newton wrote:

> 2013-12-10  Will Newton  <will.newton@linaro.org>
> 
> 	* sysdeps/arm/__longjmp.S: Don't apply pointer encryption
> 	to fp register.
> 	* sysdeps/arm/setjmp.S: Likewise.
>  	* sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add
> 	fp to register list, remove a4.
> 	* sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD):
> 	New macro.

OK, presuming you've tested this with the glibc testsuite.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

* Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer
  2013-12-10 18:07 ` Joseph S. Myers
@ 2013-12-10 18:57   ` Carlos O'Donell
  2013-12-10 20:05     ` Will Newton
  0 siblings, 1 reply; 6+ messages in thread
From: Carlos O'Donell @ 2013-12-10 18:57 UTC (permalink / raw)
  To: Joseph S. Myers, Will Newton; +Cc: libc-ports, Patch Tracking

On 12/10/2013 01:06 PM, Joseph S. Myers wrote:
> On Tue, 10 Dec 2013, Will Newton wrote:
> 
>> 2013-12-10  Will Newton  <will.newton@linaro.org>
>>
>> 	* sysdeps/arm/__longjmp.S: Don't apply pointer encryption
>> 	to fp register.
>> 	* sysdeps/arm/setjmp.S: Likewise.
>>  	* sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add
>> 	fp to register list, remove a4.
>> 	* sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD):
>> 	New macro.
> 
> OK, presuming you've tested this with the glibc testsuite.
> 

Is it really true that ruby checks the FP?

I don't see such code? Can you please point it out?

Cheers,
Carlos.

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

* Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer
  2013-12-10 18:57   ` Carlos O'Donell
@ 2013-12-10 20:05     ` Will Newton
  2013-12-10 20:11       ` Carlos O'Donell
  0 siblings, 1 reply; 6+ messages in thread
From: Will Newton @ 2013-12-10 20:05 UTC (permalink / raw)
  To: Carlos O'Donell; +Cc: Joseph S. Myers, libc-ports, Patch Tracking

On 10 December 2013 18:57, Carlos O'Donell <carlos@redhat.com> wrote:
> On 12/10/2013 01:06 PM, Joseph S. Myers wrote:
>> On Tue, 10 Dec 2013, Will Newton wrote:
>>
>>> 2013-12-10  Will Newton  <will.newton@linaro.org>
>>>
>>>      * sysdeps/arm/__longjmp.S: Don't apply pointer encryption
>>>      to fp register.
>>>      * sysdeps/arm/setjmp.S: Likewise.
>>>      * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add
>>>      fp to register list, remove a4.
>>>      * sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD):
>>>      New macro.
>>
>> OK, presuming you've tested this with the glibc testsuite.
>>
>
> Is it really true that ruby checks the FP?
>
> I don't see such code? Can you please point it out?

In vm_core.h:

   474      jmp_buf machine_regs;

In vm.c:

  1589          if (GET_THREAD() != th && th->machine_stack_start &&
th->machine_stack_end) {
  1590              rb_gc_mark_machine_stack(th);
  1591              rb_gc_mark_locations((VALUE *)&th->machine_regs,
  1592                                   (VALUE *)(&th->machine_regs) +
  1593                                   sizeof(th->machine_regs) /
sizeof(VALUE));
  1594          }

So it looks like a conservative GC that uses the jmp_buf as a data
array to find potentially reachable objects. If fp contained a pointer
to an object then the pointer encryption would render it
undiscoverable and it would not get marked as live and could be
collected in error.

There are a number of "ifs" involved and I haven't got the testsuite
running yet but it looks like a possibility.

-- 
Will Newton
Toolchain Working Group, Linaro

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

* Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer
  2013-12-10 20:05     ` Will Newton
@ 2013-12-10 20:11       ` Carlos O'Donell
  0 siblings, 0 replies; 6+ messages in thread
From: Carlos O'Donell @ 2013-12-10 20:11 UTC (permalink / raw)
  To: Will Newton; +Cc: Joseph S. Myers, libc-ports, Patch Tracking

On 12/10/2013 03:05 PM, Will Newton wrote:
> On 10 December 2013 18:57, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 12/10/2013 01:06 PM, Joseph S. Myers wrote:
>>> On Tue, 10 Dec 2013, Will Newton wrote:
>>>
>>>> 2013-12-10  Will Newton  <will.newton@linaro.org>
>>>>
>>>>      * sysdeps/arm/__longjmp.S: Don't apply pointer encryption
>>>>      to fp register.
>>>>      * sysdeps/arm/setjmp.S: Likewise.
>>>>      * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add
>>>>      fp to register list, remove a4.
>>>>      * sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD):
>>>>      New macro.
>>>
>>> OK, presuming you've tested this with the glibc testsuite.
>>>
>>
>> Is it really true that ruby checks the FP?
>>
>> I don't see such code? Can you please point it out?
> 
> In vm_core.h:
> 
>    474      jmp_buf machine_regs;
> 
> In vm.c:
> 
>   1589          if (GET_THREAD() != th && th->machine_stack_start &&
> th->machine_stack_end) {
>   1590              rb_gc_mark_machine_stack(th);
>   1591              rb_gc_mark_locations((VALUE *)&th->machine_regs,
>   1592                                   (VALUE *)(&th->machine_regs) +
>   1593                                   sizeof(th->machine_regs) /
> sizeof(VALUE));
>   1594          }
> 
> So it looks like a conservative GC that uses the jmp_buf as a data
> array to find potentially reachable objects. If fp contained a pointer
> to an object then the pointer encryption would render it
> undiscoverable and it would not get marked as live and could be
> collected in error.
> 
> There are a number of "ifs" involved and I haven't got the testsuite
> running yet but it looks like a possibility.
> 

Thanks for pointing that out.

I'll get a new glibc to the Fedora ruby maintainer and ask them to
test in parallel.

Cheers,
Carlos.

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

* Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer
  2013-12-10 17:28 [PATCH] ARM: Don't apply pointer encryption to the frame pointer Will Newton
  2013-12-10 18:07 ` Joseph S. Myers
@ 2013-12-13 15:57 ` Carlos O'Donell
  1 sibling, 0 replies; 6+ messages in thread
From: Carlos O'Donell @ 2013-12-13 15:57 UTC (permalink / raw)
  To: Will Newton, libc-ports; +Cc: Patch Tracking

On 12/10/2013 12:28 PM, Will Newton wrote:
> 
> The frame pointer register is rarely used for that purpose on ARM and
> applications that look at the contents of the jmp_buf may be relying
> on reading the value. Ruby uses the contents of jmp_buf to find the
> root set for garbage collection so relies on this pointer value being
> unencrypted.
> 
> ports/ChangeLog.arm:
> 
> 2013-12-10  Will Newton  <will.newton@linaro.org>
> 
> 	* sysdeps/arm/__longjmp.S: Don't apply pointer encryption
> 	to fp register.
> 	* sysdeps/arm/setjmp.S: Likewise.
>  	* sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add
> 	fp to register list, remove a4.
> 	* sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD):
> 	New macro.

This looks good to me.

I agree that because fp might not be used for a frame pointer,
and the AEABI doesn't mandate it, that it might have useful
required information that the ruby GC might need.

So while ruby is wrong in inspecting jmp_buf, we're actually
encrypting a general register which ruby might need to to scan
to correctly support dynamic GC.

As David Miller points out this structure might have been on the
stack and the gc would be scanning the stack looking for valid
pointers and need this information.

Cheers,
Carlos.

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

end of thread, other threads:[~2013-12-13 15:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-10 17:28 [PATCH] ARM: Don't apply pointer encryption to the frame pointer Will Newton
2013-12-10 18:07 ` Joseph S. Myers
2013-12-10 18:57   ` Carlos O'Donell
2013-12-10 20:05     ` Will Newton
2013-12-10 20:11       ` Carlos O'Donell
2013-12-13 15:57 ` Carlos O'Donell

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).