[PATCH] ARM: Don't apply pointer encryption to the frame pointer

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

[PATCH] ARM: Don't apply pointer encryption to the frame pointer

Will Newton

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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer

Joseph Myers
On Tue, 10 Dec 2013, Will Newton wrote:

> 2013-12-10  Will Newton  <[hidden email]>
>
> * 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
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer

Carlos O'Donell-6
On 12/10/2013 01:06 PM, Joseph S. Myers wrote:

> On Tue, 10 Dec 2013, Will Newton wrote:
>
>> 2013-12-10  Will Newton  <[hidden email]>
>>
>> * 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer

Will Newton
On 10 December 2013 18:57, Carlos O'Donell <[hidden email]> 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  <[hidden email]>
>>>
>>>      * 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer

Carlos O'Donell-6
On 12/10/2013 03:05 PM, Will Newton wrote:

> On 10 December 2013 18:57, Carlos O'Donell <[hidden email]> 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  <[hidden email]>
>>>>
>>>>      * 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ARM: Don't apply pointer encryption to the frame pointer

Carlos O'Donell-6
In reply to this post by Will Newton
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  <[hidden email]>
>
> * 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.