Fix ifunc support with DT_TEXTREL segments (BZ#20480)

Currently, DT_TEXTREL is incompatible with IFUNC.  When DT_TEXTREL or
DF_TEXTREL is seen, the dynamic linker calls __mprotect on the segments
with PROT_READ|PROT_WRITE before applying dynamic relocations. It leads
to segfault when performing IFUNC resolution (which requires PROT_EXEC
as well for the IFUNC resolver).

This patch makes it call __mprotect with extra PROT_WRITE bit, which
will keep the PROT_EXEC bit if exists, and thus fixes the segfault.
FreeBSD rtld libexec/rtld-elf/rtld.c (reloc_textrel_prot) does the same.

Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
sparc64-linux-gnu, sparcv9-linux-gnu, and armv8-linux-gnueabihf.

	Adam J. Richte  <adam_richter2004@yahoo.com>
	Adhemerval Zanella  <adhemerval.zanella@linaro.org>
	Fangrui Song  <maskray@google.com>

	[BZ #20480]
	* config.h.in (CAN_TEXTREL_IFUNC): New define.
	* configure.ac: Add check if linker supports textrel relocation with
	ifunc.
	* elf/dl-reloc.c (_dl_relocate_object): Use all required flags on
	DT_TEXTREL segments, not only PROT_READ and PROT_WRITE.
	* elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel.
	(CFLAGS-tst-ifunc-textrel.c): New rule.
	* elf/tst-ifunc-textrel.c: New file.
---
 ChangeLog               | 14 +++++++++++
 config.h.in             |  3 +++
 configure               | 51 +++++++++++++++++++++++++++++++++++++++
 configure.ac            | 38 +++++++++++++++++++++++++++++
 elf/Makefile            |  4 +++-
 elf/dl-reloc.c          | 20 +++++++---------
 elf/tst-ifunc-textrel.c | 53 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 170 insertions(+), 13 deletions(-)
 create mode 100644 elf/tst-ifunc-textrel.c

Ping.

On 30/08/2018 13:07, Adhemerval Zanella wrote:
> Currently, DT_TEXTREL is incompatible with IFUNC.  When DT_TEXTREL or
> DF_TEXTREL is seen, the dynamic linker calls __mprotect on the segments
> with PROT_READ|PROT_WRITE before applying dynamic relocations. It leads
> to segfault when performing IFUNC resolution (which requires PROT_EXEC
> as well for the IFUNC resolver).
> 
> This patch makes it call __mprotect with extra PROT_WRITE bit, which
> will keep the PROT_EXEC bit if exists, and thus fixes the segfault.
> FreeBSD rtld libexec/rtld-elf/rtld.c (reloc_textrel_prot) does the same.
> 
> Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
> sparc64-linux-gnu, sparcv9-linux-gnu, and armv8-linux-gnueabihf.
> 
> 	Adam J. Richte  <adam_richter2004@yahoo.com>
> 	Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 	Fangrui Song  <maskray@google.com>
> 
> 	[BZ #20480]
> 	* config.h.in (CAN_TEXTREL_IFUNC): New define.
> 	* configure.ac: Add check if linker supports textrel relocation with
> 	ifunc.
> 	* elf/dl-reloc.c (_dl_relocate_object): Use all required flags on
> 	DT_TEXTREL segments, not only PROT_READ and PROT_WRITE.
> 	* elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel.
> 	(CFLAGS-tst-ifunc-textrel.c): New rule.
> 	* elf/tst-ifunc-textrel.c: New file.
> ---
>  ChangeLog               | 14 +++++++++++
>  config.h.in             |  3 +++
>  configure               | 51 +++++++++++++++++++++++++++++++++++++++
>  configure.ac            | 38 +++++++++++++++++++++++++++++
>  elf/Makefile            |  4 +++-
>  elf/dl-reloc.c          | 20 +++++++---------
>  elf/tst-ifunc-textrel.c | 53 +++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 170 insertions(+), 13 deletions(-)
>  create mode 100644 elf/tst-ifunc-textrel.c
> 
> diff --git a/config.h.in b/config.h.in
> index 141db213a9..d6fcbbac78 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -247,4 +247,7 @@
>     in i386 6 argument syscall issue).  */
>  #define CAN_USE_REGISTER_ASM_EBP 0
>  
> +/* Linker supports ifunc with text relocation (used in tests).  */
> +#define CAN_TEXTREL_IFUNC 0
> +
>  #endif
> diff --git a/configure b/configure
> index 285a6537f0..ab4c12c5ba 100755
> --- a/configure
> +++ b/configure
> @@ -664,6 +664,7 @@ INSTALL_PROGRAM
>  sysnames
>  submachine
>  multi_arch
> +libc_cv_textrel_ifunc
>  no_stack_protector
>  stack_protector
>  libc_cv_ssp
> @@ -4052,6 +4053,56 @@ fi
>  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_cv_gcc_indirect_function" >&5
>  $as_echo "$libc_cv_gcc_indirect_function" >&6; }
>  
> +# Check if linker supports textrel relocation with ifunc (used on elf/tests).
> +# Note that it relies on libc_cv_ld_gnu_indirect_function test above.
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether the linker supports textrels along with ifunc" >&5
> +$as_echo_n "checking whether the linker supports textrels along with ifunc... " >&6; }
> +if ${libc_cv_textrel_ifunc+:} false; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  cat > conftest.S <<EOF
> +.type foo,%gnu_indirect_function
> +foo:
> +.globl _start
> +_start:
> +.globl __start
> +__start:
> +.data
> +#ifdef _LP64
> +.quad foo
> +#else
> +.long foo
> +#endif
> +.text
> +.globl address
> +address:
> +#ifdef _LP64
> +.quad address
> +#else
> +.long address
> +#endif
> +EOF
> +libc_cv_textrel_ifunc=no
> +if test $libc_cv_ld_gnu_indirect_function = yes; then
> +   if { ac_try='${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS -nostartfiles -nostdlib $no_ssp -pie -o conftest conftest.S'
> +  { { eval echo "\"\$as_me\":${as_lineno-$LINENO}: \"$ac_try\""; } >&5
> +  (eval $ac_try) 2>&5
> +  ac_status=$?
> +  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> +  test $ac_status = 0; }; }; then
> +     libc_cv_textrel_ifunc=yes
> +   fi
> +fi
> +rm -f conftest*
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_cv_textrel_ifunc" >&5
> +$as_echo "$libc_cv_textrel_ifunc" >&6; }
> +if test $libc_cv_textrel_ifunc = yes; then
> +  $as_echo "#define CAN_TEXTREL_IFUNC 1" >>confdefs.h
> +
> +fi
> +
> +
>  # Check if gcc warns about alias for function with incompatible types.
>  { $as_echo "$as_me:${as_lineno-$LINENO}: checking if compiler warns about alias for function with incompatible types" >&5
>  $as_echo_n "checking if compiler warns about alias for function with incompatible types... " >&6; }
> diff --git a/configure.ac b/configure.ac
> index 8045d44dd0..e3d5c0598b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -671,6 +671,44 @@ if ${CC-cc} -c conftest.c -o conftest.o 1>&AS_MESSAGE_LOG_FD \
>  fi
>  rm -f conftest*])
>  
> +# Check if linker supports textrel relocation with ifunc (used on elf/tests).
> +# Note that it relies on libc_cv_ld_gnu_indirect_function test above.
> +AC_CACHE_CHECK([whether the linker supports textrels along with ifunc],
> +               libc_cv_textrel_ifunc, [dnl
> +cat > conftest.S <<EOF
> +.type foo,%gnu_indirect_function
> +foo:
> +.globl _start
> +_start:
> +.globl __start
> +__start:
> +.data
> +#ifdef _LP64
> +.quad foo
> +#else
> +.long foo
> +#endif
> +.text
> +.globl address
> +address:
> +#ifdef _LP64
> +.quad address
> +#else
> +.long address
> +#endif
> +EOF
> +libc_cv_textrel_ifunc=no
> +if test $libc_cv_ld_gnu_indirect_function = yes; then
> +   if AC_TRY_COMMAND(${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS -nostartfiles -nostdlib $no_ssp -pie -o conftest conftest.S); then
> +     libc_cv_textrel_ifunc=yes
> +   fi
> +fi
> +rm -f conftest*])
> +if test $libc_cv_textrel_ifunc = yes; then
> +  AC_DEFINE(CAN_TEXTREL_IFUNC)
> +fi
> +AC_SUBST(libc_cv_textrel_ifunc)
> +
>  # Check if gcc warns about alias for function with incompatible types.
>  AC_CACHE_CHECK([if compiler warns about alias for function with incompatible types],
>  	       libc_cv_gcc_incompatible_alias, [dnl
> diff --git a/elf/Makefile b/elf/Makefile
> index cd0771307f..77eb6ef104 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -335,7 +335,8 @@ extra-test-objs += $(ifunc-test-modules:=.o)
>  test-internal-extras += $(ifunc-test-modules)
>  ifeq (yes,$(have-fpie))
>  ifunc-pie-tests = ifuncmain1pie ifuncmain1vispie ifuncmain1staticpie \
> -		  ifuncmain5pie ifuncmain6pie ifuncmain7pie
> +		  ifuncmain5pie ifuncmain6pie ifuncmain7pie \
> +		  tst-ifunc-textrel
>  tests-internal += $(ifunc-pie-tests)
>  tests-pie += $(ifunc-pie-tests)
>  endif
> @@ -1265,6 +1266,7 @@ CFLAGS-ifuncmain1staticpie.c += $(pie-ccflag)
>  CFLAGS-ifuncmain5pie.c += $(pie-ccflag)
>  CFLAGS-ifuncmain6pie.c += $(pie-ccflag)
>  CFLAGS-ifuncmain7pie.c += $(pie-ccflag)
> +CFLAGS-tst-ifunc-textrel.c += $(pic-ccflag)
>  
>  $(objpfx)ifuncmain1pie: $(objpfx)ifuncmod1.so
>  $(objpfx)ifuncmain1staticpie: $(objpfx)ifuncdep1pic.o
> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
> index 053916eeae..164f4efa10 100644
> --- a/elf/dl-reloc.c
> +++ b/elf/dl-reloc.c
> @@ -200,17 +200,6 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>  	    newp->start = PTR_ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize))
>  			  + (caddr_t) l->l_addr;
>  
> -	    if (__mprotect (newp->start, newp->len, PROT_READ|PROT_WRITE) < 0)
> -	      {
> -		errstring = N_("cannot make segment writable for relocation");
> -	      call_error:
> -		_dl_signal_error (errno, l->l_name, NULL, errstring);
> -	      }
> -
> -#if (PF_R | PF_W | PF_X) == 7 && (PROT_READ | PROT_WRITE | PROT_EXEC) == 7
> -	    newp->prot = (PF_TO_PROT
> -			  >> ((ph->p_flags & (PF_R | PF_W | PF_X)) * 4)) & 0xf;
> -#else
>  	    newp->prot = 0;
>  	    if (ph->p_flags & PF_R)
>  	      newp->prot |= PROT_READ;
> @@ -218,7 +207,14 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>  	      newp->prot |= PROT_WRITE;
>  	    if (ph->p_flags & PF_X)
>  	      newp->prot |= PROT_EXEC;
> -#endif
> +
> +	    if (__mprotect (newp->start, newp->len, newp->prot|PROT_WRITE) < 0)
> +	      {
> +		errstring = N_("cannot make segment writable for relocation");
> +	      call_error:
> +		_dl_signal_error (errno, l->l_name, NULL, errstring);
> +	      }
> +
>  	    newp->next = textrels;
>  	    textrels = newp;
>  	  }
> diff --git a/elf/tst-ifunc-textrel.c b/elf/tst-ifunc-textrel.c
> new file mode 100644
> index 0000000000..60e5ac3bf8
> --- /dev/null
> +++ b/elf/tst-ifunc-textrel.c
> @@ -0,0 +1,53 @@
> +/* Check DT_TEXTREL/DF_TEXTREL support with ifunc.
> +   Copyright (C) 2018 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/>.  */
> +
> +#include <stdint.h>
> +
> +/* Some linkers may fail to produce a shared object with ifunc and text
> +   relocations.  */
> +#if CAN_TEXTREL_IFUNC
> +/* Force a text relocation in the object.  */
> +static const uintptr_t
> +address __attribute__((section(".text"))) = (uintptr_t) &address;
> +
> +static uintptr_t
> +foo_impl (void)
> +{
> +  return address;
> +}
> +
> +void *
> +__attribute__((noinline))
> +foo (void)
> +{
> +  return (void*) foo_impl;
> +}
> +__asm__ (".type foo, %gnu_indirect_function");
> +#endif
> +
> +static int
> +do_test (void)
> +{
> +#if CAN_TEXTREL_IFUNC
> +  return (uintptr_t) foo () != 0 ? 0 : 1;
> +#else
> +  return 0;
> +#endif
> +}
> +
> +#include <support/test-driver.c>
>

* Adhemerval Zanella:

> 	Adam J. Richte  <adam_richter2004@yahoo.com>
> 	Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 	Fangrui Song  <maskray@google.com>
>
> 	[BZ #20480]
> 	* config.h.in (CAN_TEXTREL_IFUNC): New define.
> 	* configure.ac: Add check if linker supports textrel relocation with
> 	ifunc.
> 	* elf/dl-reloc.c (_dl_relocate_object): Use all required flags on
> 	DT_TEXTREL segments, not only PROT_READ and PROT_WRITE.
> 	* elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel.
> 	(CFLAGS-tst-ifunc-textrel.c): New rule.
> 	* elf/tst-ifunc-textrel.c: New file.

The code change looks okay to me.  Not sure about copyright assignment
needs for this one, but it's probably below the threshold because it's
just moving code.

I would prefer if the new test was disabled at the makefile level, not
with a preprocessor conditional.  But it's not a strong preference.

Thanks,
Florian

On 22/09/2018 09:14, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> 	Adam J. Richte  <adam_richter2004@yahoo.com>
>> 	Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>> 	Fangrui Song  <maskray@google.com>
>>
>> 	[BZ #20480]
>> 	* config.h.in (CAN_TEXTREL_IFUNC): New define.
>> 	* configure.ac: Add check if linker supports textrel relocation with
>> 	ifunc.
>> 	* elf/dl-reloc.c (_dl_relocate_object): Use all required flags on
>> 	DT_TEXTREL segments, not only PROT_READ and PROT_WRITE.
>> 	* elf/Makefile (ifunc-pie-tests): Add tst-ifunc-textrel.
>> 	(CFLAGS-tst-ifunc-textrel.c): New rule.
>> 	* elf/tst-ifunc-textrel.c: New file.
> 
> The code change looks okay to me.  Not sure about copyright assignment
> needs for this one, but it's probably below the threshold because it's
> just moving code.
> 
> I would prefer if the new test was disabled at the makefile level, not
> with a preprocessor conditional.  But it's not a strong preference.

I have changed the patch to your suggestion, thanks for reviewing it.