[RFC,25/34] hurd: Improve reply port handling when exiting signal handlers

  If we're doing signals, that means we've already got the signal thread
running, and that implies TLS having been set up. So we know that
__hurd_local_reply_port will resolve to THREAD_SELF->reply_port, and can
access that directly using the THREAD_GETMEM and THREAD_SETMEM macros.
This avoids potential miscompilations, and should also be a tiny bit
faster.

Also, use mach_port_mod_refs () and not mach_port_destroy () to destroy
the receive right. mach_port_destroy () should *never* be used on
mach_task_self (); this can easily lead to port use-after-free
vulnerabilities if the task has any other references to the same port.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---

NOTE: I don't really understand why sigunwind wants to destroy both its
current reply port and user's reply port. Prior to commit
fb304035c41c7ee2afede51e5e8568974549ba5e, it was *restoring* the user's
reply port, in which case it actually made sense to destroy the current
reply port. Post-fb304035c41c7ee2afede51e5e8568974549ba5e, wouldn't it
be better to just keep using the current reply port, destroying the
user's one?

 hurd/sigunwind.c                   | 24 +++++++++++-------------
 sysdeps/mach/hurd/i386/sigreturn.c | 21 +++++----------------
 2 files changed, 16 insertions(+), 29 deletions(-)
  

Applied, thanks!

Sergey Bugaev, le dim. 19 mars 2023 18:10:08 +0300, a ecrit:
> NOTE: I don't really understand why sigunwind wants to destroy both its
> current reply port and user's reply port. Prior to commit
> fb304035c41c7ee2afede51e5e8568974549ba5e, it was *restoring* the user's
> reply port, in which case it actually made sense to destroy the current
> reply port. Post-fb304035c41c7ee2afede51e5e8568974549ba5e, wouldn't it
> be better to just keep using the current reply port, destroying the
> user's one?

We could try that, yes.

I tend to be very cautious with reply port reuse since it can confuse
servers a lot when e.g. interrupted, so it's generally safer not to try
to reuse them.

>  hurd/sigunwind.c                   | 24 +++++++++++-------------
>  sysdeps/mach/hurd/i386/sigreturn.c | 21 +++++----------------
>  2 files changed, 16 insertions(+), 29 deletions(-)
> 
> diff --git a/hurd/sigunwind.c b/hurd/sigunwind.c
> index edd40b14..399e6900 100644
> --- a/hurd/sigunwind.c
> +++ b/hurd/sigunwind.c
> @@ -18,7 +18,6 @@
>  
>  #include <hurd.h>
>  #include <thread_state.h>
> -#include <hurd/threadvar.h>
>  #include <jmpbuf-unwind.h>
>  #include <assert.h>
>  #include <stdint.h>
> @@ -39,19 +38,18 @@ _hurdsig_longjmp_from_handler (void *data, jmp_buf env, int val)
>      {
>        /* Destroy the MiG reply port used by the signal handler, and restore
>  	 the reply port in use by the thread when interrupted.  */
> -      mach_port_t *reply_port = &__hurd_local_reply_port;
> -      if (*reply_port)
> -	{
> -	  mach_port_t port = *reply_port;
> -	  /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port
> -	     not to get another reply port, but avoids mig_dealloc_reply_port
> -	     trying to deallocate it after the receive fails (which it will,
> -	     because the reply port will be bogus, regardless).  */
> -	  *reply_port = MACH_PORT_DEAD;
> -	  __mach_port_destroy (__mach_task_self (), port);
> -	}
> +      mach_port_t reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> +      /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to
> +         get another reply port, but avoids mig_dealloc_reply_port trying to
> +         deallocate it after the receive fails (which it will, because the
> +         reply port will be bogus, regardless).  */
> +      THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
> +      if (MACH_PORT_VALID (reply_port))
> +        __mach_port_mod_refs (__mach_task_self (), reply_port,
> +                              MACH_PORT_RIGHT_RECEIVE, -1);
>        if (scp->sc_reply_port)
> -	__mach_port_destroy (__mach_task_self (), scp->sc_reply_port);
> +        __mach_port_mod_refs (__mach_task_self (), scp->sc_reply_port,
> +                              MACH_PORT_RIGHT_RECEIVE, -1);
>      }
>  
>    __spin_lock (&ss->lock);
> diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c
> index db1a01f3..29c9629f 100644
> --- a/sysdeps/mach/hurd/i386/sigreturn.c
> +++ b/sysdeps/mach/hurd/i386/sigreturn.c
> @@ -19,7 +19,6 @@ register int *sp asm ("%esp");
>  
>  #include <hurd.h>
>  #include <hurd/signal.h>
> -#include <hurd/threadvar.h>
>  #include <hurd/msg.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -59,7 +58,7 @@ __sigreturn (struct sigcontext *scp)
>  {
>    struct hurd_sigstate *ss;
>    struct hurd_userlink *link = (void *) &scp[1];
> -  mach_port_t *reply_port;
> +  mach_port_t reply_port;
>  
>    if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
>      {
> @@ -101,20 +100,10 @@ __sigreturn (struct sigcontext *scp)
>  
>    /* Destroy the MiG reply port used by the signal handler, and restore the
>       reply port in use by the thread when interrupted.  */
> -  reply_port = &__hurd_local_reply_port;
> -  if (*reply_port)
> -    {
> -      mach_port_t port = *reply_port;
> -
> -      /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to
> -	 get another reply port, but avoids mig_dealloc_reply_port trying to
> -	 deallocate it after the receive fails (which it will, because the
> -	 reply port will be bogus, whether we do this or not).  */
> -      *reply_port = MACH_PORT_DEAD;
> -
> -      __mach_port_destroy (__mach_task_self (), port);
> -    }
> -  *reply_port = scp->sc_reply_port;
> +  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> +  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
> +  __mach_port_mod_refs (__mach_task_self (), reply_port,
> +                        MACH_PORT_RIGHT_RECEIVE, -1);
>  
>    if (scp->sc_fpused)
>      /* Restore the FPU state.  Mach conveniently stores the state
> -- 
> 2.39.2
>
  

On Tue, Apr 11, 2023 at 1:03 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> I tend to be very cautious with reply port reuse since it can confuse
> servers a lot when e.g. interrupted, so it's generally safer not to try
> to reuse them.

Huh? Confuse how?

For one thing, the servers are unable to even tell if two send-once
rights point to the same port (which is actually something that I'd
very much like to change, since it would greatly benefit rpctrace, but
that's a topic that deserves its own wall-of-text email). The servers
can still notice if their send-once right dies though.

If a call has been interrupted and we have not received a reply (EINTR
or not), we surely must destroy the reply port used, if only for
security reasons (so a malicious server won't be able to inject a fake
reply to some other RPC we make later). But I don't see a reason to
destroy the *current* reply port, the one used during signal handling,
which we're longjumping out of. On the other hand it doesn't hurt much
either, and signal handling is a slow path anyway, so we may just play
it safe.

Sergey
  

Sergey Bugaev, le mar. 11 avril 2023 10:44:17 +0300, a ecrit:
> On Tue, Apr 11, 2023 at 1:03 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > I tend to be very cautious with reply port reuse since it can confuse
> > servers a lot when e.g. interrupted, so it's generally safer not to try
> > to reuse them.
> 
> Huh? Confuse how?

On interruption, it's hard to know whether the server will actually send
a message on the reply port or not, in the end. So if you re-use a reply
port but unfortunately the server sent a reply on it in the end, you get
all reply messages shifted.

> But I don't see a reason to destroy the *current* reply port, the one
> used during signal handling, which we're longjumping out of.

Simplicity. I agree that it should work fine as of now. But then
somebody contributes something, doesn't notice that case, breaks it and
mayhem comes, but only rarely since signals don't happen often.

> On the other hand it doesn't hurt much either, and signal handling is
> a slow path anyway, so we may just play it safe.

Yes, please :)

Samuel
  

On Tue, Apr 11, 2023 at 11:15 PM Samuel Thibault
<samuel.thibault@gnu.org> wrote:
>
> Sergey Bugaev, le mar. 11 avril 2023 10:44:17 +0300, a ecrit:
> > On Tue, Apr 11, 2023 at 1:03 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > > I tend to be very cautious with reply port reuse since it can confuse
> > > servers a lot when e.g. interrupted, so it's generally safer not to try
> > > to reuse them.
> >
> > Huh? Confuse how?
>
> On interruption, it's hard to know whether the server will actually send
> a message on the reply port or not, in the end. So if you re-use a reply
> port but unfortunately the server sent a reply on it in the end, you get
> all reply messages shifted.

Yes, but this is confusing the client, not the server.

But I agree that we must destroy the port on which we have made an RFC
but have not received a reply. This isn't unique to interruptions,
it's a general thing with msg recv timing out.

Sergey
  

Hello,

Sergey Bugaev, le dim. 19 mars 2023 18:10:08 +0300, a ecrit:
> Also, use mach_port_mod_refs () and not mach_port_destroy () to destroy
> the receive right. mach_port_destroy () should *never* be used on
> mach_task_self (); this can easily lead to port use-after-free
> vulnerabilities if the task has any other references to the same port.

I had to revert the sigreturn part of this, it was making
signal/tst-signal signal/tst-raise signal/tst-minsigstksz-5
htl/tst-raise1 fail.

> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
> 
> NOTE: I don't really understand why sigunwind wants to destroy both its
> current reply port and user's reply port. Prior to commit
> fb304035c41c7ee2afede51e5e8568974549ba5e, it was *restoring* the user's
> reply port, in which case it actually made sense to destroy the current
> reply port. Post-fb304035c41c7ee2afede51e5e8568974549ba5e, wouldn't it
> be better to just keep using the current reply port, destroying the
> user's one?
> 
>  hurd/sigunwind.c                   | 24 +++++++++++-------------
>  sysdeps/mach/hurd/i386/sigreturn.c | 21 +++++----------------
>  2 files changed, 16 insertions(+), 29 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c
> index db1a01f3..29c9629f 100644
> --- a/sysdeps/mach/hurd/i386/sigreturn.c
> +++ b/sysdeps/mach/hurd/i386/sigreturn.c
> @@ -101,20 +100,10 @@ __sigreturn (struct sigcontext *scp)
>  
>    /* Destroy the MiG reply port used by the signal handler, and restore the
>       reply port in use by the thread when interrupted.  */
[...]
> -      __mach_port_destroy (__mach_task_self (), port);
> +  __mach_port_mod_refs (__mach_task_self (), reply_port,
> +                        MACH_PORT_RIGHT_RECEIVE, -1);