From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id A904E3858D1E for ; Wed, 17 May 2023 20:59:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A904E3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gnu.org Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pzOFZ-0003oP-Ay; Wed, 17 May 2023 16:59:57 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=In-Reply-To:MIME-Version:References:Subject:To:From: Date; bh=hOlFe5aCAaf6N//yyW6TN3ROzwPMwzX/EGNLRcpO8BA=; b=jiK+rs69Wf2sdyL6b8EB 2SZla8QsmUSHWqaZXz2EhdM36pO2SaNIc/nXjo3CodWhHwwOmWy0LiTtoDNIqlRM5G//N0RZCVYNU 8iaBaSekiFKSk3tCeE24EIpjWWjEcUXyF2pIgtMc/lQ38+WXJeOkbwuPzYcd79J4rVZ3Wxv+iieYT 4cMhgO7yUfrI8erDhs3ZxMm7VnWfbichYSiMSLpvHWuFaOPS/38ufkt8Me0N7VPoJJbmQ5dCFqqoF sNCkcVTQkjfwlJ/7cdJMrQ1E+4AYi+OohaszfEoGv1gIIjVq/Xn7J81umkAJuecxOqlSZEpEHz5St Lv3dKKdbuQTd1A==; Received: from 2a01cb008c251f00de41a9fffe47ec49.ipv6.abo.wanadoo.fr ([2a01:cb00:8c25:1f00:de41:a9ff:fe47:ec49] helo=begin.home) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pzOFZ-0000W5-4M; Wed, 17 May 2023 16:59:57 -0400 Received: from samy by begin.home with local (Exim 4.96) (envelope-from ) id 1pzOFX-006LS7-1J; Wed, 17 May 2023 22:59:55 +0200 Date: Wed, 17 May 2023 22:59:55 +0200 From: Samuel Thibault To: Sergey Bugaev Cc: libc-alpha@sourceware.org, bug-hurd@gnu.org Subject: Re: [RFC PATCH 06/10] hurd: Make sure to not use tcb->self Message-ID: <20230517205955.xb53s7fl5exydk2z@begin> Mail-Followup-To: Sergey Bugaev , libc-alpha@sourceware.org, bug-hurd@gnu.org References: <20230517191436.73636-1-bugaevc@gmail.com> <20230517191436.73636-7-bugaevc@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230517191436.73636-7-bugaevc@gmail.com> Organization: I am not organized User-Agent: NeoMutt/20170609 (1.8.3) X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > --- > 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 > #include > #include > +#include > > #include > > @@ -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.