[v5,05/22] elf: Fix initial-exec TLS access on audit modules (BZ #28096)

Message ID 20211109183347.2943786-6-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Multiple rtld-audit fixes |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Netto Nov. 9, 2021, 6:33 p.m. UTC
  For ldaudit 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 ldaudit setup.  However, subsequent thread
creation would need to follow the default behaviour.

This patch fixes by making_dl_new_object() sets a new link_map flag
'l_dont_set_tls_static' only for __RTLD_AUDIT modules.  The flag is
later reset on _dl_allocate_tls_init().

Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu.
---
 elf/Makefile         |  5 ++++
 elf/dl-object.c      |  3 ++
 elf/dl-tls.c         | 16 +++++++---
 elf/rtld.c           |  2 ++
 elf/tst-audit21.c    | 42 +++++++++++++++++++++++++++
 elf/tst-auditmod21.c | 69 ++++++++++++++++++++++++++++++++++++++++++++
 include/link.h       |  2 ++
 7 files changed, 135 insertions(+), 4 deletions(-)
 create mode 100644 elf/tst-audit21.c
 create mode 100644 elf/tst-auditmod21.c
  

Comments

Florian Weimer Nov. 10, 2021, 1:23 p.m. UTC | #1
* Adhemerval Zanella:

> diff --git a/elf/Makefile b/elf/Makefile
> index 4758cb23c4..861e83236e 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile

> +$(objpfx)tst-audit21.out: $(objpfx)tst-auditmod21.so
> +tst-audit21-ENV = LD_AUDIT=$(objpfx)tst-auditmod21.so

This is needed to fix a Hurd build failure:

$(objpfx)tst-audit21: $(shared-thread-library)

> diff --git a/elf/dl-object.c b/elf/dl-object.c
> index 1875599eb2..eb2158a84b 100644
> --- a/elf/dl-object.c
> +++ b/elf/dl-object.c
> @@ -175,6 +175,9 @@ _dl_new_object (char *realname, const char *libname, int type,
>  
>    new->l_local_scope[0] = &new->l_searchlist;
>  
> +  if (mode & __RTLD_AUDIT)
> +    new->l_dont_set_tls_static = 1;
> +
>    /* Determine the origin.  If allocating the link map for the main
>       executable, the realname is not known and "".  In this case, the
>       origin needs to be determined by other means.  However, in case

> diff --git a/elf/rtld.c b/elf/rtld.c
> index 8953347b00..db1817655f 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1055,6 +1055,8 @@ ERROR: audit interface '%s' requires version %d (maximum supported version %d);
>  
>    /* Mark the DSO as being used for auditing.  */
>    dlmargs.map->l_auditing = 1;
> +  /* Mark the DSO to not clear the TLS bss in tls initialization.  */
> +  dlmargs.map->l_dont_set_tls_static = 1;
>  }
>  
>  /* Notify the the audit modules that the object MAP has already been

I'm not sure if this actually works.  As far as I understand it,
everything in an audit namespace needs this special treatment, and not
just the audit module itself.

I think we shuld add a parameter to _dl_allocate_tls_init and that
indicates the initialization should only be applied to objects in the
base namespace.  This way, initialization is also skipped for dlopen'ed
modules in an audit namespace.

Thanks,
Florian
  
Adhemerval Zanella Netto Nov. 11, 2021, 6:54 p.m. UTC | #2
On 10/11/2021 10:23, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 4758cb23c4..861e83236e 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
> 
>> +$(objpfx)tst-audit21.out: $(objpfx)tst-auditmod21.so
>> +tst-audit21-ENV = LD_AUDIT=$(objpfx)tst-auditmod21.so
> 
> This is needed to fix a Hurd build failure:
> 
> $(objpfx)tst-audit21: $(shared-thread-library)

Ack.

> 
>> diff --git a/elf/dl-object.c b/elf/dl-object.c
>> index 1875599eb2..eb2158a84b 100644
>> --- a/elf/dl-object.c
>> +++ b/elf/dl-object.c
>> @@ -175,6 +175,9 @@ _dl_new_object (char *realname, const char *libname, int type,
>>  
>>    new->l_local_scope[0] = &new->l_searchlist;
>>  
>> +  if (mode & __RTLD_AUDIT)
>> +    new->l_dont_set_tls_static = 1;
>> +
>>    /* Determine the origin.  If allocating the link map for the main
>>       executable, the realname is not known and "".  In this case, the
>>       origin needs to be determined by other means.  However, in case
> 
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 8953347b00..db1817655f 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -1055,6 +1055,8 @@ ERROR: audit interface '%s' requires version %d (maximum supported version %d);
>>  
>>    /* Mark the DSO as being used for auditing.  */
>>    dlmargs.map->l_auditing = 1;
>> +  /* Mark the DSO to not clear the TLS bss in tls initialization.  */
>> +  dlmargs.map->l_dont_set_tls_static = 1;
>>  }
>>  
>>  /* Notify the the audit modules that the object MAP has already been
> 
> I'm not sure if this actually works.  As far as I understand it,
> everything in an audit namespace needs this special treatment, and not
> just the audit module itself.

Yes, and since each audit modules is loaded with LM_ID_NEWLM it means
all its dependencies.  Adding some instrumentation, it works on the audit
modules does have some dependencies:

$ ldd /home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21a.so
        linux-vdso.so.1 (0x00007fff4a172000)
        /home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21b.so (0x00007fcd11fa5000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fcd11d61000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fcd11fb1000)

$ LD_AUDIT=elf/tst-auditmod21a.so elf/ld.so --library-path . elf/tst-audit21
   2281374:     [../elf/dl-tls.c:603] map->l_name= l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=elf/tst-auditmod21a.so l_dont_set_tls_static=1
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21b.so l_dont_set_tls_static=1
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=1
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21c.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name= l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=elf/tst-auditmod21a.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21b.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21c.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=0

The l_dont_set_tls_static should be set for the audit module and its
dependencies.

> 
> I think we shuld add a parameter to _dl_allocate_tls_init and that
> indicates the initialization should only be applied to objects in the
> base namespace.  This way, initialization is also skipped for dlopen'ed
> modules in an audit namespace.

I am not sure if this would be best solution, I will check whether 
dlopen does work with this patch.
  

Patch

diff --git a/elf/Makefile b/elf/Makefile
index 4758cb23c4..861e83236e 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -228,6 +228,7 @@  tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-audit18a tst-audit18b \
 	 tst-audit19 \
 	 tst-audit20 \
+	 tst-audit21 \
 	 tst-single_threaded tst-single_threaded-pthread \
 	 tst-tls-ie tst-tls-ie-dlmopen argv0test \
 	 tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \
@@ -319,6 +320,7 @@  modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-auditmod18a tst-auditmod18b tst-audit18bmod \
 		tst-auditmod19 \
 		tst-auditmod20 tst-audit20mod \
+		tst-auditmod21 \
 		$(if $(CXX),tst-unique3lib tst-unique3lib2 tst-unique4lib \
 		  tst-nodelete-uniquemod tst-nodelete-rtldmod \
 		  tst-nodelete-zmod \
@@ -1571,6 +1573,9 @@  $(objpfx)tst-audit20.out: $(objpfx)tst-auditmod20.so \
 			  $(objpfx)tst-audit20mod.so
 tst-audit20-ARGS = -- $(host-test-program-cmd)
 
+$(objpfx)tst-audit21.out: $(objpfx)tst-auditmod21.so
+tst-audit21-ENV = LD_AUDIT=$(objpfx)tst-auditmod21.so
+
 # tst-sonamemove links against an older implementation of the library.
 LDFLAGS-tst-sonamemove-linkmod1.so = \
   -Wl,--version-script=tst-sonamemove-linkmod1.map \
diff --git a/elf/dl-object.c b/elf/dl-object.c
index 1875599eb2..eb2158a84b 100644
--- a/elf/dl-object.c
+++ b/elf/dl-object.c
@@ -175,6 +175,9 @@  _dl_new_object (char *realname, const char *libname, int type,
 
   new->l_local_scope[0] = &new->l_searchlist;
 
+  if (mode & __RTLD_AUDIT)
+    new->l_dont_set_tls_static = 1;
+
   /* Determine the origin.  If allocating the link map for the main
      executable, the realname is not known and "".  In this case, the
      origin needs to be determined by other means.  However, in case
diff --git a/elf/dl-tls.c b/elf/dl-tls.c
index 9260d2d696..512ed9a9dc 100644
--- a/elf/dl-tls.c
+++ b/elf/dl-tls.c
@@ -593,10 +593,18 @@  _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.  */
-	  memset (__mempcpy (dest, map->l_tls_initimage,
-			     map->l_tls_initimage_size), '\0',
-		  map->l_tls_blocksize - map->l_tls_initimage_size);
+	  /* Copy the initialization image and clear the BSS part.  For
+	     ldaudit modules or depedencies with initial-exec TLS, we can not
+	     set the initial TLS image on default loader initialization
+	     because it would already be set by the ldaudit setup.  However,
+	     subsequent thread creation would need to follow the default
+	     behaviour.   */
+	  if (__glibc_unlikely (!map->l_dont_set_tls_static))
+	    memset (__mempcpy (dest, map->l_tls_initimage,
+			       map->l_tls_initimage_size), '\0',
+		    map->l_tls_blocksize - map->l_tls_initimage_size);
+	  else
+	    map->l_dont_set_tls_static = 0;
 	}
 
       total += cnt;
diff --git a/elf/rtld.c b/elf/rtld.c
index 8953347b00..db1817655f 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -1055,6 +1055,8 @@  ERROR: audit interface '%s' requires version %d (maximum supported version %d);
 
   /* Mark the DSO as being used for auditing.  */
   dlmargs.map->l_auditing = 1;
+  /* Mark the DSO to not clear the TLS bss in tls initialization.  */
+  dlmargs.map->l_dont_set_tls_static = 1;
 }
 
 /* Notify the the audit modules that the object MAP has already been
diff --git a/elf/tst-audit21.c b/elf/tst-audit21.c
new file mode 100644
index 0000000000..307cb6fc3b
--- /dev/null
+++ b/elf/tst-audit21.c
@@ -0,0 +1,42 @@ 
+/* Check DT_AUDIT with static TLS.
+   Copyright (C) 2021 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-auditmod21.c b/elf/tst-auditmod21.c
new file mode 100644
index 0000000000..e6248622f4
--- /dev/null
+++ b/elf/tst-auditmod21.c
@@ -0,0 +1,69 @@ 
+/* Check DT_AUDIT with static TLS.
+   Copyright (C) 2021 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;
+
+static volatile int out;
+
+static void
+call_libc (void)
+{
+  /* isspace() access 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;
+  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/include/link.h b/include/link.h
index 6a9f788d2b..ba4c588873 100644
--- a/include/link.h
+++ b/include/link.h
@@ -195,6 +195,8 @@  struct link_map
     unsigned int l_need_tls_init:1; /* Nonzero if GL(dl_init_static_tls)
 				       should be called on this link map
 				       when relocation finishes.  */
+    unsigned int l_dont_set_tls_static:1; /* Non zero if static TLS setup should
+					     not be initialized.  */
     unsigned int l_auditing:1;	/* Nonzero if the DSO is used in auditing.  */
     unsigned int l_audit_any_plt:1; /* Nonzero if at least one audit module
 				       is interested in the PLT interception.*/