public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Sergey Bugaev <bugaevc@gmail.com>
To: libc-alpha@sourceware.org, bug-hurd@gnu.org
Subject: [RFC PATCH 06/10] hurd: Make sure to not use tcb->self
Date: Wed, 17 May 2023 22:14:32 +0300	[thread overview]
Message-ID: <20230517191436.73636-7-bugaevc@gmail.com> (raw)
In-Reply-To: <20230517191436.73636-1-bugaevc@gmail.com>

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


  parent reply	other threads:[~2023-05-17 19:14 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-17 19:14 [PATCH 00/10] Stack setup & misc fixes for x86_64-gnu Sergey Bugaev
2023-05-17 19:14 ` [PATCH 01/10] Remove sysdeps/generic/thread_state.h Sergey Bugaev
2023-05-17 20:50   ` Samuel Thibault
2023-05-17 19:14 ` [PATCH 02/10] mach: Define MACHINE_THREAD_STATE_SETUP_CALL Sergey Bugaev
2023-05-17 20:52   ` Samuel Thibault
2023-05-17 19:14 ` [PATCH 03/10] hurd: Use MACHINE_THREAD_STATE_SETUP_CALL Sergey Bugaev
2023-05-17 20:52   ` [PATCH 03/10] hurd: Use MACHINE_THREAD_STATE_SETUP_CALLo Samuel Thibault
2023-05-17 19:14 ` [PATCH 04/10] mach: Add __mach_setup_thread_call () Sergey Bugaev
2023-05-17 20:56   ` Samuel Thibault
2023-05-17 19:14 ` [PATCH 05/10] hurd: Use " Sergey Bugaev
2023-05-17 20:57   ` Samuel Thibault
2023-05-17 19:14 ` Sergey Bugaev [this message]
2023-05-17 20:59   ` [RFC PATCH 06/10] hurd: Make sure to not use tcb->self Samuel Thibault
2023-05-18 18:55     ` Joseph Myers
2023-05-18 19:33       ` Sergey Bugaev
2023-05-18 20:16         ` Joseph Myers
2023-05-18 23:47           ` Samuel Thibault
2023-05-19  8:22           ` Sergey Bugaev
2023-05-19  9:39             ` Florian Weimer
2023-05-19 16:50             ` Joseph Myers
2023-05-19 14:47           ` [PATCH] hurd: Fix using interposable hurd_thread_self Sergey Bugaev
2023-05-19 18:57             ` Samuel Thibault
2023-05-17 19:14 ` [PATCH 07/10] hurd: Fix x86_64 _hurd_tls_fork Sergey Bugaev
2023-05-17 21:01   ` Samuel Thibault
2023-05-17 19:14 ` [PATCH 08/10] hurd: Fix setting up pthreads Sergey Bugaev
2023-05-17 21:02   ` Samuel Thibault
2023-05-17 19:14 ` [PATCH 09/10] hurd: Also make it possible to call strlen very early Sergey Bugaev
2023-05-17 21:04   ` Samuel Thibault
2023-05-17 19:14 ` [RFC PATCH 10/10] hurd: Regenerate errno.h Sergey Bugaev
2023-05-17 19:39   ` Joseph Myers
2023-05-17 21:04     ` Samuel Thibault

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230517191436.73636-7-bugaevc@gmail.com \
    --to=bugaevc@gmail.com \
    --cc=bug-hurd@gnu.org \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).