Re: [PATCH v12 2/4] elf: Fix initial-exec TLS access on audit modules (BZ #28096)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 01/02/2022 02:29, Carlos O'Donell wrote:
> On 1/25/22 13:36, Adhemerval Zanella wrote:
>> For audit modules and dependencies with initial-exec TLS, we can not
>> set the initial TLS image on default loader initialization because it
>> would already be set by the audit setup.  However, subsequent thread
>> creation would need to follow the default behaviour.
> 
> Correct, there is a circular dependency, we need the initial TLS image setup for
> the auditor before the loader does the setup.
> 
>> This patch fixes it by setting l_auditing link_map field not only
>> for the audit modules, but also for all its dependencies.  This is
>> used on _dl_allocate_tls_init to avoid the static TLS initialization
>> only at loading time.
> 
> s/only at loading/at load/g

Ack.

> 
>> Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu.
> 
> Tested on x86_64 and i686 and it looks good for glibc 2.35.
> 
> Please post v13 with fixes and I'll ACK that as release manager.
> 
> 
>> ---
>>  elf/Makefile               |  8 ++++
>>  elf/dl-tls.c               | 13 +++++--
>>  elf/rtld.c                 |  2 +-
>>  elf/tst-audit21.c          | 42 ++++++++++++++++++++
>>  elf/tst-auditmod21a.c      | 80 ++++++++++++++++++++++++++++++++++++++
>>  elf/tst-auditmod21b.c      | 22 +++++++++++
>>  nptl/allocatestack.c       |  2 +-
>>  sysdeps/generic/ldsodefs.h |  2 +-
>>  8 files changed, 165 insertions(+), 6 deletions(-)
>>  create mode 100644 elf/tst-audit21.c
>>  create mode 100644 elf/tst-auditmod21a.c
>>  create mode 100644 elf/tst-auditmod21b.c
>>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index a3b4468593..7d01b71f6a 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -376,6 +376,7 @@ tests += \
>>    tst-audit18 \
>>    tst-audit19b \
>>    tst-audit20 \
>> +  tst-audit21 \
> 
> OK. Test.
> 
>>    tst-audit22 \
>>    tst-audit23 \
>>    tst-auditmany \
>> @@ -696,6 +697,8 @@ modules-names = \
>>    tst-auditmod19a \
>>    tst-auditmod19b \
>>    tst-auditmod20 \
>> +  tst-auditmod21a \
>> +  tst-auditmod21b \
> 
> OK. Two audit modules.
> 
>>    tst-auditmod22 \
>>    tst-auditmod23 \
>>    tst-auxvalmod \
>> @@ -2136,6 +2139,11 @@ tst-audit19b-ARGS = -- $(host-test-program-cmd)
>>  $(objpfx)tst-audit20.out: $(objpfx)tst-auditmod20.so
>>  tst-audit20-ENV = LD_AUDIT=$(objpfx)tst-auditmod20.so
>>  
>> +$(objpfx)tst-audit21: $(shared-thread-library)
>> +$(objpfx)tst-audit21.out: $(objpfx)tst-auditmod21a.so
>> +$(objpfx)tst-auditmod21a.so: $(objpfx)tst-auditmod21b.so
> 
> OK. A deps B.
> 
>> +tst-audit21-ENV = LD_AUDIT=$(objpfx)tst-auditmod21a.so
> 
> OK. New test is audited by A, which deps B.
> 
>> +
>>  $(objpfx)tst-audit22.out: $(objpfx)tst-auditmod22.so
>>  tst-audit22-ARGS = -- $(host-test-program-cmd)
>>  
>> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
>> index 8ba70c9a9d..ccbe39660b 100644
>> --- a/elf/dl-tls.c
>> +++ b/elf/dl-tls.c
>> @@ -520,7 +520,7 @@ _dl_resize_dtv (dtv_t *dtv, size_t max_modid)
>>  
>>  
> 
> This needs a comment explaining bool init_tls.
> 
> /* Allocate initial TLS.  RESULT should be a non-NULL pointer to storage
>    for the TLS space.  The DTV may be resized, and so this function may
>    call malloc to allocate that space.  The loader's GL(dl_load_tls_lock)
>    is taken when manipulating global TLS-related data in the laoder.

Ack.

> 
>>  void *
>> -_dl_allocate_tls_init (void *result)
>> +_dl_allocate_tls_init (void *result, bool init_tls)
> 
> OK. This is called by a reinitialization of re-used stacks. It is called
> by normal _dl_allocate_tls, which is called when a thread allocates a DTV.
> 
>>  {
>>    if (result == NULL)
>>      /* The memory allocation failed.  */
>> @@ -593,7 +593,14 @@ _dl_allocate_tls_init (void *result)
>>  	     some platforms use in static programs requires it.  */
>>  	  dtv[map->l_tls_modid].pointer.val = dest;
>>  
>> -	  /* Copy the initialization image and clear the BSS part.  */
>> +	  /* Copy the initialization image and clear the BSS part.  For
>> +	     audit modules or depedencies with initial-exec TLS, we can not
> 
> s/depedencies/dependencies/g

Ack.

> 
>> +	     set the initial TLS image on default loader initialization
>> +	     because it would already be set by the audit setup.  However,
>> +	     subsequent thread creation would need to follow the default
>> +	     behaviour.   */
>> +	  if (map->l_ns != LM_ID_BASE && !init_tls)
>> +	    continue;
> 
> OK. The tricky part here is that this works because GL(dl_tls_dtv_slotinfo_list)
> is a *GLOBAL* list that is irrespective of linker namespace. So when we come to setup
> the static TLS for the thread via rtld we will redo the already done setup for the libc
> in the non-main-namespace.

Yeap, that is the main issue indeed.

> 
>>  	  memset (__mempcpy (dest, map->l_tls_initimage,
>>  			     map->l_tls_initimage_size), '\0',
>>  		  map->l_tls_blocksize - map->l_tls_initimage_size);
>> @@ -620,7 +627,7 @@ _dl_allocate_tls (void *mem)
>>  {
>>    return _dl_allocate_tls_init (mem == NULL
>>  				? _dl_allocate_tls_storage ()
>> -				: allocate_dtv (mem));
>> +				: allocate_dtv (mem), true);
> 
> OK. Correct, when calling via _dl_allocate_tls we always want to initialize TLS.
> 
>>  }
>>  rtld_hidden_def (_dl_allocate_tls)
>>  
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 8d233f77be..10436f7034 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -2462,7 +2462,7 @@ dl_main (const ElfW(Phdr) *phdr,
>>       into the main thread's TLS area, which we allocated above.
>>       Note: thread-local variables must only be accessed after completing
>>       the next step.  */
>> -  _dl_allocate_tls_init (tcbp);
>> +  _dl_allocate_tls_init (tcbp, false);
> 
> OK. When doing this step we have already potentially initialized everything
> via init_tls(naudit) earlier, but "false" is only useful if we are in the
> non-base namespace.
> 
>>  
>>    /* And finally install it for the main thread.  */
>>    if (! tls_init_tp_called)
>> diff --git a/elf/tst-audit21.c b/elf/tst-audit21.c
>> new file mode 100644
>> index 0000000000..41446a2107
>> --- /dev/null
>> +++ b/elf/tst-audit21.c
>> @@ -0,0 +1,42 @@
>> +/* Check DT_AUDIT with static TLS.
> 
> s/DT_AUDIT/LD_AUDIT/g

Ack.

> 
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include <ctype.h>
>> +#include <support/xthread.h>
>> +#include <support/check.h>
>> +
>> +static volatile __thread int out __attribute__ ((tls_model ("initial-exec")));
> 
> OK. out has size int.
> 
>> +
>> +static void *
>> +tf (void *arg)
>> +{
>> +  TEST_COMPARE (out, 0);
>> +  out = isspace (' ');
> 
> OK. Uses ctype data, which uses tls also.
> 
>> +  return NULL;
>> +}
>> +
>> +int main (int argc, char *argv[])
>> +{
>> +  TEST_COMPARE (out, 0);
>> +  out = isspace (' ');
>> +
>> +  pthread_t t = xpthread_create (NULL, tf, NULL);
> 
> OK. In the new thread look at the value.
> 
>> +  xpthread_join (t);
>> +
>> +  return 0;
>> +}
>> diff --git a/elf/tst-auditmod21a.c b/elf/tst-auditmod21a.c
>> new file mode 100644
>> index 0000000000..4c0f256969
>> --- /dev/null
>> +++ b/elf/tst-auditmod21a.c
>> @@ -0,0 +1,80 @@
>> +/* Check DT_AUDIT with static TLS.
> 
> s/DT_AUDIT/LD_AUDIT/g

Ack.

> 
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include <ctype.h>
>> +#include <stdlib.h>
>> +#include <link.h>
>> +
>> +#define tls_ie __attribute__ ((tls_model ("initial-exec")))
>> +
>> +__thread int tls_var0 tls_ie;
>> +__thread int tls_var1 tls_ie = 0x10;
> 
> OK. Two TLS IE mode vars one with a non-zero intializer.
> 
>> +
>> +/* Defined at tst-auditmod21b.so  */
>> +extern __thread int tls_var2;
>> +extern __thread int tls_var3;
>> +
>> +static volatile int out;
>> +
>> +static void
>> +call_libc (void)
>> +{
>> +  /* isspace access the initial-exec glibc TLS variables, which are
> 
> s/access/accesses/g

Ack.

> 
>> +     setup in glibc initialization.  */
>> +  out = isspace (' ');
>> +}
>> +
>> +unsigned int
>> +la_version (unsigned int v)
>> +{
> 
> OK. la_version is called to initialize this module, so it's after all the
> early tls initialization done by the loader.
> 
>> +  tls_var0 = 0x1;
>> +  if (tls_var1 != 0x10)
>> +    abort ();
>> +  tls_var1 = 0x20;
>> +
>> +  tls_var2 = 0x2;
>> +  if (tls_var3 != 0x20)
>> +    abort ();
>> +  tls_var3 = 0x40;
>> +
>> +  call_libc ();
>> +
>> +  return LAV_CURRENT;
>> +}
>> +
>> +unsigned int
>> +la_objopen (struct link_map* map, Lmid_t lmid, uintptr_t* cookie)
>> +{
>> +  call_libc ();
>> +  *cookie = (uintptr_t) map;
>> +  return 0;
>> +}
>> +
>> +void
>> +la_activity (uintptr_t* cookie, unsigned int flag)
>> +{
> 
> OK. By the time we have activity our TLS will have been overwritten.
> 
>> +  if (tls_var0 != 0x1 || tls_var1 != 0x20)
>> +    abort ();
>> +  call_libc ();
>> +}
>> +
>> +void
>> +la_preinit (uintptr_t* cookie)
>> +{
>> +  call_libc ();
> 
> OK. Trigger libc tls usage after all DSOs are loaded but before main.
> 
>> +}
>> diff --git a/elf/tst-auditmod21b.c b/elf/tst-auditmod21b.c
>> new file mode 100644
>> index 0000000000..69705cf75a
>> --- /dev/null
>> +++ b/elf/tst-auditmod21b.c
>> @@ -0,0 +1,22 @@
>> +/* Check DT_AUDIT with static TLS.
> 
> s/DT_AUDIT/LD_AUDIT/g

Ack.

> 
>> +   Copyright (C) 2022 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#define tls_ie __attribute__ ((tls_model ("initial-exec")))
>> +
>> +__thread int tls_var2 tls_ie;
>> +__thread int tls_var3 tls_ie = 0x20;
> 
> OK.
> 
>> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
>> index 3fb085f9a1..34a33164ff 100644
>> --- a/nptl/allocatestack.c
>> +++ b/nptl/allocatestack.c
>> @@ -138,7 +138,7 @@ get_cached_stack (size_t *sizep, void **memp)
>>    memset (dtv, '\0', (dtv[-1].counter + 1) * sizeof (dtv_t));
>>  
>>    /* Re-initialize the TLS.  */
>> -  _dl_allocate_tls_init (TLS_TPADJ (result));
>> +  _dl_allocate_tls_init (TLS_TPADJ (result), true);
> 
> OK. Yes, a thread needs to reinitialize this for the new stack.
> 
>>  
>>    return result;
>>  }
>> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
>> index f6b2b415a6..97061bdf9f 100644
>> --- a/sysdeps/generic/ldsodefs.h
>> +++ b/sysdeps/generic/ldsodefs.h
>> @@ -1282,7 +1282,7 @@ extern void _dl_allocate_static_tls (struct link_map *map) attribute_hidden;
>>  /* These are internal entry points to the two halves of _dl_allocate_tls,
>>     only used within rtld.c itself at startup time.  */
>>  extern void *_dl_allocate_tls_storage (void) attribute_hidden;
>> -extern void *_dl_allocate_tls_init (void *);
>> +extern void *_dl_allocate_tls_init (void *, bool);
> 
> OK.
> 
>>  rtld_hidden_proto (_dl_allocate_tls_init)
>>  
>>  /* Deallocate memory allocated with _dl_allocate_tls.  */
> 
>