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

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

On 2/1/22 09:46, 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.
> 
> 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
> at load time.
> 
> Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu.

OK for glibc 2.35. Please push.

I meant to add more comments about init_tls bool in the function, but I can do that
later. Thanks for updating to v13.

Tested on x86_64 and i686.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/Makefile               |  8 ++++
>  elf/dl-tls.c               | 17 ++++++--
>  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, 169 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 ddd0e84afc..72ba6a1e66 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -377,6 +377,7 @@ tests += \
>    tst-audit18 \
>    tst-audit19b \
>    tst-audit20 \
> +  tst-audit21 \
>    tst-audit22 \
>    tst-audit23 \
>    tst-auditmany \
> @@ -697,6 +698,8 @@ modules-names = \
>    tst-auditmod19a \
>    tst-auditmod19b \
>    tst-auditmod20 \
> +  tst-auditmod21a \
> +  tst-auditmod21b \
>    tst-auditmod22 \
>    tst-auditmod23 \
>    tst-auxvalmod \
> @@ -2139,6 +2142,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
> +tst-audit21-ENV = LD_AUDIT=$(objpfx)tst-auditmod21a.so
> +
>  $(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..093cdddb7e 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -519,8 +519,12 @@ _dl_resize_dtv (dtv_t *dtv, size_t max_modid)
>  }
>  
>  
> +/* 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 loader.  */
>  void *
> -_dl_allocate_tls_init (void *result)
> +_dl_allocate_tls_init (void *result, bool init_tls)
>  {
>    if (result == NULL)
>      /* The memory allocation failed.  */
> @@ -593,7 +597,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 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.   */
> +	  if (map->l_ns != LM_ID_BASE && !init_tls)
> +	    continue;
>  	  memset (__mempcpy (dest, map->l_tls_initimage,
>  			     map->l_tls_initimage_size), '\0',
>  		  map->l_tls_blocksize - map->l_tls_initimage_size);
> @@ -620,7 +631,7 @@ _dl_allocate_tls (void *mem)
>  {
>    return _dl_allocate_tls_init (mem == NULL
>  				? _dl_allocate_tls_storage ()
> -				: allocate_dtv (mem));
> +				: allocate_dtv (mem), true);
>  }
>  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);
>  
>    /* 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..3a47ab64d4
> --- /dev/null
> +++ b/elf/tst-audit21.c
> @@ -0,0 +1,42 @@
> +/* Check LD_AUDIT with static TLS.
> +   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")));
> +
> +static void *
> +tf (void *arg)
> +{
> +  TEST_COMPARE (out, 0);
> +  out = isspace (' ');
> +  return NULL;
> +}
> +
> +int main (int argc, char *argv[])
> +{
> +  TEST_COMPARE (out, 0);
> +  out = isspace (' ');
> +
> +  pthread_t t = xpthread_create (NULL, tf, NULL);
> +  xpthread_join (t);
> +
> +  return 0;
> +}
> diff --git a/elf/tst-auditmod21a.c b/elf/tst-auditmod21a.c
> new file mode 100644
> index 0000000000..f6d51b5c05
> --- /dev/null
> +++ b/elf/tst-auditmod21a.c
> @@ -0,0 +1,80 @@
> +/* Check LD_AUDIT with static TLS.
> +   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;
> +
> +/* 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 accesses the initial-exec glibc TLS variables, which are
> +     setup in glibc initialization.  */
> +  out = isspace (' ');
> +}
> +
> +unsigned int
> +la_version (unsigned int v)
> +{
> +  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)
> +{
> +  if (tls_var0 != 0x1 || tls_var1 != 0x20)
> +    abort ();
> +  call_libc ();
> +}
> +
> +void
> +la_preinit (uintptr_t* cookie)
> +{
> +  call_libc ();
> +}
> diff --git a/elf/tst-auditmod21b.c b/elf/tst-auditmod21b.c
> new file mode 100644
> index 0000000000..6ba5335b75
> --- /dev/null
> +++ b/elf/tst-auditmod21b.c
> @@ -0,0 +1,22 @@
> +/* Check LD_AUDIT with static TLS.
> +   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;
> 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);
>  
>    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);
>  rtld_hidden_proto (_dl_allocate_tls_init)
>  
>  /* Deallocate memory allocated with _dl_allocate_tls.  */


-- 
Cheers,
Carlos.