From: Samuel Thibault <samuel.thibault@gnu.org>
To: Sergey Bugaev <bugaevc@gmail.com>
Cc: libc-alpha@sourceware.org, bug-hurd@gnu.org
Subject: Re: [RFC PATCH 06/10] hurd: Make sure to not use tcb->self
Date: Wed, 17 May 2023 22:59:55 +0200 [thread overview]
Message-ID: <20230517205955.xb53s7fl5exydk2z@begin> (raw)
In-Reply-To: <20230517191436.73636-7-bugaevc@gmail.com>
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.
next prev parent reply other threads:[~2023-05-17 20:59 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 ` [RFC PATCH 06/10] hurd: Make sure to not use tcb->self Sergey Bugaev
2023-05-17 20:59 ` Samuel Thibault [this message]
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=20230517205955.xb53s7fl5exydk2z@begin \
--to=samuel.thibault@gnu.org \
--cc=bug-hurd@gnu.org \
--cc=bugaevc@gmail.com \
--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).