Re: [RFC PATCH 06/10] hurd: Make sure to not use tcb->self

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le mer. 17 mai 2023 22:14:32 +0300, a ecrit:
> Unlike sigstate->thread, tcb->self did not hold a Mach port reference on
> the thread port it names. This means that the port can be deallocated,
> and the name reused for something else, without anyone noticing. Using
> tcb->self will then lead to port use-after-free.
> 
> Fortunately nothing was accessing tcb->self, other than it being
> intially set to then-valid thread port name upon TCB initialization. To
> assert that this keeps being the case without altering TCB layout,
> rename self -> self_do_not_use, and stop initializing it.
> 
> Also, do not (re-)allocate a whole separate and unused stack for the
> main thread, and just exit __pthread_setup early in this case.
> 
> Found upon attempting to use tcb->self and getting unexpected crashes.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/i386/tls.h         |  3 +--
>  sysdeps/mach/hurd/x86/htl/pt-setup.c | 34 ++++++++++------------------
>  sysdeps/mach/hurd/x86_64/tls.h       |  3 +--
>  3 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/i386/tls.h b/sysdeps/mach/hurd/i386/tls.h
> index e124fb10..ba283008 100644
> --- a/sysdeps/mach/hurd/i386/tls.h
> +++ b/sysdeps/mach/hurd/i386/tls.h
> @@ -32,7 +32,7 @@ typedef struct
>  {
>    void *tcb;                 /* Points to this structure.  */
>    dtv_t *dtv;                        /* Vector of pointers to TLS data.  */
> -  thread_t self;             /* This thread's control port.  */
> +  thread_t self_do_not_use;  /* This thread's control port.  */
>    int multiple_threads;
>    uintptr_t sysinfo;
>    uintptr_t stack_guard;
> @@ -419,7 +419,6 @@ _hurd_tls_new (thread_t child, tcbhead_t *tcb)
>    HURD_TLS_DESC_DECL (desc, tcb);
>  
>    tcb->tcb = tcb;
> -  tcb->self = child;
>  
>    if (HURD_SEL_LDT (sel))
>      err = __i386_set_ldt (child, sel, &desc, 1);
> diff --git a/sysdeps/mach/hurd/x86/htl/pt-setup.c 
> b/sysdeps/mach/hurd/x86/htl/pt-setup.c
> index 3abd92b2..686124d7 100644
> --- a/sysdeps/mach/hurd/x86/htl/pt-setup.c
> +++ b/sysdeps/mach/hurd/x86/htl/pt-setup.c
> @@ -19,6 +19,7 @@
>  #include <stdint.h>
>  #include <assert.h>
>  #include <mach.h>
> +#include <hurd.h>
>  
>  #include <pt-internal.h>
>  
> @@ -76,35 +77,24 @@ __pthread_setup (struct __pthread *thread,
>                                     void *), void *(*start_routine) (void *),
>                void *arg)
>  {
> -  tcbhead_t *tcb;
>    error_t err;
> -  mach_port_t ktid;
>  
> -  thread->mcontext.pc = entry_point;
> -  thread->mcontext.sp = stack_setup (thread, start_routine, arg);
> -
> -  ktid = __mach_thread_self ();
> -  if (thread->kernel_thread == ktid)
> +  if (thread->kernel_thread == hurd_thread_self ())
>      /* Fix up the TCB for the main thread.  The C library has already
>         installed a TCB, which we want to keep using.  This TCB must not
>         be freed so don't register it in the thread structure.  On the
>         other hand, it's not yet possible to reliably release a TCB.
> -       Leave the unused one registered so that it doesn't leak.  The
> -       only thing left to do is to correctly set the `self' member in
> -       the already existing TCB.  */
> -    tcb = THREAD_SELF;
> -  else
> -    {
> -      err = __thread_set_pcsptp (thread->kernel_thread,
> -                              1, thread->mcontext.pc,
> -                              1, thread->mcontext.sp,
> -                              1, thread->tcb);
> -      assert_perror (err);
> -      tcb = thread->tcb;
> -    }
> -  __mach_port_deallocate (__mach_task_self (), ktid);
> +       Leave the unused one registered so that it doesn't leak.  */
> +    return 0;
> +
> +  thread->mcontext.pc = entry_point;
> +  thread->mcontext.sp = stack_setup (thread, start_routine, arg);
>  
> -  tcb->self = thread->kernel_thread;
> +  err = __thread_set_pcsptp (thread->kernel_thread,
> +                          1, thread->mcontext.pc,
> +                          1, thread->mcontext.sp,
> +                          1, thread->tcb);
> +  assert_perror (err);
>  
>    return 0;
>  }
> diff --git a/sysdeps/mach/hurd/x86_64/tls.h b/sysdeps/mach/hurd/x86_64/tls.h
> index 1274723a..35dcef44 100644
> --- a/sysdeps/mach/hurd/x86_64/tls.h
> +++ b/sysdeps/mach/hurd/x86_64/tls.h
> @@ -35,7 +35,7 @@ typedef struct
>  {
>    void *tcb;                 /* Points to this structure.  */
>    dtv_t *dtv;                        /* Vector of pointers to TLS data.  */
> -  thread_t self;             /* This thread's control port.  */
> +  thread_t self_do_no_use;   /* This thread's control port.  */
>    int __glibc_padding1;
>    int multiple_threads;
>    int gscope_flag;
> @@ -158,7 +158,6 @@ _hurd_tls_new (thread_t child, tcbhead_t *tcb)
>    struct i386_fsgs_base_state state;
>  
>    tcb->tcb = tcb;
> -  tcb->self = child;
>  
>    /* Install the TCB address into FS base.  */
>    state.fs_base = (uintptr_t) tcb;
> -- 
> 2.40.1
> 
> 

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