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

[Carlos O'Donell <carlos@redhat.com>]

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

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

>  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

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

>  	  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

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

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

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

> +   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.  */


-- 
Cheers,
Carlos.