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

  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(-)
  

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
> 
>
  

I suspect this of causing linknamespace test failures:

Contents of conform/POSIX2008/pthread.h/linknamespace.out:

[initial] pthread_create -> [libpthread.a(pt-create.o)] __pthread_setup -> [libpthread.a(pt-setup.o)] hurd_thread_self

(On x86_64 there's also a localplt test failure: "Extra PLT reference: 
libc.so: hurd_thread_self".  In addition, x86_64 has a c++-types-check 
failure.  Thus, neither Hurd configuration has clean results for 
compilation tests from build-many-glibcs.py at present.)
  

Hello,

On Thu, May 18, 2023 at 9:55 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> I suspect this of causing linknamespace test failures:
>
> Contents of conform/POSIX2008/pthread.h/linknamespace.out:
>
> [initial] pthread_create -> [libpthread.a(pt-create.o)] __pthread_setup -> [libpthread.a(pt-setup.o)] hurd_thread_self
>
> (On x86_64 there's also a localplt test failure: "Extra PLT reference:
> libc.so: hurd_thread_self".  In addition, x86_64 has a c++-types-check
> failure.  Thus, neither Hurd configuration has clean results for
> compilation tests from build-many-glibcs.py at present.)

Thank you, and sorry :|

Do I understand it right that this is because of hurd_thread_self
being publicly exported and interposable? A quick grep shows that
nothing else inside glibc was using hurd_thread_self indeed.

Would the right way to resolve this be introducing a hidden
__hurd_thread_self and using that? We could also make it
__mach_thread_self () + __mach_port_deallocate (), but why do +2
syscalls when we already have all the required info in userspace.

What's the C++ type check failure? Surely this patch (nor
2f8ecb58a59eb82c43214d000842d99644a662d1 "hurd: Fix x86_64
_hurd_tls_fork") has modified any public headers?

Sergey
  

On Thu, 18 May 2023, Sergey Bugaev via Libc-alpha wrote:

> Hello,
> 
> On Thu, May 18, 2023 at 9:55 PM Joseph Myers <joseph@codesourcery.com> wrote:
> >
> > I suspect this of causing linknamespace test failures:
> >
> > Contents of conform/POSIX2008/pthread.h/linknamespace.out:
> >
> > [initial] pthread_create -> [libpthread.a(pt-create.o)] __pthread_setup -> [libpthread.a(pt-setup.o)] hurd_thread_self
> >
> > (On x86_64 there's also a localplt test failure: "Extra PLT reference:
> > libc.so: hurd_thread_self".  In addition, x86_64 has a c++-types-check
> > failure.  Thus, neither Hurd configuration has clean results for
> > compilation tests from build-many-glibcs.py at present.)
> 
> Thank you, and sorry :|
> 
> Do I understand it right that this is because of hurd_thread_self
> being publicly exported and interposable? A quick grep shows that
> nothing else inside glibc was using hurd_thread_self indeed.
> 
> Would the right way to resolve this be introducing a hidden
> __hurd_thread_self and using that? We could also make it

Yes.

Strictly there are two separate issues: (a) calling an exported symbol 
should be done via a hidden alias, to avoid going via the PLT, and (b) 
functions in a standard namespace should not call names in the user's 
namespace, which requires using a name such as __hurd_thread_self instead 
(which should also be marked hidden to make the call optimally efficient).

> What's the C++ type check failure? Surely this patch (nor
> 2f8ecb58a59eb82c43214d000842d99644a662d1 "hurd: Fix x86_64
> _hurd_tls_fork") has modified any public headers?

The C++ type check failure was already present before this patch.

--- sysdeps/mach/hurd/x86_64/c++-types.data     2023-05-02 09:14:30.246903708 +0000
+++ -   2023-05-18 02:08:06.184068438 +0000
@@ -1 +1 @@
-blkcnt64_t:x
+blkcnt64_t:l
@@ -8 +8 @@
-dev_t:j
+dev_t:m
@@ -10 +10 @@
-fsblkcnt64_t:y
+fsblkcnt64_t:m
@@ -12 +12 @@
-fsfilcnt64_t:y
+fsfilcnt64_t:m
@@ -14 +14 @@
-fsid_t:y
+fsid_t:m
@@ -17 +17 @@
-ino64_t:y
+ino64_t:m
@@ -21 +21 @@
-int64_t:x
+int64_t:l
@@ -23 +23 @@
-intptr_t:i
+intptr_t:l
@@ -25 +25 @@
-loff_t:x
+loff_t:l
@@ -27,2 +27,2 @@
-nlink_t:j
-off64_t:x
+nlink_t:m
+off64_t:l
@@ -43,4 +43,4 @@
-pthread_t:i
-quad_t:x
-register_t:i
-rlim64_t:y
+pthread_t:l
+quad_t:l
+register_t:l
+rlim64_t:m
@@ -49 +49 @@
-size_t:j
+size_t:m
@@ -51 +51 @@
-ssize_t:i
+ssize_t:l
@@ -60 +60 @@
-u_int64_t:y
+u_int64_t:m
@@ -64 +64 @@
-u_quad_t:y
+u_quad_t:m
  

Joseph Myers, le jeu. 18 mai 2023 20:16:03 +0000, a ecrit:
> The C++ type check failure was already present before this patch.
> 
> --- sysdeps/mach/hurd/x86_64/c++-types.data     2023-05-02 09:14:30.246903708 +0000
> +++ -   2023-05-18 02:08:06.184068438 +0000
> @@ -1 +1 @@
> -blkcnt64_t:x
> +blkcnt64_t:l

etc.

Uh, I'm confused. I do remember seeing a c++-types test failing and thus
adding the c++ data file, but somehow apparently only added the 32bit
one. And I'm not actually seeing the c++-types failure in my test box,
thus it went unnoticed... I commited the update to 64, thanks!

Samuel
  

On Thu, May 18, 2023 at 11:16 PM Joseph Myers <joseph@codesourcery.com> wrote:
> Strictly there are two separate issues: (a) calling an exported symbol
> should be done via a hidden alias, to avoid going via the PLT, and (b)
> functions in a standard namespace should not call names in the user's
> namespace, which requires using a name such as __hurd_thread_self instead
> (which should also be marked hidden to make the call optimally efficient).

While we're talking about this, could you please say if my
understanding below is correct (and correct me if not)?

'foo' is a public symbol, to be used by the user, and also
interposable by the user. Always called via PLT (except from inside
the user's code when redefined inside the executable, in which case
the compiler/linker will see that no PLT is needed), and should not be
called from inside glibc.

'__foo' is a (public? private? semi-private?) symbol, the user code is
not supposed to use it, but it's exported from libc.so for the benefit
of other glibc code that ends up in different DSOs and still wants to
call the original version even when 'foo' gets interposed. Invoked via
PLT from other DSOs, not invoked from libc.so because of...

'__GI___foo' is a private non-exported symbol created by the
hidden_def/hidden_proto macro being used on '__foo', this is what the
code in libc.so (or: whatever DSO the symbol is hidden_def'ed in)
calls to avoid PLT jumps.

Q: why '__foo', why not use hidden_def/hidden_proto on the 'foo' directly?
A: that would only work for code that ends up in libc.so (or rather,
the same DSOs as 'foo'), but we still want other code to also call the
non-interposed, original version of 'foo'

Is that ^^ correct? Should each and every function that is exported to
the user and also used inside libc's own code have '__foo' and
'__GI___foo' versions? What does 'GI' even stand for?

Thanks in advance,
Sergey
  

* Sergey Bugaev:

> On Thu, May 18, 2023 at 11:16 PM Joseph Myers <joseph@codesourcery.com> wrote:
>> Strictly there are two separate issues: (a) calling an exported symbol
>> should be done via a hidden alias, to avoid going via the PLT, and (b)
>> functions in a standard namespace should not call names in the user's
>> namespace, which requires using a name such as __hurd_thread_self instead
>> (which should also be marked hidden to make the call optimally efficient).
>
> While we're talking about this, could you please say if my
> understanding below is correct (and correct me if not)?
>
> 'foo' is a public symbol, to be used by the user, and also
> interposable by the user. Always called via PLT (except from inside
> the user's code when redefined inside the executable, in which case
> the compiler/linker will see that no PLT is needed), and should not be
> called from inside glibc.
>
> '__foo' is a (public? private? semi-private?) symbol, the user code is
> not supposed to use it, but it's exported from libc.so for the benefit
> of other glibc code that ends up in different DSOs and still wants to
> call the original version even when 'foo' gets interposed. Invoked via
> PLT from other DSOs, not invoked from libc.so because of...

__foo may be exported or not.  We have some __ symbols that are used by
the implementation (so GCC etc.), and probably some that are expected to
be used by users.  Truely exported symbols have a GLIBC_2.y symbol
version, internal exported symbols (usually added for coordination
between different shared objects that make up glibc) have a
GLIBC_PRIVATE symbol version.

Some old internal symbols have GLIBC_2.0 or similar early versions
because GLIBC_PRIVATE did not exist then.

> '__GI___foo' is a private non-exported symbol created by the
> hidden_def/hidden_proto macro being used on '__foo', this is what the
> code in libc.so (or: whatever DSO the symbol is hidden_def'ed in)
> calls to avoid PLT jumps.

Correct.

> Q: why '__foo', why not use hidden_def/hidden_proto on the 'foo' directly?
> A: that would only work for code that ends up in libc.so (or rather,
> the same DSOs as 'foo'), but we still want other code to also call the
> non-interposed, original version of 'foo'

hidden_def/hidden_proto does not do anything for static linking, given
the macros are defined today.

It's of course possible to do all this quite differently.

Thanks,
Florian
  

On Fri, 19 May 2023, Sergey Bugaev via Libc-alpha wrote:

> 'foo' is a public symbol, to be used by the user, and also
> interposable by the user. Always called via PLT (except from inside
> the user's code when redefined inside the executable, in which case
> the compiler/linker will see that no PLT is needed), and should not be
> called from inside glibc.

Should not be called from within glibc via the PLT within the same 
library.

It's OK for foo to be called from bar if foo is part of all the standards 
that contain bar.  But in that case, if the call is from the same library, 
*_hidden_def / *_hidden_proto should be used to avoid calling via the PLT.

If foo is not part of all the standards that contain bar, then glibc code 
for bar should use __foo instead to avoid namespace issues, especially for 
static linking.

If __foo is needed by executables, *_nonshared.a or other shared 
libraries, then it also needs to be exported from the library it's in (in 
which case PLT avoidance is also relevant for __foo, so *_hidden_* should 
be used for __foo).

If __foo is only used within a single library and doesn't need to be 
exported, it's OK to declare it directly with attribute_hidden rather than 
using *_hidden_* to create __GI___foo as an alias.  (In this case, if you 
forget to use attribute_hidden, you won't get test failures - the symbol 
in fact won't get exported, because only symbols explicitly listed in the 
version maps get exported, and so it won't get a PLT entry.  But in some 
cases, code generation is more efficient if the compiler knows that the 
symbol is hidden.  So when you're calling an unexported symbol in the same 
library, it's still desirable for it to be declared as hidden in a way 
visible to the compiler.)  The more complicated hidden_proto / hidden_def 
approach also works for unexported symbols used within a single library, 
it's just more complicated than necessary in that case.