diff mbox

[BZ,#17075] ARM: Fix immediate calculation of R_ARM_TLS_DESC

Message ID alpine.DEB.1.10.1406192226490.3924@tp.orcam.me.uk
State Superseded
Headers show

Commit Message

Maciej W. Rozycki June 19, 2014, 10:29 p.m. UTC
Hello,

 I've noticed external symbol value calculation made in the dynamic linker 
while processing the R_ARM_TLS_DESC reloc in the immediately-bound case is 
broken.  To figure out the final value of the symbol the calculation makes 
use of the descriptor's argument that in this case is set to the symbol's 
table index in the symbol table ORed with 0x80000000 and adds it to the 
actual value of the symbol resolved by the linker.

 Such calculated value is then used if the static specialisation is 
selected and the typical result is a segfault.  This is easily reproduced 
by trying any program that has (or whose link-time dependency has) 
R_ARM_TLS_DESC relocs against external symbols and running it with 
$LD_BIND_NOW set to 1 in the environment.

 Such calculation is plain wrong and in the case of an external symbol the 
value of the descriptor's argument provided by the static linker can be 
simply discarded as it's only needed by the lazy specialisation to resolve 
the symbol (that in this case already has been).

 This is addressed with the patch below.  A test case is included.  It has 
been verified across a number multilibs, all failing the test case before 
and passing after the fix has been applied.  No regressions, but one'd 
expect that for such a localised change.

 It is worth noting that it is imperative that the $LD_BIND_NOW setting is 
used, it is not enough to link the test case's executable and dynamic 
module with `-Wl,-z,now'.  It may mean that the DT_BIND_NOW dynamic tag 
the `-z now' static linker's option produces is not respected for some 
reason, but I haven't investigated it further.

 The test case required extra `configure' scriptery, because older 
versions of GCC do not support TLS descriptors or the `-mtls-dialect=' 
option.  Additionally GCC does not validate binutils' support for TLS 
decriptors at the build time, so modern GCC built with the effective 
configuration of `--with-tls=gnu' will still work with older binutils and 
accept the `-mtls-dialect=gnu2' option, but the assembler will then choke 
on the lack of support for the syntax used to produce TLS descriptor 
relocations if any code is produced that requests them.  Therefore the 
`configure' test uses a piece of inline assembly that explicitly requests 
a TLS descriptor relocation on a symbol and is guaranteed to break with 
older binutils.  Consequently both GCC and binutils support is verified 
with a single `configure' test.

 OK to apply?

2014-06-19  Maciej W. Rozycki  <macro@codesourcery.com>

	[BZ #17075]
	* sysdeps/arm/dl-machine.h (elf_machine_rel) <R_ARM_TLS_DESC>:
	Fix calculation of the symbol's value.
	* sysdeps/arm/tst-armtlsdesc.c: New file.
	* sysdeps/arm/tst-armtlsdescmod.c: New file
	* sysdeps/arm/Makefile (tests): Add `tst-armtlsdesc'.
	(modules-names): Add `tst-armtlsdescmod'.
	(CFLAGS-tst-armtlsdescmod.c, tst-armtlsdesc-ENV): New variables.
	($(objpfx)tst-armtlsdesc): New dependency.
	* sysdeps/arm/configure.ac: Add a check for tools' GNU descriptor 
	TLS scheme support.
	* config.make.in (have-arm-tls-desc): New variable.
	* configure.ac: Handle it.
	* sysdeps/arm/configure: Regenerate.
	* configure: Regenerate.

  Maciej

glibc-arm-tls-desc-imm.diff

Comments

Joseph Myers June 19, 2014, 11:38 p.m. UTC | #1
On Thu, 19 Jun 2014, Maciej W. Rozycki wrote:

> 	* config.make.in (have-arm-tls-desc): New variable.
> 	* configure.ac: Handle it.

You shouldn't need these changes to architecture-independent files.  Try 
using the LIBC_CONFIG_VAR macro from aclocal.m4 instead.
Rich Felker June 20, 2014, 2:40 a.m. UTC | #2
On Thu, Jun 19, 2014 at 11:29:44PM +0100, Maciej W. Rozycki wrote:
> Index: glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h
> ===================================================================
> --- glibc-fsf-trunk-quilt.orig/sysdeps/arm/dl-machine.h	2014-06-19 22:24:23.731968218 +0100
> +++ glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h	2014-06-19 22:25:06.741661376 +0100
> @@ -452,7 +452,10 @@ elf_machine_rel (struct link_map *map, c
>  	    else
>  # endif
>  	      {
> -		value = sym->st_value + td->argument.value;
> +		if (td->argument.value & 0x80000000)
> +		  value = sym->st_value;
> +		else
> +		  value = td->argument.value;

Can you explain why sym->st_value is completely ignored in the else
case? My understanding is that (aside from the high-bit-set condition
you found) td->argument.value is semantically an addend; I certainly
don't see any way it could already contain the desired symbol value
(offset).

Rich
Maciej W. Rozycki June 20, 2014, 5:02 p.m. UTC | #3
On Fri, 20 Jun 2014, Rich Felker wrote:

> > Index: glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h
> > ===================================================================
> > --- glibc-fsf-trunk-quilt.orig/sysdeps/arm/dl-machine.h	2014-06-19 22:24:23.731968218 +0100
> > +++ glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h	2014-06-19 22:25:06.741661376 +0100
> > @@ -452,7 +452,10 @@ elf_machine_rel (struct link_map *map, c
> >  	    else
> >  # endif
> >  	      {
> > -		value = sym->st_value + td->argument.value;
> > +		if (td->argument.value & 0x80000000)
> > +		  value = sym->st_value;
> > +		else
> > +		  value = td->argument.value;
> 
> Can you explain why sym->st_value is completely ignored in the else
> case? My understanding is that (aside from the high-bit-set condition
> you found) td->argument.value is semantically an addend; I certainly
> don't see any way it could already contain the desired symbol value
> (offset).

 Yes, it's an addend, to the TLS segment's address, which is added later 
on:

		  td->argument.value = value + sym_map->l_tls_offset;

in this code block.  Note that sym->st_value is 0 in the local case 
anyway, which is why current code works for this one.

 However thanks for asking this question -- this is a bug fix that I made 
long ago and you made me refresh some information.  That in turn made me 
realise there are three rather than two cases (local, global lazy and 
global immediate) to be considered here and my fix only handles the two 
formers, regressing the latter.  We also need three test cases to cover 
these variants.  I'll send an update shortly.

  Maciej
diff mbox

Patch

Index: glibc-fsf-trunk-quilt/config.make.in
===================================================================
--- glibc-fsf-trunk-quilt.orig/config.make.in	2014-06-19 22:24:26.231947549 +0100
+++ glibc-fsf-trunk-quilt/config.make.in	2014-06-19 22:25:11.741944867 +0100
@@ -65,6 +65,7 @@  bind-now = @bindnow@
 have-hash-style = @libc_cv_hashstyle@
 use-default-link = @use_default_link@
 output-format = @libc_cv_output_format@
+have-arm-tls-desc = @have_arm_tls_desc@
 
 static-libgcc = @libc_cv_gcc_static_libgcc@
 
Index: glibc-fsf-trunk-quilt/configure
===================================================================
--- glibc-fsf-trunk-quilt.orig/configure	2014-06-19 22:24:26.231947549 +0100
+++ glibc-fsf-trunk-quilt/configure	2014-06-19 22:25:12.241747789 +0100
@@ -581,6 +581,7 @@  shared
 static
 ldd_rewrite_script
 use_ldconfig
+have_arm_tls_desc
 libc_cv_forced_unwind
 libc_cv_rootsbindir
 libc_cv_localstatedir
@@ -7225,6 +7226,7 @@  libc_cv_sysconfdir=$sysconfdir
 libc_cv_localstatedir=$localstatedir
 libc_cv_gcc_unwind_find_fde=no
 libc_cv_idn=no
+have_arm_tls_desc=no
 
 # Iterate over all the sysdep directories we will use, running their
 # configure fragments.
@@ -7295,6 +7297,8 @@  fi
 
 
 
+
+
 if test x$use_ldconfig = xyes; then
   $as_echo "#define USE_LDCONFIG 1" >>confdefs.h
 
Index: glibc-fsf-trunk-quilt/configure.ac
===================================================================
--- glibc-fsf-trunk-quilt.orig/configure.ac	2014-06-19 22:24:26.231947549 +0100
+++ glibc-fsf-trunk-quilt/configure.ac	2014-06-19 22:25:12.241747789 +0100
@@ -2065,6 +2065,7 @@  libc_cv_sysconfdir=$sysconfdir
 libc_cv_localstatedir=$localstatedir
 libc_cv_gcc_unwind_find_fde=no
 libc_cv_idn=no
+have_arm_tls_desc=no
 
 # Iterate over all the sysdep directories we will use, running their
 # configure fragments.
@@ -2127,6 +2128,8 @@  AC_SUBST(libc_cv_localstatedir)
 AC_SUBST(libc_cv_rootsbindir)
 AC_SUBST(libc_cv_forced_unwind)
 
+AC_SUBST(have_arm_tls_desc)
+
 if test x$use_ldconfig = xyes; then
   AC_DEFINE(USE_LDCONFIG)
 fi
Index: glibc-fsf-trunk-quilt/sysdeps/arm/Makefile
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/Makefile	2014-06-19 22:24:26.231947549 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/Makefile	2014-06-19 22:25:12.741947016 +0100
@@ -11,6 +11,16 @@  $(objpfx)libgcc-stubs.a: $(objpfx)aeabi_
 	$(build-extra-lib)
 
 lib-noranlib: $(objpfx)libgcc-stubs.a
+
+ifeq ($(build-shared),yes)
+ifeq ($(have-arm-tls-desc),yes)
+tests += tst-armtlsdesc
+modules-names += tst-armtlsdescmod
+CFLAGS-tst-armtlsdescmod.c += -mtls-dialect=gnu2
+tst-armtlsdesc-ENV = LD_BIND_NOW=1
+$(objpfx)tst-armtlsdesc: $(objpfx)tst-armtlsdescmod.so
+endif
+endif
 endif
 
 ifeq ($(subdir),csu)
Index: glibc-fsf-trunk-quilt/sysdeps/arm/configure
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/configure	2014-06-19 22:24:26.231947549 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/configure	2014-06-19 22:25:12.741947016 +0100
@@ -203,3 +203,33 @@  else
   config_vars="$config_vars
 default-abi = soft"
 fi
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether the build tools support the GNU descriptor TLS scheme" >&5
+$as_echo_n "checking whether the build tools support the GNU descriptor TLS scheme... " >&6; }
+if ${libc_cv_arm_tls_desc+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  old_CFLAGS="$CFLAGS"
+  CFLAGS="$CFLAGS -mtls-dialect=gnu2"
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+asm (".word\tfoo(tlsdesc)");
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  libc_cv_arm_tls_desc=yes
+else
+  libc_cv_arm_tls_desc=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+  CFLAGS="$old_CFLAGS"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_cv_arm_tls_desc" >&5
+$as_echo "$libc_cv_arm_tls_desc" >&6; }
+have_arm_tls_desc=$libc_cv_arm_tls_desc
Index: glibc-fsf-trunk-quilt/sysdeps/arm/configure.ac
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/configure.ac	2014-06-19 22:24:26.231947549 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/configure.ac	2014-06-19 22:25:12.741947016 +0100
@@ -44,3 +44,12 @@  if test $libc_cv_arm_pcs_vfp = yes; then
 else
   LIBC_CONFIG_VAR([default-abi], [soft])
 fi
+
+AC_CACHE_CHECK([whether the build tools support the GNU descriptor TLS scheme],
+  [libc_cv_arm_tls_desc],
+  [old_CFLAGS="$CFLAGS"
+  CFLAGS="$CFLAGS -mtls-dialect=gnu2"
+  AC_COMPILE_IFELSE([AC_LANG_PROGRAM([asm (".word\tfoo(tlsdesc)");], [])],
+    [libc_cv_arm_tls_desc=yes], [libc_cv_arm_tls_desc=no])
+  CFLAGS="$old_CFLAGS"])
+have_arm_tls_desc=$libc_cv_arm_tls_desc
Index: glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/dl-machine.h	2014-06-19 22:24:23.731968218 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h	2014-06-19 22:25:06.741661376 +0100
@@ -452,7 +452,10 @@  elf_machine_rel (struct link_map *map, c
 	    else
 # endif
 	      {
-		value = sym->st_value + td->argument.value;
+		if (td->argument.value & 0x80000000)
+		  value = sym->st_value;
+		else
+		  value = td->argument.value;
 
 # ifndef RTLD_BOOTSTRAP
 #  ifndef SHARED
Index: glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdesc.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdesc.c	2014-06-19 22:25:12.741947016 +0100
@@ -0,0 +1,28 @@ 
+/* ARM immediate binding GNU TLS descriptor relocation test.
+   Copyright (C) 2013-2014 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
+   <http://www.gnu.org/licenses/>.  */
+
+int getfoo (void);
+
+int
+do_test (void)
+{
+  return getfoo ();
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
Index: glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescmod.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescmod.c	2014-06-19 22:25:12.741947016 +0100
@@ -0,0 +1,25 @@ 
+/* DSO used for ARM immediate binding GNU TLS descriptor relocation test.
+   Copyright (C) 2013-2014 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
+   <http://www.gnu.org/licenses/>.  */
+
+int __thread foo;
+
+int
+getfoo (void)
+{
+  return foo;
+}