Re: [PATCH 1/5] hurd: Fix restoring reply port in sigreturn

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le ven. 14 avril 2023 22:36:56 +0300, a ecrit:
> We must not use the user's reply port (scp->sc_reply_port) for any of
> our own RPCs, otherwise various things break. So, use MACH_PORT_DEAD as
> a reply port when destroying our reply port, and make sure to do this
> after _hurd_sigstate_unlock (), which may do a gsync_wake () RPC.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/i386/sigreturn.c | 35 ++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/i386/sigreturn.c 
> b/sysdeps/mach/hurd/i386/sigreturn.c
> index 4f196710..a0fc8891 100644
> --- a/sysdeps/mach/hurd/i386/sigreturn.c
> +++ b/sysdeps/mach/hurd/i386/sigreturn.c
> @@ -26,11 +26,29 @@ register int *sp asm ("%esp");
>  /* This is run on the thread stack after restoring it, to be able to
>     unlock SS off sigstack.  */
>  static void
> -__sigreturn2 (int *usp)
> +__sigreturn2 (int *usp, struct sigcontext *scp)
>  {
> +  mach_port_t reply_port;
>    struct hurd_sigstate *ss = _hurd_self_sigstate ();
>    _hurd_sigstate_unlock (ss);
>  
> +  /* Destroy the MiG reply port used by the signal handler, and restore the
> +     reply port in use by the thread when interrupted.
> +
> +     We cannot use the original reply port for our RPCs that we do here, 
> since
> +     we could unexpectedly receive/consume a reply message meant for the user
> +     (in particular, msg_sig_post_reply), and also since we would deallocate
> +     the port if *our* RPC fails, which we don't want to do since the user
> +     still has the old name.  And so, temporarily set MACH_PORT_DEAD as our
> +     reply name, and make sure destroying the port is the very last RPC we
> +     do.  */
> +  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> +  THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
> +  if (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);
> +
>    sp = usp;
>  #define A(line) asm volatile (#line)
>    /* The members in the sigcontext are arranged in this order
> @@ -58,7 +76,6 @@ __sigreturn (struct sigcontext *scp)
>  {
>    struct hurd_sigstate *ss;
>    struct hurd_userlink *link = (void *) &scp[1];
> -  mach_port_t reply_port;
>  
>    if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
>      {
> @@ -98,13 +115,6 @@ __sigreturn (struct sigcontext *scp)
>    if (scp->sc_onstack)
>      ss->sigaltstack.ss_flags &= ~SS_ONSTACK;
>  
> -  /* Destroy the MiG reply port used by the signal handler, and restore the
> -     reply port in use by the thread when interrupted.  */
> -  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> -  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
> -  if (MACH_PORT_VALID (reply_port))
> -    __mach_port_destroy (__mach_task_self (), reply_port);
> -
>    if (scp->sc_fpused)
>      /* Restore the FPU state.  Mach conveniently stores the state
>         in the format the i387 `frstor' instruction uses to restore it.  */
> @@ -115,15 +125,16 @@ __sigreturn (struct sigcontext *scp)
>         copy the registers onto the user's stack, switch there, pop and
>         return.  */
>  
> -    int *usp = (int *) scp->sc_uesp;
> +    int usp_arg, *usp = (int *) scp->sc_uesp;
>  
>      *--usp = scp->sc_eip;
>      *--usp = scp->sc_efl;
>      memcpy (usp -= 12, &scp->sc_i386_thread_state, 12 * sizeof (int));
> +    usp_arg = (int) usp;
>  
> +    *--usp = (int) scp;
>      /* Pass usp to __sigreturn2 so it can unwind itself easily.  */
> -    *(usp-1) = (int) usp;
> -    --usp;
> +    *--usp = usp_arg;
>      /* Bogus return address for __sigreturn2 */
>      *--usp = 0;
>      *--usp = (int) __sigreturn2;
> -- 
> 2.39.2
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.