Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Adhemerval Zanella Netto]

On 19/04/23 18:16, Sergey Bugaev wrote:
> On Wed, Apr 19, 2023 at 11:45 PM Adhemerval Zanella Netto
 is this really needed now? playing silly games with this fds will always 
 result in silly prices.
>>
>> My understanding of this code is to enforce that on setuid program with
>> stdin/stdout/stderr closed any operation fail.
> 
> Yes, but is that still considered desirable / a good idea? As opposed
> to making such operations no-op successfully (opening /dev/null with
> the expected mode).
> 

Good question, this is essentially a hardening for setsuid binaries since
opening the file in the *expected* way is not the intended behavior (even
though the C runtime expects that STDIN_FILENO, STDOUT_FILENO, and
STDERR_FILENO are in fact opened). As far I could check, this is really a 
glibc extension (both FreeBSD and OpenBSD does not seem to add such 
hardening).

I am not really sure how effective is this hardening, it seems more a
development one to enforce that system daemon are spawned correctly.

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

On Wed, Apr 19, 2023 at 11:52 PM Sergey Bugaev  wrote:
> On Wed, Apr 19, 2023 at 10:48 PM Luca Dariz  wrote:
> > * i386/i386/pcb.c: switch FSBASE/GSBASE on context switch and
> >   implement accessors in thread setstatus/getstatus
> > * i386/i386/thread.h: add new state to thread saved state
> > * kern/thread.c: add i386_FSGS_BASE_STATE handler
>
> Hi Luca -- this is great!
>
> I will build gnumach with your changes and see if that works with my
> latest build of x86_64-gnu glibc.

Well, too bad -- as is, we don't even get to actually doing the
thread_set_state () RPC.

We do reach the call to __thread_set_state (), but then it uses
__mig_memcpy (), which is just 'return memcpy (...)'. And (because of
the static build?), that memcpy () is the full real ifunc-selected
memcpy. So it jumps to the memcpy@plt, which jumps to
*(mem...@got.plt), which is supposed to jump into the rtld and then
run the ifunc resolver and do its smarts and eventually jump to the
right memcpy...

But of course none of that is happening because we haven't really
started running any of rtld proper yet, we're still inside
_hurd_stack_setup (). So there was no-one to apply the relocations to
our .got.plt yet, and they point to who knows where. (In practice,
back to the PLT.)

Why had this worked for me when I was running it on Linux? Most likely
I have skipped over the whole __thread_set_state () call and not just
the 'syscall'. Why was this not an issue for us on i386? Two reasons:

* i386_set_gdt () doesn't need memcpy;
* memcpy is not ifunc-resolved in static i386

We can't run libc_start_main () (which is what calls
ARCH_INIT_CPU_FEATURES and then _dl_relocate_static_pie) before we
get our argv/envp and our new stack, but to do that we need MIG, and
MIG will memcpy...

So how do we get out of this chicken-and-egg situation? Well, like
this of course:

leaq __memcpy_sse2_unaligned(%rip), %rax
movq %rax, memcpy@GOTPCREL(%rip)

Call that either a terrible hack or a brilliant solution :) :) :)
and I'm sure H.J. Lu over on libc-alpha will appreciate it when I
send the patch. But -- it works! I can get through memcpy now, and
__thread_set_state () succeeds and writes $fs_base as seen from GDB!
So, great work on this!

Now, this exposed some of my own bugs in glibc... and after quickly
fixing them, I'm now stuck at trying to allocate memory. Specifically,
I'd like to have memory at 0x2000 and some further, but
gnumach is not letting me! -- because that's more than
VM_MAX_USER_ADDRESS, which is defined to VM_MAX_ADDRESS, which is then
defined to 0xc000, same as on i386. That's of course too small.
Can we bump this substantially?

Sergey

Re: [PATCH 5/5] add setting gs/fsbase

[Samuel Thibault]

Hello,

Sergey Bugaev, le jeu. 20 avril 2023 14:51:04 +0300, a ecrit:
> Why was this not an issue for us on i386?

See 56010b73e81e2cb1082e418699f98353598fe671 and its __mig_memcpy.

> VM_MAX_USER_ADDRESS, which is defined to VM_MAX_ADDRESS, which is then
> defined to 0xc000, same as on i386. That's of course too small.
> Can we bump this substantially?

Of course !

Samuel

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

On Thu, Apr 20, 2023 at 2:56 PM Samuel Thibault  wrote:
>
> Hello,
>
> Sergey Bugaev, le jeu. 20 avril 2023 14:51:04 +0300, a ecrit:
> > Why was this not an issue for us on i386?
>
> See 56010b73e81e2cb1082e418699f98353598fe671 and its __mig_memcpy.

Interesting; but that one's dealing with the SHARED case, isn't it? In
my case everything's statically linked so circular relocations between
libc and lib*user aren't an issue; but on the other hand we need to
run __thread_set_state (and other MIG routines) super early, unlike in
SHARED.

> > VM_MAX_USER_ADDRESS, which is defined to VM_MAX_ADDRESS, which is then
> > defined to 0xc000, same as on i386. That's of course too small.
> > Can we bump this substantially?
>
> Of course !

Let me actually try doing #define VM_MAX_ADDRESS KENEL_MAP_BASE...

Sergey

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Cristian Rodríguez]

On Thu, Apr 20, 2023 at 7:47 AM Adhemerval Zanella Netto <
adhemerval.zane...@linaro.org> wrote:

>
>
>
> I am not really sure how effective is this hardening, it seems more a
> development one to enforce that system daemon are spawned correctly.
>

Exactly, my understanding is that it is a futile exercise ..if one
sufficient privilege at that stage one can do whatever is desired..  why
even bother messing with the standard fds..

Re: [PATCH 5/5] add setting gs/fsbase

[Samuel Thibault]

Sergey Bugaev, le jeu. 20 avril 2023 15:05:25 +0300, a ecrit:
> On Thu, Apr 20, 2023 at 2:56 PM Samuel Thibault  
> wrote:
> >
> > Hello,
> >
> > Sergey Bugaev, le jeu. 20 avril 2023 14:51:04 +0300, a ecrit:
> > > Why was this not an issue for us on i386?
> >
> > See 56010b73e81e2cb1082e418699f98353598fe671 and its __mig_memcpy.
> 
> Interesting; but that one's dealing with the SHARED case, isn't it?

Yes but I guess it fixed the static case too?

> > > VM_MAX_USER_ADDRESS, which is defined to VM_MAX_ADDRESS, which is then
> > > defined to 0xc000, same as on i386. That's of course too small.
> > > Can we bump this substantially?
> >
> > Of course !
> 
> Let me actually try doing #define VM_MAX_ADDRESS KENEL_MAP_BASE...

It doesn't need to be that high :)

And actually it probably cannot: IIRC not all bits can be used in amd64
virtual addresses anyway.

Samuel

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

On Thu, Apr 20, 2023 at 3:08 PM Samuel Thibault  wrote:
> > > See 56010b73e81e2cb1082e418699f98353598fe671 and its __mig_memcpy.
> >
> > Interesting; but that one's dealing with the SHARED case, isn't it?
>
> Yes but I guess it fixed the static case too?

We must be talking past each other -- evidently it did not fix the
static case, because I'm running into that issue now. On x86_64, that
is; while on i386 it is working for the two reasons I listed.

Moreover, I don't see how it could have changed anything for the
static case. For the shared case, yes, lib*user will now reference
__mig_memcpy from libc.so, and that one needs a simple relocation
without ifunc. For the static case, it's all in the same binary; the
only relocations left after static linking are the ifunc relocations,
and the issue I'm having is different (and happens at an earlier
stage).

Or in other words: the issue I am/was running into was not that ifunc
resolvers wouldn't work because of yet unrelocated _rtld_global_ro or
some such, but that they aren't getting called in the first place.
Instead we fetch some garbage pointers from the GOT *before* they are
initialized by calling the ifunc resolvers, and jump there and
eventually crash. (Or are they not garbage? It's suspicious that they
really point back to the PLT.)

> > > > VM_MAX_USER_ADDRESS, which is defined to VM_MAX_ADDRESS, which is then
> > > > defined to 0xc000, same as on i386. That's of course too small.
> > > > Can we bump this substantially?
> > >
> > > Of course !
> >
> > Let me actually try doing #define VM_MAX_ADDRESS KENEL_MAP_BASE...
>
> It doesn't need to be that high :)
>
> And actually it probably cannot: IIRC not all bits can be used in amd64
> virtual addresses anyway.

Yes, but apparently it still works for the kernel? Or is that some
other kind of address?

What would be a correct value then, 1 << 48?

Sergey

Re: [PATCH 5/5] add setting gs/fsbase

[Samuel Thibault]

Sergey Bugaev, le jeu. 20 avril 2023 15:27:15 +0300, a ecrit:
> On Thu, Apr 20, 2023 at 3:08 PM Samuel Thibault  
> wrote:
> > > > See 56010b73e81e2cb1082e418699f98353598fe671 and its __mig_memcpy.
> > >
> > > Interesting; but that one's dealing with the SHARED case, isn't it?
> >
> > Yes but I guess it fixed the static case too?
> 
> For the shared case, yes, lib*user will now reference
> __mig_memcpy from libc.so, and that one needs a simple relocation
> without ifunc.

Ah, I missed the part that said that __mig_memcpy just calls the
(ifunc-) memcpy.

I guess I shouldn't be replying when I don't actually have time to check
all ins and outs of what I'm talking about :)

> > > > > VM_MAX_USER_ADDRESS, which is defined to VM_MAX_ADDRESS, which is then
> > > > > defined to 0xc000, same as on i386. That's of course too small.
> > > > > Can we bump this substantially?
> > > >
> > > > Of course !
> > >
> > > Let me actually try doing #define VM_MAX_ADDRESS KENEL_MAP_BASE...
> >
> > It doesn't need to be that high :)
> >
> > And actually it probably cannot: IIRC not all bits can be used in amd64
> > virtual addresses anyway.
> 
> Yes, but apparently it still works for the kernel? Or is that some
> other kind of address?

IIRC the last really-usable bit of the virtual address is just
replicated (to 0 for user and to 1 for kernel).

> What would be a correct value then, 1 << 48?

I don't know by heart, but something like that yes.

Samuel

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

On Thu, Apr 20, 2023 at 3:33 PM Samuel Thibault  wrote:
> Sergey Bugaev, le jeu. 20 avril 2023 15:27:15 +0300, a ecrit:
> > On Thu, Apr 20, 2023 at 3:08 PM Samuel Thibault  
> > wrote:
> > > > > See 56010b73e81e2cb1082e418699f98353598fe671 and its __mig_memcpy.
> > > >
> > > > Interesting; but that one's dealing with the SHARED case, isn't it?
> > >
> > > Yes but I guess it fixed the static case too?
> >
> > For the shared case, yes, lib*user will now reference
> > __mig_memcpy from libc.so, and that one needs a simple relocation
> > without ifunc.
>
> Ah, I missed the part that said that __mig_memcpy just calls the
> (ifunc-) memcpy.
>
> I guess I shouldn't be replying when I don't actually have time to check
> all ins and outs of what I'm talking about :)

It's alright, I'm guilty of that too :) And also indeed I have just
spent hours debugging this, so I have some mental model of what's
going on (whether a correct one is another question...), but if you
haven't looked into this for some time, you wouldn't.

> > What would be a correct value then, 1 << 48?
>
> I don't know by heart, but something like that yes.

Well that value is making gnumach crash before the task even runs :(

XNU has 0x7000, which is still larger than 0x2000, so
I'm going to try that one. Though they probably mean 0x8000
and have just left a single page of address space for the commpage.

Sergey

Re: [PATCH 5/5] add setting gs/fsbase

[Luca]

Il 20 aprile 2023 12:46:51 UTC, Sergey Bugaev  ha scritto:
>On Thu, Apr 20, 2023 at 3:33 PM Samuel Thibault  
>wrote:
>> Sergey Bugaev, le jeu. 20 avril 2023 15:27:15 +0300, a ecrit:
>> > On Thu, Apr 20, 2023 at 3:08 PM Samuel Thibault  
>> > wrote:
>> > > > > See 56010b73e81e2cb1082e418699f98353598fe671 and its __mig_memcpy.
>> > > >
>> > > > Interesting; but that one's dealing with the SHARED case, isn't it?
>> > >
>> > > Yes but I guess it fixed the static case too?
>> >
>> > For the shared case, yes, lib*user will now reference
>> > __mig_memcpy from libc.so, and that one needs a simple relocation
>> > without ifunc.
>>
>> Ah, I missed the part that said that __mig_memcpy just calls the
>> (ifunc-) memcpy.
>>
>> I guess I shouldn't be replying when I don't actually have time to check
>> all ins and outs of what I'm talking about :)
>
>It's alright, I'm guilty of that too :) And also indeed I have just
>spent hours debugging this, so I have some mental model of what's
>going on (whether a correct one is another question...), but if you
>haven't looked into this for some time, you wouldn't.
>
>> > What would be a correct value then, 1 << 48?
>>
>> I don't know by heart, but something like that yes.
>
>Well that value is making gnumach crash before the task even runs :(
>
>XNU has 0x7000, which is still larger than 0x2000, so
>I'm going to try that one. Though they probably mean 0x8000
>and have just left a single page of address space for the commpage.

There is likely some limitation in the pmap module in gnumach, IIRC it should 
statically allocate the user L3 page tables as long as the limit is below the 
first 512GB, but I never really tested it yet. Would it work for you to shrink 
temporarily the VM space, at least for testing?

Luxa

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

On Thu, Apr 20, 2023 at 4:01 PM Luca  wrote:
> There is likely some limitation in the pmap module in gnumach, IIRC it should 
> statically allocate the user L3 page tables as long as the limit is below the 
> first 512GB, but I never really tested it yet.

That sounds plausible, yes, I've seen some sort of TODOs/FIXMEs
referencing L3 page tables while grepping for VM_MAX_USER_ADDRESS. Not
that I have any clue how any of this works.

Please look into this!

> Would it work for you to shrink temporarily the VM space, at least for 
> testing?

Yeah, that's also what I've been thinking to try out next. I'm
somewhat surprised that gcc/ld placed my static executable in the
rather low part of the address space, but here it actually works in
our favor.

Is that actually configurable somewhere (at gcc/binutils build time, I
mean)? I'm thinking we will want to map the lower 4GB of address space
inaccessible like dyld does, and not just one page, to catch pointer
truncation in addition to nullptr dereferences. But that would need
gcc/ld to know to not use low addresses for their needs.

Sergey

Re: [PATCH 5/5] add setting gs/fsbase

[Samuel Thibault]

Sergey Bugaev, le jeu. 20 avril 2023 16:18:29 +0300, a ecrit:
> On Thu, Apr 20, 2023 at 4:08 PM Sergey Bugaev  wrote:
> > > Would it work for you to shrink temporarily the VM space, at least for 
> > > testing?
> >
> > Yeah, that's also what I've been thinking to try out next. I'm
> > somewhat surprised that gcc/ld placed my static executable in the
> > rather low part of the address space, but here it actually works in
> > our favor.
> 
> It's alive

!! \o/ !!

Re: [PATCH 5/5] add setting gs/fsbase

[Luca]

Il 20 aprile 2023 13:18:29 UTC, Sergey Bugaev  ha scritto:
>On Thu, Apr 20, 2023 at 4:08 PM Sergey Bugaev  wrote:
>> > Would it work for you to shrink temporarily the VM space, at least for 
>> > testing?
>>
>> Yeah, that's also what I've been thinking to try out next. I'm
>> somewhat surprised that gcc/ld placed my static executable in the
>> rather low part of the address space, but here it actually works in
>> our favor.
>
>It's alive

Wow!!!

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

FWIW, mach-bootstrap-hello is supposed to then echo back what you type
on the console, but it doesn't work. From some debugging, it seems
that once I input something, the following device_write () call hangs
here:

(gdb)
ast_taken () at ../kern/ast.c:88
88 if (reasons & AST_NETWORK)
(gdb)
96 if (self != current_processor()->idle_thread) {
(gdb) n
98 while (thread_should_halt(self))
(gdb) l
93 * or thread_block from the idle thread.
94 */
95
96 if (self != current_processor()->idle_thread) {
97 #ifndef MIGRATING_THREADS
98 while (thread_should_halt(self))
99 thread_halt_self(thread_exception_return);
100 #endif
101
102 /*
(gdb) bt
#0  ast_taken () at ../kern/ast.c:98
#1  0x81011b8e in _return_from_trap () at ../x86_64/locore.S:571
#2  0x9f442868 in ?? ()
#3  0x81018a70 in ?? () at ../kern/sched_prim.c:924
#4  0x9f43ffb8 in ?? ()
#5  0x in ?? ()

I don't know what to make of that, but perhaps you will. Note that
again, this worked on i386.

Sergey

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Adhemerval Zanella Netto]

On 20/04/23 09:06, Cristian Rodríguez wrote:
> 
> 
> On Thu, Apr 20, 2023 at 7:47 AM Adhemerval Zanella Netto 
> mailto:adhemerval.zane...@linaro.org>> wrote:
> 
> 
> 
> 
> I am not really sure how effective is this hardening, it seems more a
> development one to enforce that system daemon are spawned correctly.
> 
> 
> Exactly, my understanding is that it is a futile exercise ..if one sufficient 
> privilege at that stage one can do whatever is desired..  why even bother 
> messing with the standard fds..

I don't have a strong opinion, but I tend to agree that this hardening does
not add much specially now that we have a lot of granular ways to limit 
process execution (such as capabilities, seccomp, etc.).

[PATCH 1/2] hurd: Don't migrate reply port into __init1_tcbhead

[Sergey Bugaev]

Properly differentiate between setting up the real TLS with
TLS_INIT_TP, and setting up the early TLS (__init1_tcbhead) in static
builds. In the latter case, don't yet migrate the reply port into the
TCB, and don't yet set __libc_tls_initialized to 1.

This also lets us move the __init1_desc assignment inside
_hurd_tls_init ().

Fixes cd019ddd892e182277fadd6aedccc57fa3923c8d
"hurd: Don't leak __hurd_reply_port0"

Signed-off-by: Sergey Bugaev 
---
 sysdeps/mach/hurd/i386/tls.h   | 18 --
 sysdeps/mach/hurd/x86/init-first.c |  7 +--
 sysdeps/mach/hurd/x86_64/tls.h | 11 ++-
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/sysdeps/mach/hurd/i386/tls.h b/sysdeps/mach/hurd/i386/tls.h
index 0ac8917a..e124fb10 100644
--- a/sysdeps/mach/hurd/i386/tls.h
+++ b/sysdeps/mach/hurd/i386/tls.h
@@ -136,7 +136,7 @@ __LIBC_NO_TLS (void)
special attention since 'errno' is not yet available and if the
operation can cause a failure 'errno' must not be touched.  */
 static inline bool __attribute__ ((unused))
-_hurd_tls_init (tcbhead_t *tcb)
+_hurd_tls_init (tcbhead_t *tcb, bool full)
 {
   HURD_TLS_DESC_DECL (desc, tcb);
   thread_t self = __mach_thread_self ();
@@ -148,8 +148,9 @@ _hurd_tls_init (tcbhead_t *tcb)
   tcb->tcb = tcb;
   /* We always at least start the sigthread anyway.  */
   tcb->multiple_threads = 1;
-  /* Take over the reply port we've been using.  */
-  tcb->reply_port = __hurd_reply_port0;
+  if (full)
+/* Take over the reply port we've been using.  */
+tcb->reply_port = __hurd_reply_port0;
 
   /* Get the first available selector.  */
   int sel = -1;
@@ -175,15 +176,20 @@ _hurd_tls_init (tcbhead_t *tcb)
 
   /* Now install the new selector.  */
   asm volatile ("mov %w0, %%gs" :: "q" (sel));
-  /* This port is now owned by the TCB.  */
-  __hurd_reply_port0 = MACH_PORT_NULL;
+  if (full)
+/* This port is now owned by the TCB.  */
+__hurd_reply_port0 = MACH_PORT_NULL;
+#ifndef SHARED
+  else
+__init1_desc = sel;
+#endif
 
 out:
   __mach_port_deallocate (__mach_task_self (), self);
   return success;
 }
 
-# define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr))
+# define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr), 1)
 #else /* defined (SHARED) && !IS_IN (rtld) */
 # define __LIBC_NO_TLS() 0
 #endif
diff --git a/sysdeps/mach/hurd/x86/init-first.c 
b/sysdeps/mach/hurd/x86/init-first.c
index 89a5f44c..d74a3c86 100644
--- a/sysdeps/mach/hurd/x86/init-first.c
+++ b/sysdeps/mach/hurd/x86/init-first.c
@@ -156,12 +156,7 @@ first_init (void)
 #ifndef SHARED
   /* In the static case, we need to set up TLS early so that the stack
  protection guard can be read at gs:0x14 by the gcc-generated snippets.  */
-  _hurd_tls_init (&__init1_tcbhead);
-
-  /* Make sure __LIBC_NO_TLS () keeps evaluating to 1.  */
-# ifndef __x86_64__
-  asm ("movw %%gs,%w0" : "=m" (__init1_desc));
-# endif
+  _hurd_tls_init (&__init1_tcbhead, 0);
 #endif
 
   RUN_RELHOOK (_hurd_preinit_hook, ());
diff --git a/sysdeps/mach/hurd/x86_64/tls.h b/sysdeps/mach/hurd/x86_64/tls.h
index 2b7135f6..1274723a 100644
--- a/sysdeps/mach/hurd/x86_64/tls.h
+++ b/sysdeps/mach/hurd/x86_64/tls.h
@@ -173,7 +173,7 @@ extern unsigned char __libc_tls_initialized;
 #  define __LIBC_NO_TLS() __builtin_expect (!__libc_tls_initialized, 0)
 
 static inline bool __attribute__ ((unused))
-_hurd_tls_init (tcbhead_t *tcb)
+_hurd_tls_init (tcbhead_t *tcb, bool full)
 {
   error_t err;
   thread_t self = __mach_thread_self ();
@@ -181,11 +181,12 @@ _hurd_tls_init (tcbhead_t *tcb)
 
   /* We always at least start the sigthread anyway.  */
   tcb->multiple_threads = 1;
-  /* Take over the reply port we've been using.  */
-  tcb->reply_port = __hurd_reply_port0;
+  if (full)
+/* Take over the reply port we've been using.  */
+tcb->reply_port = __hurd_reply_port0;
 
   err = _hurd_tls_new (self, tcb);
-  if (err == 0)
+  if (err == 0 && full)
 {
   __libc_tls_initialized = 1;
   /* This port is now owned by the TCB.  */
@@ -195,7 +196,7 @@ _hurd_tls_init (tcbhead_t *tcb)
   return err == 0;
 }
 
-#  define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr))
+#  define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr), 1)
 # else /* defined (SHARED) && !IS_IN (rtld) */
 #  define __LIBC_NO_TLS() 0
 # endif
-- 
2.40.0

[VERY RFC PATCH 2/2] hurd: Make it possible to call memcpy very early

[Sergey Bugaev]

Normally, in static builds, the first code that runs is _start, in e.g.
sysdeps/x86_64/start.S, which quickly calls __libc_start_main, passing
it the argv etc. Among the first things __libc_start_main does is
initializing the tunables (based on env), then CPU features, and then
calls _dl_relocate_static_pie (). Specifically, this runs ifunc
resolvers to pick, based on the CPU features discovered earlier, the
most suitable implementation of "string" functions such as memcpy.

Before that point, calling memcpy (or other ifunc-resolved functions)
will not work.

In the Hurd port, things are more complex. In order to get argv/env for
our process, glibc normally needs to do an RPC to the exec server,
unless our args/env are already located on the stack (which is what
happens to bootstrap processes spawned by GNU Mach). Fetching our
argv/env from the exec server has to be done before the call to
__libc_start_main, since we need to know what our argv/env are to pass
them to __libc_start_main.

On the other hand, the implementation of the RPC (and other initial
setup needed on the Hurd before __libc_start_main can be run) is not
very trivial. In particular, it may (and on x86_64, will) use memcpy.
But as described above, calling memcpy before __libc_start_main can not
work, since the GOT entry for it is not yet initialized at that point.

Work around this by pre-filling the GOT entry with the baseline version
of memcpy, __memcpy_sse2_unaligned. This makes it possible for early
calls to memcpy to just work. Once _dl_relocate_static_pie () is called,
the baseline version will get replaced with the most suitable one, and
that's what subsequent calls of memcpy are going to call.

Also, apply the same treatment to __stpncpy, which can also be used by
the RPCs (see mig_strncpy.c), and is an ifunc-resolved function on both
x86_64 and i386.

Tested on x86_64-gnu (!).

Signed-off-by: Sergey Bugaev 
---

Please tell me:

* if the approach is at all sane
* if there's a better way to do this without hardcoding
  "__memcpy_sse2_unaligned"
* are the GOT entries for indirect functions supposed to be statically
  initialized to anything (in the binary)? if yes, why? if not, why is
  PROGBITS and not NOBITS?
* am I doing all this _GLOBAL_OFFSET_TABLE_, @GOT, @GOTOFF, @GOTPCREL
  correctly?
* should there be a !PIC version as well? does the GOT exist under
  !PIC (to access indirect functions), and if it does then how do I
  access it? it would seem gcc just generates a direct $function even
  for indirect functions in this case.

 sysdeps/mach/hurd/i386/static-start.S   | 7 +++
 sysdeps/mach/hurd/x86_64/static-start.S | 8 
 2 files changed, 15 insertions(+)

diff --git a/sysdeps/mach/hurd/i386/static-start.S 
b/sysdeps/mach/hurd/i386/static-start.S
index c5d12645..1b1ae559 100644
--- a/sysdeps/mach/hurd/i386/static-start.S
+++ b/sysdeps/mach/hurd/i386/static-start.S
@@ -19,6 +19,13 @@
.text
.globl _start
 _start:
+#ifdef PIC
+   call __x86.get_pc_thunk.bx
+   addl $_GLOBAL_OFFSET_TABLE_, %ebx
+   leal __stpncpy_ia32@GOTOFF(%ebx), %eax
+   movl %eax, __stpncpy@GOT(%ebx)
+#endif
+
call _hurd_stack_setup
xorl %edx, %edx
jmp _start1
diff --git a/sysdeps/mach/hurd/x86_64/static-start.S 
b/sysdeps/mach/hurd/x86_64/static-start.S
index 982d3d52..81b3c0ac 100644
--- a/sysdeps/mach/hurd/x86_64/static-start.S
+++ b/sysdeps/mach/hurd/x86_64/static-start.S
@@ -19,6 +19,14 @@
.text
.globl _start
 _start:
+
+#ifdef PIC
+   leaq __memcpy_sse2_unaligned(%rip), %rax
+   movq %rax, memcpy@GOTPCREL(%rip)
+   leaq __stpncpy_sse2_unaligned(%rip), %rax
+   movq %rax, __stpncpy@GOTPCREL(%rip)
+#endif
+
call _hurd_stack_setup
xorq %rdx, %rdx
jmp _start1
-- 
2.40.0

Re: [PATCH 3/5] fix exception message format for 64-bit userspace

[Luca Dariz]

Hi Flavio,

Il 20/04/23 04:04, Flávio Cruz ha scritto:
On Wed, Apr 19, 2023 at 3:47 PM Luca Dariz > wrote:


* kern/exception.c: message fields need to be aligned to 8 bytes for a
   64-bit userspace, so add the required padding if needed, as done by
   MIG.

I believe this shouldn't be necessary due to 
https://git.savannah.gnu.org/cgit/hurd/gnumach.git/commit/?id=8e5e86fc13732b60d2e4d14152d92db1f1ae73f9  which forces mach_msg_type_t to always be 8 byte aligned. If you check the size before and after your patch, it will be the same.


you're right, I must have made this change before this commit.


Luca

Re: [VERY RFC PATCH 2/2] hurd: Make it possible to call memcpy very early

[H.J. Lu]

On Thu, Apr 20, 2023 at 11:43 AM Sergey Bugaev  wrote:
>
> Normally, in static builds, the first code that runs is _start, in e.g.
> sysdeps/x86_64/start.S, which quickly calls __libc_start_main, passing
> it the argv etc. Among the first things __libc_start_main does is
> initializing the tunables (based on env), then CPU features, and then
> calls _dl_relocate_static_pie (). Specifically, this runs ifunc
> resolvers to pick, based on the CPU features discovered earlier, the
> most suitable implementation of "string" functions such as memcpy.
>
> Before that point, calling memcpy (or other ifunc-resolved functions)
> will not work.
>
> In the Hurd port, things are more complex. In order to get argv/env for
> our process, glibc normally needs to do an RPC to the exec server,
> unless our args/env are already located on the stack (which is what
> happens to bootstrap processes spawned by GNU Mach). Fetching our
> argv/env from the exec server has to be done before the call to
> __libc_start_main, since we need to know what our argv/env are to pass
> them to __libc_start_main.
>
> On the other hand, the implementation of the RPC (and other initial
> setup needed on the Hurd before __libc_start_main can be run) is not
> very trivial. In particular, it may (and on x86_64, will) use memcpy.
> But as described above, calling memcpy before __libc_start_main can not
> work, since the GOT entry for it is not yet initialized at that point.
>
> Work around this by pre-filling the GOT entry with the baseline version
> of memcpy, __memcpy_sse2_unaligned. This makes it possible for early
> calls to memcpy to just work. Once _dl_relocate_static_pie () is called,
> the baseline version will get replaced with the most suitable one, and
> that's what subsequent calls of memcpy are going to call.
>
> Also, apply the same treatment to __stpncpy, which can also be used by
> the RPCs (see mig_strncpy.c), and is an ifunc-resolved function on both
> x86_64 and i386.
>
> Tested on x86_64-gnu (!).
>
> Signed-off-by: Sergey Bugaev 
> ---
>
> Please tell me:
>
> * if the approach is at all sane
> * if there's a better way to do this without hardcoding
>   "__memcpy_sse2_unaligned"
> * are the GOT entries for indirect functions supposed to be statically
>   initialized to anything (in the binary)? if yes, why? if not, why is
>   PROGBITS and not NOBITS?
> * am I doing all this _GLOBAL_OFFSET_TABLE_, @GOT, @GOTOFF, @GOTPCREL
>   correctly?
> * should there be a !PIC version as well? does the GOT exist under
>   !PIC (to access indirect functions), and if it does then how do I
>   access it? it would seem gcc just generates a direct $function even
>   for indirect functions in this case.
>
>  sysdeps/mach/hurd/i386/static-start.S   | 7 +++
>  sysdeps/mach/hurd/x86_64/static-start.S | 8 
>  2 files changed, 15 insertions(+)
>
> diff --git a/sysdeps/mach/hurd/i386/static-start.S 
> b/sysdeps/mach/hurd/i386/static-start.S
> index c5d12645..1b1ae559 100644
> --- a/sysdeps/mach/hurd/i386/static-start.S
> +++ b/sysdeps/mach/hurd/i386/static-start.S
> @@ -19,6 +19,13 @@
> .text
> .globl _start
>  _start:
> +#ifdef PIC
> +   call __x86.get_pc_thunk.bx
> +   addl $_GLOBAL_OFFSET_TABLE_, %ebx
> +   leal __stpncpy_ia32@GOTOFF(%ebx), %eax
> +   movl %eax, __stpncpy@GOT(%ebx)
> +#endif
> +
> call _hurd_stack_setup
> xorl %edx, %edx
> jmp _start1
> diff --git a/sysdeps/mach/hurd/x86_64/static-start.S 
> b/sysdeps/mach/hurd/x86_64/static-start.S
> index 982d3d52..81b3c0ac 100644
> --- a/sysdeps/mach/hurd/x86_64/static-start.S
> +++ b/sysdeps/mach/hurd/x86_64/static-start.S
> @@ -19,6 +19,14 @@
> .text
> .globl _start
>  _start:
> +
> +#ifdef PIC
> +   leaq __memcpy_sse2_unaligned(%rip), %rax
> +   movq %rax, memcpy@GOTPCREL(%rip)
> +   leaq __stpncpy_sse2_unaligned(%rip), %rax
> +   movq %rax, __stpncpy@GOTPCREL(%rip)
> +#endif
> +
> call _hurd_stack_setup
> xorq %rdx, %rdx
> jmp _start1
> --
> 2.40.0
>

Doesn't it disable IFUNC for memcpy and stpncpy?

-- 
H.J.

Re: [VERY RFC PATCH 2/2] hurd: Make it possible to call memcpy very early

[Adhemerval Zanella Netto]

On 20/04/23 17:25, H.J. Lu via Libc-alpha wrote:
> On Thu, Apr 20, 2023 at 11:43 AM Sergey Bugaev  wrote:
>>
>> Normally, in static builds, the first code that runs is _start, in e.g.
>> sysdeps/x86_64/start.S, which quickly calls __libc_start_main, passing
>> it the argv etc. Among the first things __libc_start_main does is
>> initializing the tunables (based on env), then CPU features, and then
>> calls _dl_relocate_static_pie (). Specifically, this runs ifunc
>> resolvers to pick, based on the CPU features discovered earlier, the
>> most suitable implementation of "string" functions such as memcpy.
>>
>> Before that point, calling memcpy (or other ifunc-resolved functions)
>> will not work.
>>
>> In the Hurd port, things are more complex. In order to get argv/env for
>> our process, glibc normally needs to do an RPC to the exec server,
>> unless our args/env are already located on the stack (which is what
>> happens to bootstrap processes spawned by GNU Mach). Fetching our
>> argv/env from the exec server has to be done before the call to
>> __libc_start_main, since we need to know what our argv/env are to pass
>> them to __libc_start_main.
>>
>> On the other hand, the implementation of the RPC (and other initial
>> setup needed on the Hurd before __libc_start_main can be run) is not
>> very trivial. In particular, it may (and on x86_64, will) use memcpy.
>> But as described above, calling memcpy before __libc_start_main can not
>> work, since the GOT entry for it is not yet initialized at that point.
>>
>> Work around this by pre-filling the GOT entry with the baseline version
>> of memcpy, __memcpy_sse2_unaligned. This makes it possible for early
>> calls to memcpy to just work. Once _dl_relocate_static_pie () is called,
>> the baseline version will get replaced with the most suitable one, and
>> that's what subsequent calls of memcpy are going to call.
>>
>> Also, apply the same treatment to __stpncpy, which can also be used by
>> the RPCs (see mig_strncpy.c), and is an ifunc-resolved function on both
>> x86_64 and i386.
>>
>> Tested on x86_64-gnu (!).
>>
>> Signed-off-by: Sergey Bugaev 
>> ---
>>
>> Please tell me:
>>
>> * if the approach is at all sane
>> * if there's a better way to do this without hardcoding
>>   "__memcpy_sse2_unaligned"
>> * are the GOT entries for indirect functions supposed to be statically
>>   initialized to anything (in the binary)? if yes, why? if not, why is
>>   PROGBITS and not NOBITS?
>> * am I doing all this _GLOBAL_OFFSET_TABLE_, @GOT, @GOTOFF, @GOTPCREL
>>   correctly?
>> * should there be a !PIC version as well? does the GOT exist under
>>   !PIC (to access indirect functions), and if it does then how do I
>>   access it? it would seem gcc just generates a direct $function even
>>   for indirect functions in this case.
>>
>>  sysdeps/mach/hurd/i386/static-start.S   | 7 +++
>>  sysdeps/mach/hurd/x86_64/static-start.S | 8 
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/sysdeps/mach/hurd/i386/static-start.S 
>> b/sysdeps/mach/hurd/i386/static-start.S
>> index c5d12645..1b1ae559 100644
>> --- a/sysdeps/mach/hurd/i386/static-start.S
>> +++ b/sysdeps/mach/hurd/i386/static-start.S
>> @@ -19,6 +19,13 @@
>> .text
>> .globl _start
>>  _start:
>> +#ifdef PIC
>> +   call __x86.get_pc_thunk.bx
>> +   addl $_GLOBAL_OFFSET_TABLE_, %ebx
>> +   leal __stpncpy_ia32@GOTOFF(%ebx), %eax
>> +   movl %eax, __stpncpy@GOT(%ebx)
>> +#endif
>> +
>> call _hurd_stack_setup
>> xorl %edx, %edx
>> jmp _start1
>> diff --git a/sysdeps/mach/hurd/x86_64/static-start.S 
>> b/sysdeps/mach/hurd/x86_64/static-start.S
>> index 982d3d52..81b3c0ac 100644
>> --- a/sysdeps/mach/hurd/x86_64/static-start.S
>> +++ b/sysdeps/mach/hurd/x86_64/static-start.S
>> @@ -19,6 +19,14 @@
>> .text
>> .globl _start
>>  _start:
>> +
>> +#ifdef PIC
>> +   leaq __memcpy_sse2_unaligned(%rip), %rax
>> +   movq %rax, memcpy@GOTPCREL(%rip)
>> +   leaq __stpncpy_sse2_unaligned(%rip), %rax
>> +   movq %rax, __stpncpy@GOTPCREL(%rip)
>> +#endif
>> +
>> call _hurd_stack_setup
>> xorq %rdx, %rdx
>> jmp _start1
>> --
>> 2.40.0
>>
> 
> Doesn't it disable IFUNC for memcpy and stpncpy?
> 


Can't you use a similar strategy done by 
5355f9ca7b10183ce06e8a18003ba30f43774858 ?

Re: [PATCH 1/5] fix address fault for 32-on-64-bit syscall

[Samuel Thibault]

Applied, thanks!

Luca Dariz, le mer. 19 avril 2023 21:46:59 +0200, a ecrit:
> * x86_64/locore.S: the faulty address is found in %rbp and not in
>   %rsi, so copy that in CR2
> ---
>  x86_64/locore.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/x86_64/locore.S b/x86_64/locore.S
> index 47d9085c..ea5c71d6 100644
> --- a/x86_64/locore.S
> +++ b/x86_64/locore.S
> @@ -1213,7 +1213,7 @@ mach_call_call:
>  mach_call_addr_push:
>   movq%r11,%rsp   /* clean parameters from stack */
>  mach_call_addr:
> - movq%rsi,R_CR2(%rbx)/* set fault address */
> + movq%rbp,R_CR2(%rbx)/* set fault address */
>   movq$(T_PAGE_FAULT),R_TRAPNO(%rbx)
>   /* set page-fault trap */
>   movq$(T_PF_USER),R_ERR(%rbx)
> -- 
> 2.30.2
> 
> 

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

Re: [PATCH 2/5] fix copyoutmsg for 64-bit userspace

[Samuel Thibault]

Applied, thanks!

Luca Dariz, le mer. 19 avril 2023 21:47:00 +0200, a ecrit:
> * x86_64/copy_user.c: use the correct user/kernel msg structure
> ---
>  x86_64/copy_user.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/x86_64/copy_user.c b/x86_64/copy_user.c
> index b5084996..f76e44c9 100644
> --- a/x86_64/copy_user.c
> +++ b/x86_64/copy_user.c
> @@ -430,7 +430,7 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, 
> const size_t ksize)
>usaddr = (vm_offset_t)(umsg + 1);
>keaddr = ksaddr + ksize - sizeof(mach_msg_header_t);
>  
> -  if (ksize > sizeof(mach_msg_user_header_t))
> +  if (ksize > sizeof(mach_msg_header_t))
>  {
>while (ksaddr < keaddr)
>  {
> @@ -484,8 +484,7 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, 
> const size_t ksize)
>  
>mach_msg_size_t usize;
>usize = sizeof(mach_msg_user_header_t) + usaddr - (vm_offset_t)(umsg + 1);
> -  usize = usize;
> -  if (copyout(&usize, &umsg->msgh_size, sizeof(kmsg->msgh_size)))
> +  if (copyout(&usize, &umsg->msgh_size, sizeof(umsg->msgh_size)))
>  return 1;
>  
>return 0;
> -- 
> 2.30.2
> 
> 

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

Re: [PATCH 5/5] add setting gs/fsbase

[Samuel Thibault]

Hello,

Sergey Bugaev, le jeu. 20 avril 2023 16:08:45 +0300, a ecrit:
> Is that actually configurable somewhere (at gcc/binutils build time, I
> mean)?

It's probably in the link script, somewhere in
/usr/lib/*/ldscripts/elf*x86_64*

> I'm thinking we will want to map the lower 4GB of address space
> inaccessible like dyld does, and not just one page, to catch pointer
> truncation in addition to nullptr dereferences.

That sounds useful indeed.

> But that would need gcc/ld to know to not use low addresses for their
> needs.

It seems that binutils' ld/emulparams/ there are already some per-OS
files: elf_x86_64_cloudabi.sh elf_x86_64_fbsd.sh elf_x86_64_haiku.sh
elf_x86_64_sol2.sh

One can probably create a gnu file that overrides 

TEXT_START_ADDR=0x40

BTW, which dynamic linker path do we have set for x86_64-gnu?

Samuel

Re: [RFC PATCH v2 5/7] hurd: Make dl-sysdep's open () cope with O_IGNORE_CTTY

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le mer. 19 avril 2023 19:02:05 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/dl-sysdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 2d595d00..6e167e12 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -289,8 +289,8 @@ open_file (const char *file_name, int flags,
>return MACH_PORT_NULL;
>  }
>  
> -  assert (!(flags & ~(O_READ | O_EXEC | O_CLOEXEC)));
> -  flags &= ~O_CLOEXEC;
> +  assert (!(flags & ~(O_READ | O_EXEC | O_CLOEXEC | O_IGNORE_CTTY)));
> +  flags &= ~(O_CLOEXEC | O_IGNORE_CTTY);
>  
>startdir = _dl_hurd_data->portarray[file_name[0] == '/'
> ? INIT_PORT_CRDIR : INIT_PORT_CWDIR];
> -- 
> 2.40.0
> 

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

Re: [PATCH 1/4] hurd: Don't pass fd flags in CMSG_DATA

[Samuel Thibault]

Sergey Bugaev, le lun. 17 avril 2023 16:38:59 +0300, a ecrit:
> The only valid flag defined here is FD_CLOEXEC. It is of no concern to
> the receiving process whether or not the sender process wants to close
> its copy of sent file descriptor upon exec,

Ok, but couldn't there be some flags that we could want to transfer, in
the future? I'd better keep the infrastructure, even if it is not
actually useful for now. So that people who end up needing something see
that passing it is already supported.

Samuel

> and it should not influence
> whether or not the received file descriptor gets the FD_CLOEXEC flag
> set in the receiving process.
> 
> The latter should in fact be dependent on the MSG_CMSG_CLOEXEC flag
> being passed to the recvmsg () call, which is going to be implemented
> in the following commit.
> 
> Fixes 344e755248ce02c0f8d095d11cc49e340703d926
> "hurd: Support sending file descriptors over Unix sockets"
> 
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/recvmsg.c | 3 +--
>  sysdeps/mach/hurd/sendmsg.c | 4 ++--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index 39de86f6..5e8b18c6 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -189,7 +189,6 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>  if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
>{
>   /* SCM_RIGHTS support.  */
> - /* The fd's flags are passed in the control data.  */
>   int *fds = (int *) CMSG_DATA (cmsg);
>   nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
>  / sizeof (int);
> @@ -200,7 +199,7 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>   if (err)
> goto cleanup;
>   fds[j] = opened_fds[newfds] = _hurd_intern_fd (newports[newfds],
> -fds[j], 0);
> +0, 0);
>   if (fds[j] == -1)
> {
>   err = errno;
> diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
> index 5871d1d8..77a720fb 100644
> --- a/sysdeps/mach/hurd/sendmsg.c
> +++ b/sysdeps/mach/hurd/sendmsg.c
> @@ -138,8 +138,8 @@ __libc_sendmsg (int fd, const struct msghdr *message, int 
> flags)
>0, 0, 0, 0);
>  if (! err)
>nports++;
> -/* We pass the flags in the control data.  */
> -fds[i] = descriptor->flags;
> +/* We just pass 0 in the control data.  */
> +fds[i] = 0;
>  err;
>}));
>  
> -- 
> 2.39.2
> 

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

Re: [PATCH 1/4] hurd: Don't pass fd flags in CMSG_DATA

[Sergey Bugaev]

On Fri, Apr 21, 2023 at 12:14 AM Samuel Thibault
 wrote:
> Sergey Bugaev, le lun. 17 avril 2023 16:38:59 +0300, a ecrit:
> > The only valid flag defined here is FD_CLOEXEC. It is of no concern to
> > the receiving process whether or not the sender process wants to close
> > its copy of sent file descriptor upon exec,
>
> Ok, but couldn't there be some flags that we could want to transfer, in
> the future?

Unlikely -- it's been years (I don't know how old FD_CLOEXEC is
exactly, but it surely predates O_CLOEXEC by many years) and AFAIK
nobody came up with any ideas for more fd flags, other than
FD_CLOFORK, but that wouldn't be transferable either.

And the whole idea of fd flags (as opposed to flags applied to the
open file itself, the peropen in Hurd terms, like O_NONBLOCK and
O_ASYNC) is that they apply to that single file descriptor, and are
not carried over when it's dup'ed. sendmsg+recvmsg is like a remote
dup in this sense.

> I'd better keep the infrastructure, even if it is not
> actually useful for now. So that people who end up needing something see
> that passing it is already supported.

I understand, but also it's not like there's a lot of infrastructure
that I'm removing here. You could think of it that way: the
infrastructure for passing an integer value along with the port is
still there, but currently no valid flags for it are defined, and so 0
is always used. We could spell it as

fds[i] = descriptor->flags & ~FD_CLOEXEC;

if you would prefer; but that would still always evaluate to 0 (but
the compiler wouldn't be aware and would generate the extra
instructions).

Sergey

Re: [PATCH 1/4] hurd: Don't pass fd flags in CMSG_DATA

[Samuel Thibault]

Sergey Bugaev, le ven. 21 avril 2023 00:47:43 +0300, a ecrit:
> You could think of it that way: the
> infrastructure for passing an integer value along with the port is
> still there, but currently no valid flags for it are defined, and so 0
> is always used. We could spell it as
> 
> fds[i] = descriptor->flags & ~FD_CLOEXEC;
> 
> if you would prefer;

Yes, I'd prefer.

> but that would still always evaluate to 0 (but the compiler wouldn't
> be aware and would generate the extra instructions).

Yes, sure, but that still looks preferable to me maintenance-wise.

Samuel

Re: [PATCH 1/2] hurd: Don't migrate reply port into __init1_tcbhead

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le jeu. 20 avril 2023 21:42:19 +0300, a ecrit:
> Properly differentiate between setting up the real TLS with
> TLS_INIT_TP, and setting up the early TLS (__init1_tcbhead) in static
> builds. In the latter case, don't yet migrate the reply port into the
> TCB, and don't yet set __libc_tls_initialized to 1.
> 
> This also lets us move the __init1_desc assignment inside
> _hurd_tls_init ().
> 
> Fixes cd019ddd892e182277fadd6aedccc57fa3923c8d
> "hurd: Don't leak __hurd_reply_port0"
> 
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/i386/tls.h   | 18 --
>  sysdeps/mach/hurd/x86/init-first.c |  7 +--
>  sysdeps/mach/hurd/x86_64/tls.h | 11 ++-
>  3 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/i386/tls.h b/sysdeps/mach/hurd/i386/tls.h
> index 0ac8917a..e124fb10 100644
> --- a/sysdeps/mach/hurd/i386/tls.h
> +++ b/sysdeps/mach/hurd/i386/tls.h
> @@ -136,7 +136,7 @@ __LIBC_NO_TLS (void)
> special attention since 'errno' is not yet available and if the
> operation can cause a failure 'errno' must not be touched.  */
>  static inline bool __attribute__ ((unused))
> -_hurd_tls_init (tcbhead_t *tcb)
> +_hurd_tls_init (tcbhead_t *tcb, bool full)
>  {
>HURD_TLS_DESC_DECL (desc, tcb);
>thread_t self = __mach_thread_self ();
> @@ -148,8 +148,9 @@ _hurd_tls_init (tcbhead_t *tcb)
>tcb->tcb = tcb;
>/* We always at least start the sigthread anyway.  */
>tcb->multiple_threads = 1;
> -  /* Take over the reply port we've been using.  */
> -  tcb->reply_port = __hurd_reply_port0;
> +  if (full)
> +/* Take over the reply port we've been using.  */
> +tcb->reply_port = __hurd_reply_port0;
>  
>/* Get the first available selector.  */
>int sel = -1;
> @@ -175,15 +176,20 @@ _hurd_tls_init (tcbhead_t *tcb)
>  
>/* Now install the new selector.  */
>asm volatile ("mov %w0, %%gs" :: "q" (sel));
> -  /* This port is now owned by the TCB.  */
> -  __hurd_reply_port0 = MACH_PORT_NULL;
> +  if (full)
> +/* This port is now owned by the TCB.  */
> +__hurd_reply_port0 = MACH_PORT_NULL;
> +#ifndef SHARED
> +  else
> +__init1_desc = sel;
> +#endif
>  
>  out:
>__mach_port_deallocate (__mach_task_self (), self);
>return success;
>  }
>  
> -# define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr))
> +# define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr), 1)
>  #else /* defined (SHARED) && !IS_IN (rtld) */
>  # define __LIBC_NO_TLS() 0
>  #endif
> diff --git a/sysdeps/mach/hurd/x86/init-first.c 
> b/sysdeps/mach/hurd/x86/init-first.c
> index 89a5f44c..d74a3c86 100644
> --- a/sysdeps/mach/hurd/x86/init-first.c
> +++ b/sysdeps/mach/hurd/x86/init-first.c
> @@ -156,12 +156,7 @@ first_init (void)
>  #ifndef SHARED
>/* In the static case, we need to set up TLS early so that the stack
>   protection guard can be read at gs:0x14 by the gcc-generated snippets.  
> */
> -  _hurd_tls_init (&__init1_tcbhead);
> -
> -  /* Make sure __LIBC_NO_TLS () keeps evaluating to 1.  */
> -# ifndef __x86_64__
> -  asm ("movw %%gs,%w0" : "=m" (__init1_desc));
> -# endif
> +  _hurd_tls_init (&__init1_tcbhead, 0);
>  #endif
>  
>RUN_RELHOOK (_hurd_preinit_hook, ());
> diff --git a/sysdeps/mach/hurd/x86_64/tls.h b/sysdeps/mach/hurd/x86_64/tls.h
> index 2b7135f6..1274723a 100644
> --- a/sysdeps/mach/hurd/x86_64/tls.h
> +++ b/sysdeps/mach/hurd/x86_64/tls.h
> @@ -173,7 +173,7 @@ extern unsigned char __libc_tls_initialized;
>  #  define __LIBC_NO_TLS() __builtin_expect (!__libc_tls_initialized, 0)
>  
>  static inline bool __attribute__ ((unused))
> -_hurd_tls_init (tcbhead_t *tcb)
> +_hurd_tls_init (tcbhead_t *tcb, bool full)
>  {
>error_t err;
>thread_t self = __mach_thread_self ();
> @@ -181,11 +181,12 @@ _hurd_tls_init (tcbhead_t *tcb)
>  
>/* We always at least start the sigthread anyway.  */
>tcb->multiple_threads = 1;
> -  /* Take over the reply port we've been using.  */
> -  tcb->reply_port = __hurd_reply_port0;
> +  if (full)
> +/* Take over the reply port we've been using.  */
> +tcb->reply_port = __hurd_reply_port0;
>  
>err = _hurd_tls_new (self, tcb);
> -  if (err == 0)
> +  if (err == 0 && full)
>  {
>__libc_tls_initialized = 1;
>/* This port is now owned by the TCB.  */
> @@ -195,7 +196,7 @@ _hurd_tls_init (tcbhead_t *tcb)
>return err == 0;
>  }
>  
> -#  define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr))
> +#  define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr), 1)
>  # else /* defined (SHARED) && !IS_IN (rtld) */
>  #  define __LIBC_NO_TLS() 0
>  # endif
> -- 
> 2.40.0
> 

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