Re: 64bit startup

[Sergey Bugaev]

Hello,

1. I have figured out how to use separate debuginfo; it's really easy:
   you add it with add-symbol-file (either together with the ELF
   containing actual code, or without it seems to work just fine too).
   For the .text address, you specify where the original ELF's .text is
   mapped -- so if you're adding both ELFs, just specify the same
   address. Using this, I'm able to debug Debian libc builds with
   relative ease -- great!

2. Your image really does work very well! It's amazing!

3. As to the bogus port bug:

it's not the TCB that is getting overwritten, but sigcontext! This is
the exact thing I was bringing attention to earlier: we're dumping the
registers onto the user's stack, but we could be overwriting the
sigcontext which is also located there somewhere -- and it turns out we
are, though seemingly we do it later inside __sigreturn2 when we use the
user's stack to call some functions (unlock the sigstate, deallocate the
signal reply port). Specifically, we were clobbering scp->sc_reply_port
which we then restore -- which means the code running after the signal
handler returned was seeing its tcb->reply_port suddenly change into a
bogus value.

The solution we were thinking about -- making some space for the
register dump at the end of 'struct stackframe' -- won't work, since
it's not only the register dump that gets located there; we're using
that stack to call functions, and who knows how much stack space they're
going to need. Fortunately, I'm copying registers in just the right
order that still works in the face of the register dump and the
sigcontext possibly overlapping (namely: the same order as they are in
sigcontext, and going from the higher addresses down), and sc_reply_port
is the only other bit of data we load from the sigcontext, so fixing
this should be a matter of loading sc_reply_port *before* we start
clobbering the sigcontex.

I'm attaching a patch that does that and also documents the pitfalls
(and also removes an extra block scope / level of indentation). It's
untested, but it builds and the generated code looks OK (sc_reply_port
gets loaded straight into %rdx, to be passed into __sigreturn2). Please
test!

The stack guard issue must be caused by something else.

Sergey

-- >8 --

From: Sergey Bugaev <bugaevc@gmail.com>
Subject: [PATCH] hurd: Fix x86_64 sigreturn restoring bogus reply_port

Since the area of the user's stack we use for the registers dump (and
otherwise as __sigreturn2's stack) can and does overlap the sigcontext,
we have to be very careful about the order of loads and stores that we
do. In particular we have to load sc_reply_port before we start
clobbering the sigcontext.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/x86_64/sigreturn.c | 84 +++++++++++++++-------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/sysdeps/mach/hurd/x86_64/sigreturn.c 
b/sysdeps/mach/hurd/x86_64/sigreturn.c
index f37ae119..94a379d3 100644
--- a/sysdeps/mach/hurd/x86_64/sigreturn.c
+++ b/sysdeps/mach/hurd/x86_64/sigreturn.c
@@ -24,7 +24,7 @@
    unlock SS off sigstack.  */
 void
 __sigreturn2 (struct hurd_sigstate *ss, uintptr_t *usp,
-              struct sigcontext *scp)
+              mach_port_t sc_reply_port)
 {
   mach_port_t reply_port;
   _hurd_sigstate_unlock (ss);
@@ -44,7 +44,7 @@ __sigreturn2 (struct hurd_sigstate *ss, uintptr_t *usp,
   if (__glibc_likely (MACH_PORT_VALID (reply_port)))
     (void) __mach_port_mod_refs (__mach_task_self (), reply_port,
                                  MACH_PORT_RIGHT_RECEIVE, -1);
-  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
+  THREAD_SETMEM (THREAD_SELF, reply_port, sc_reply_port);
 
   asm volatile (
                 /* Point the stack to the register dump.  */
@@ -77,6 +77,8 @@ __sigreturn (struct sigcontext *scp)
 {
   struct hurd_sigstate *ss;
   struct hurd_userlink *link = (void *) &scp[1];
+  uintptr_t *usp;
+  mach_port_t sc_reply_port;
 
   if (__glibc_unlikely (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK)))
     return __hurd_fail (EINVAL);
@@ -118,42 +120,48 @@ __sigreturn (struct sigcontext *scp)
        in the format the i387 `frstor' instruction uses to restore it.  */
     asm volatile ("frstor %0" : : "m" (scp->sc_fpsave));
 
-  {
-    /* There are convenient instructions to pop state off the stack, so we
-       copy the registers onto the user's stack, switch there, pop and
-       return.  */
-
-    uintptr_t *usp = (uintptr_t *) (scp->sc_ursp - 128);
-
-    *--usp = scp->sc_rip;
-    *--usp = scp->sc_rfl;
-    *--usp = scp->sc_rax;
-    *--usp = scp->sc_rcx;
-    *--usp = scp->sc_rdx;
-    *--usp = scp->sc_rbx;
-    *--usp = scp->sc_rbp;
-    *--usp = scp->sc_rsi;
-    *--usp = scp->sc_rdi;
-    *--usp = scp->sc_r15;
-    *--usp = scp->sc_r14;
-    *--usp = scp->sc_r13;
-    *--usp = scp->sc_r12;
-    *--usp = scp->sc_r11;
-    *--usp = scp->sc_r10;
-    *--usp = scp->sc_r9;
-    *--usp = scp->sc_r8;
-
-    /* Switch to the user's stack that we have just prepared, and call
-       __sigreturn2.  Clobber "memory" to make sure GCC flushes the stack
-       setup to actual memory.  We align the stack as per the ABI, but pass
-       the original usp to __sigreturn2 as an argument.  */
-    asm volatile ("movq %1, %%rsp\n"
-                  "andq $-16, %%rsp\n"
-                  "call __sigreturn2" :
-                  : "D" (ss), "S" (usp), "d" (scp)
-                  : "memory");
-    __builtin_unreachable ();
-  }
+  /* Copy the registers onto the user's stack, to be able to release the
+     altstack (by unlocking sigstate).  Note that unless an altstack is used,
+     the sigcontext will itself be located on the user's stack, so we may well
+     be overwriting it here (or later in __sigreturn2).
+
+     So: do this very carefully.  First, load sc_reply_port, which is the only
+     other bit of sigcontext that __sigreturn2 needs.  Then copy the registers
+     without reordering them, but skipping the ones we won't need.  We have to
+     copy starting from the larger addresses down, since our register dump is
+     located at a larger address than the sigcontext.  */
+
+  sc_reply_port = scp->sc_reply_port;
+  usp = (uintptr_t *) (scp->sc_ursp - 128);
+
+  *--usp = scp->sc_rip;
+  *--usp = scp->sc_rfl;
+  *--usp = scp->sc_rax;
+  *--usp = scp->sc_rcx;
+  *--usp = scp->sc_rdx;
+  *--usp = scp->sc_rbx;
+  *--usp = scp->sc_rbp;
+  *--usp = scp->sc_rsi;
+  *--usp = scp->sc_rdi;
+  *--usp = scp->sc_r15;
+  *--usp = scp->sc_r14;
+  *--usp = scp->sc_r13;
+  *--usp = scp->sc_r12;
+  *--usp = scp->sc_r11;
+  *--usp = scp->sc_r10;
+  *--usp = scp->sc_r9;
+  *--usp = scp->sc_r8;
+
+  /* Switch to the user's stack that we have just prepared, and call
+     __sigreturn2.  Clobber "memory" to make sure GCC flushes the stack
+     setup to actual memory.  We align the stack as per the ABI, but pass
+     the original usp to __sigreturn2 as an argument.  */
+  asm volatile ("movq %1, %%rsp\n"
+                "andq $-16, %%rsp\n"
+                "call __sigreturn2" :
+                : "D" (ss), "S" (usp), "d" (sc_reply_port)
+                : "memory");
+  __builtin_unreachable ();
 }
 
 weak_alias (__sigreturn, sigreturn)
-- 
2.40.1