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

Message ID alpine.DEB.1.10.1406201803130.25395@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki June 20, 2014, 5:21 p.m. UTC
Hello,

 I've noticed external symbol value calculation made in the dynamic linker 
is broken while processing the R_ARM_TLS_DESC reloc in the global
immediately-bound case, where code has been originally linked for lazy 
binding.  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 has been linked with 
`-Wl,-z,lazy' in effect (no DT_BIND_NOW dynamic tag present), and running 
it with $LD_BIND_NOW set to 1 in the environment.

 Such calculation is plain wrong and in the case of a lazily-bound 
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.  Three test cases are included, 
to cover the local, global lazy and global immediate references.  It has 
been verified across a number multilibs, all failing the global lazy test 
case before and passing after the fix has been applied.  No regressions, 
but one'd expect that for such a localised change.

 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.

 There is no need to verify binutils' support for the `-z lazy' linker 
option as all the versions that support TLS descriptors also support the 
said option (it has been present since 2000).

 This is an updated version, changes from version 1:

1. Joseph's note about using LIBC_CONFIG_VAR taken into account, no more 
   changes outside sysdeps/arm/.

2. The global immediate case taken into account and the regression 
   introduced by the previous revision of this change fixed.  Test cases
   added to handle all the three kinds of references: local, global lazy
   and global immediate.  The latter test case covers the regression with 
   the previous revision of this change.

 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-armtlsdescloc.c: New file.
	* sysdeps/arm/tst-armtlsdesclocmod.c: New file.
	* sysdeps/arm/tst-armtlsdescextnow.c: New file.
	* sysdeps/arm/tst-armtlsdescextlazymod.c: New file.
	* sysdeps/arm/tst-armtlsdescextlazy.c: New file.
	* sysdeps/arm/tst-armtlsdescextnowmod.c: New file.
	* sysdeps/arm/Makefile (tests): Add `tst-armtlsdesc',
	`tst-armtlsdescextnow' and `tst-armtlsdescextlazy'.
	(modules-names): Add `tst-armtlsdescmod',
	`tst-armtlsdescextlazymod' and `tst-armtlsdescextnowmod'.
	(CPPFLAGS-tst-armtlsdescextnowmod.c): New variable.
	(CPPFLAGS-tst-armtlsdescextlazymod.c): Likewise.
	(CFLAGS-tst-armtlsdesclocmod.c): Likewise.
	(CFLAGS-tst-armtlsdescextnowmod.c): Likewise.
	(CFLAGS-tst-armtlsdescextlazymod.c): Likewise.
	(LDFLAGS-tst-armtlsdescextnowmod.so): Likewise.
	($(objpfx)tst-armtlsdescloc): New dependency.
	($(objpfx)tst-armtlsdescextnow): Likewise.
	($(objpfx)tst-armtlsdescextlazy): Likewise.
	* sysdeps/arm/configure.ac: Add a check for tools' GNU descriptor 
	TLS scheme support.
	* sysdeps/arm/configure: Regenerate.

  Maciej

glibc-arm-tls-desc-imm.diff

Comments

Joseph Myers June 20, 2014, 5:39 p.m. UTC | #1
On Fri, 20 Jun 2014, Maciej W. Rozycki wrote:

>  This is an updated version, changes from version 1:
> 
> 1. Joseph's note about using LIBC_CONFIG_VAR taken into account, no more 
>    changes outside sysdeps/arm/.
> 
> 2. The global immediate case taken into account and the regression 
>    introduced by the previous revision of this change fixed.  Test cases
>    added to handle all the three kinds of references: local, global lazy
>    and global immediate.  The latter test case covers the regression with 
>    the previous revision of this change.
> 
>  OK to apply?

OK.
Rich Felker June 20, 2014, 5:40 p.m. UTC | #2
On Fri, Jun 20, 2014 at 06:21:29PM +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-20 05:46:45.000000000 +0100
> +++ glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h	2014-06-20 07:08:08.301787149 +0100
> @@ -452,7 +452,10 @@ elf_machine_rel (struct link_map *map, c
>  	    else
>  # endif
>  	      {
> -		value = sym->st_value + td->argument.value;
> +		if (ELF32_R_SYM (reloc->r_info) == STN_UNDEF)
> +		  value = td->argument.value;
> +		else
> +		  value = sym->st_value;

Is this logic correct in general, i.e. is the immediate addend always
(semantically) zero for symbolic relocations? Or is it legal to have a
symbolic relocation with an offset, e.g. if a module accesses x[42];
in __thread int x[50]; but no other elements and the compiler wants to
generate a relocation referencing x[42] without the need for applying
an additional offset to the address of x[0] at runtime as part of the
referencing code?

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

> Is this logic correct in general, i.e. is the immediate addend always
> (semantically) zero for symbolic relocations? Or is it legal to have a
> symbolic relocation with an offset, e.g. if a module accesses x[42];
> in __thread int x[50]; but no other elements and the compiler wants to
> generate a relocation referencing x[42] without the need for applying
> an additional offset to the address of x[0] at runtime as part of the
> referencing code?

 This relocation has no addend, please refer to the specification, 
attached to the bug report.

  Maciej
Maciej W. Rozycki June 20, 2014, 7:25 p.m. UTC | #4
On Fri, 20 Jun 2014, Joseph S. Myers wrote:

> >  This is an updated version, changes from version 1:
> > 
> > 1. Joseph's note about using LIBC_CONFIG_VAR taken into account, no more 
> >    changes outside sysdeps/arm/.
> > 
> > 2. The global immediate case taken into account and the regression 
> >    introduced by the previous revision of this change fixed.  Test cases
> >    added to handle all the three kinds of references: local, global lazy
> >    and global immediate.  The latter test case covers the regression with 
> >    the previous revision of this change.
> > 
> >  OK to apply?
> 
> OK.

 Committed now (with dates in the test cases amended to show `2014' rather 
than `2013-2014').  Thanks for your review.

  Maciej

Patch
diff mbox

Index: glibc-fsf-trunk-quilt/sysdeps/arm/Makefile
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/Makefile	2014-06-20 05:46:45.000000000 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/Makefile	2014-06-20 06:53:01.661995040 +0100
@@ -11,6 +11,26 @@  $(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-armtlsdescloc tst-armtlsdescextnow tst-armtlsdescextlazy
+modules-names += tst-armtlsdesclocmod
+modules-names += tst-armtlsdescextlazymod tst-armtlsdescextnowmod
+CPPFLAGS-tst-armtlsdescextnowmod.c += -Dstatic=
+CPPFLAGS-tst-armtlsdescextlazymod.c += -Dstatic=
+CFLAGS-tst-armtlsdesclocmod.c += -mtls-dialect=gnu2
+CFLAGS-tst-armtlsdescextnowmod.c += -mtls-dialect=gnu2
+CFLAGS-tst-armtlsdescextlazymod.c += -mtls-dialect=gnu2
+LDFLAGS-tst-armtlsdescextnowmod.so += -Wl,-z,now
+tst-armtlsdescloc-ENV = LD_BIND_NOW=1
+tst-armtlsdescextnow-ENV = LD_BIND_NOW=1
+tst-armtlsdescextlazy-ENV = LD_BIND_NOW=1
+$(objpfx)tst-armtlsdescloc: $(objpfx)tst-armtlsdesclocmod.so
+$(objpfx)tst-armtlsdescextnow: $(objpfx)tst-armtlsdescextnowmod.so
+$(objpfx)tst-armtlsdescextlazy: $(objpfx)tst-armtlsdescextlazymod.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-20 05:46:45.000000000 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/configure	2014-06-20 05:52:38.571931573 +0100
@@ -203,3 +203,39 @@  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; }
+if test $libc_cv_arm_tls_desc = yes; then
+  config_vars="$config_vars
+have-arm-tls-desc = yes"
+else
+  config_vars="$config_vars
+have-arm-tls-desc = no"
+fi
Index: glibc-fsf-trunk-quilt/sysdeps/arm/configure.ac
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/configure.ac	2014-06-20 05:46:45.000000000 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/configure.ac	2014-06-20 05:52:38.571931573 +0100
@@ -44,3 +44,16 @@  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"])
+if test $libc_cv_arm_tls_desc = yes; then
+  LIBC_CONFIG_VAR([have-arm-tls-desc], [yes])
+else
+  LIBC_CONFIG_VAR([have-arm-tls-desc], [no])
+fi
Index: glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h
===================================================================
--- glibc-fsf-trunk-quilt.orig/sysdeps/arm/dl-machine.h	2014-06-20 05:46:45.000000000 +0100
+++ glibc-fsf-trunk-quilt/sysdeps/arm/dl-machine.h	2014-06-20 07:08:08.301787149 +0100
@@ -452,7 +452,10 @@  elf_machine_rel (struct link_map *map, c
 	    else
 # endif
 	      {
-		value = sym->st_value + td->argument.value;
+		if (ELF32_R_SYM (reloc->r_info) == STN_UNDEF)
+		  value = td->argument.value;
+		else
+		  value = sym->st_value;
 
 # ifndef RTLD_BOOTSTRAP
 #  ifndef SHARED
Index: glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextlazy.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextlazy.c	2014-06-20 05:59:57.641975761 +0100
@@ -0,0 +1 @@ 
+#include "tst-armtlsdescloc.c"
Index: glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextlazymod.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextlazymod.c	2014-06-20 06:51:44.141980570 +0100
@@ -0,0 +1 @@ 
+#include "tst-armtlsdesclocmod.c"
Index: glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextnow.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextnow.c	2014-06-20 06:01:07.151978683 +0100
@@ -0,0 +1 @@ 
+#include "tst-armtlsdescloc.c"
Index: glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextnowmod.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescextnowmod.c	2014-06-20 06:51:34.141867198 +0100
@@ -0,0 +1 @@ 
+#include "tst-armtlsdesclocmod.c"
Index: glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescloc.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdescloc.c	2014-06-20 05:52:38.571931573 +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-armtlsdesclocmod.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc-fsf-trunk-quilt/sysdeps/arm/tst-armtlsdesclocmod.c	2014-06-20 06:51:07.641787978 +0100
@@ -0,0 +1,44 @@ 
+/* 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/>.  */
+
+static int __thread bar = 1;
+static int __thread foo;
+
+int
+getfoo (void)
+{
+  return foo;
+}
+
+void
+setfoo (int i)
+{
+  foo = 1;
+}
+
+int
+getbar (void)
+{
+  return bar;
+}
+
+void
+setbar (int i)
+{
+  bar = 1;
+}