elf: Apply attribute_relro to pointers in elf/dl-minimal.c

The present code leaves the function pointers unprotected, but moves
some of the static functions into .data.rel.ro instead.  This causes
the linker to produce an allocatable, executable, writable section
and eventually an RWX load segment.  Not only do we really do not
want that, it also breaks valgrind because valgrind does not load
debuginfo from the mmap interceptor if all it sees are RX and RWX
mappings.

-----
 elf/dl-minimal.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

On Wed, Feb 26, 2020 at 7:18 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> The present code leaves the function pointers unprotected, but moves
> some of the static functions into .data.rel.ro instead.  This causes
> the linker to produce an allocatable, executable, writable section
> and eventually an RWX load segment.  Not only do we really do not
> want that, it also breaks valgrind because valgrind does not load
> debuginfo from the mmap interceptor if all it sees are RX and RWX
> mappings.
>
> -----
>  elf/dl-minimal.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c
> index c79ce23be4..7c64e24c87 100644
> --- a/elf/dl-minimal.c
> +++ b/elf/dl-minimal.c
> @@ -39,16 +39,16 @@
>    implementation below.  Before the final relocation,
>    __rtld_malloc_init_real is called to replace the pointers with the
>    real implementation.  */
> -__typeof (calloc) *__rtld_calloc;
> -__typeof (free) *__rtld_free;
> -__typeof (malloc) *__rtld_malloc;
> -__typeof (realloc) *__rtld_realloc;
> +__typeof (calloc) *__rtld_calloc attribute_relro;
> +__typeof (free) *__rtld_free attribute_relro;
> +__typeof (malloc) *__rtld_malloc attribute_relro;
> +__typeof (realloc) *__rtld_realloc attribute_relro;
>
>  /* Defined below.  */
> -static __typeof (calloc) rtld_calloc attribute_relro;
> -static __typeof (free) rtld_free attribute_relro;
> -static __typeof (malloc) rtld_malloc attribute_relro;
> -static __typeof (realloc) rtld_realloc attribute_relro;
> +static __typeof (calloc) rtld_calloc;
> +static __typeof (free) rtld_free;
> +static __typeof (malloc) rtld_malloc;
> +static __typeof (realloc) rtld_realloc;
>
>  void
>  __rtld_malloc_init_stubs (void)
>

LGTM.

Thanks.

This change (commit 758599bc9dcc5764e862bd9e1613c5d1e6efc5d3) breaks the 
build for alpha-linux-gnu with GCC and binutils mainline.  I get a series 
of errors of the form (this is in the "compilers" glibc build):

/scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os: 
in function `calloc':
/scratch/jmyers/glibc/many10/src/glibc/elf/../include/rtld-malloc.h:44:(.text+0xd98): 
relocation truncated to fit: GPREL16 against symbol `__rtld_calloc' 
defined in .data.rel.ro section in 
/scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os
/scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os: 
in function `malloc':
/scratch/jmyers/glibc/many10/src/glibc/elf/../include/rtld-malloc.h:56:(.text+0x2978): 
relocation truncated to fit: GPREL16 against symbol `__rtld_malloc' 
defined in .data.rel.ro section in 
/scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os

https://sourceware.org/ml/libc-testresults/2020-q1/msg00270.html

Reverting to the previous glibc commit, while keeping other components the 
same, eliminates that build failure.

I don't know whether the problem lies in GCC, binutils or glibc (but 
https://sourceware.org/ml/libc-testresults/2020-q1/msg00271.html shows it 
doesn't appear with GCC 9 branch and binutils 2.34 branch).

* Joseph Myers:

> This change (commit 758599bc9dcc5764e862bd9e1613c5d1e6efc5d3) breaks the 
> build for alpha-linux-gnu with GCC and binutils mainline.  I get a series 
> of errors of the form (this is in the "compilers" glibc build):
>
> /scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os: 
> in function `calloc':
> /scratch/jmyers/glibc/many10/src/glibc/elf/../include/rtld-malloc.h:44:(.text+0xd98): 
> relocation truncated to fit: GPREL16 against symbol `__rtld_calloc' 
> defined in .data.rel.ro section in 
> /scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os
> /scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os: 
> in function `malloc':
> /scratch/jmyers/glibc/many10/src/glibc/elf/../include/rtld-malloc.h:56:(.text+0x2978): 
> relocation truncated to fit: GPREL16 against symbol `__rtld_malloc' 
> defined in .data.rel.ro section in 
> /scratch/jmyers/glibc/many10/build/compilers/alpha-linux-gnu/glibc/alpha-linux-gnu/elf/librtld.os

I don't know much about alpha unfortunately.  Running this

$ objdump -d --reloc /lib/ld-linux.so.2  | grep -E  'lda\s.*gp,'

on a default build (not mainline, so successful) gives me what I
assume are GP-relative load instructions ranging from

    9ff4:       40 80 bd 23     lda     gp,-32704(gp)

to

    a040:       f4 7f bd 23     lda     gp,32756(gp)

That seems to be rather close to the 64 KiB displacement that alpha
apparently supports.

This suggests me that we aren't hitting a bug as such, but that we
were already rather near the limit before, and the additional PLT
avoidance that the patch brought finally moved us across the limit.

Would it be acceptable to switch from -fpic to -fPIC to fix this
(if it addresses the issue)?

diff --git a/sysdeps/alpha/Makefile b/sysdeps/alpha/Makefile
index da52c1d4d1..baf5d480e5 100644
--- a/sysdeps/alpha/Makefile
+++ b/sysdeps/alpha/Makefile
@@ -57,10 +57,6 @@ endif
 # "current" rounding mode, and it's easiest to set this with all of them.
 sysdep-CFLAGS += -mieee -mfp-rounding-mode=d
 
-# libc.so requires about 16k for the small data area, which is well
-# below the 64k maximum.
-pic-ccflag = -fpic
-
 #  Software floating-point emulation.
 
 ifeq ($(subdir),soft-fp)

* Florian Weimer:

> Would it be acceptable to switch from -fpic to -fPIC to fix this
> (if it addresses the issue)?
>
> diff --git a/sysdeps/alpha/Makefile b/sysdeps/alpha/Makefile
> index da52c1d4d1..baf5d480e5 100644
> --- a/sysdeps/alpha/Makefile
> +++ b/sysdeps/alpha/Makefile
> @@ -57,10 +57,6 @@ endif
>  # "current" rounding mode, and it's easiest to set this with all of them.
>  sysdep-CFLAGS += -mieee -mfp-rounding-mode=d
>  
> -# libc.so requires about 16k for the small data area, which is well
> -# below the 64k maximum.
> -pic-ccflag = -fpic
> -
>  #  Software floating-point emulation.
>  
>  ifeq ($(subdir),soft-fp)

alpha builds again with this patch.

I also tried to remove the direct __ehdr_start reference in rtld.c:

  /* Starting from binutils-2.23, the linker will define the magic symbol
     __ehdr_start to point to our own ELF header if it is visible in a
     segment that also includes the phdrs.  If that's not available, we use
     the old method that assumes the beginning of the file is part of the
     lowest-addressed PT_LOAD segment.  */
#ifdef HAVE_EHDR_START
  extern const ElfW(Ehdr) __ehdr_start __attribute__ ((visibility ("hidden")));
  rtld_ehdr = &__ehdr_start;
#else
  rtld_ehdr = (void *) GL(dl_rtld_map).l_map_start;
#endif

It looks like something that could be problematic to represent due to
section ordering.  But getting rid of that (either by reverting to the
old mechanism or introducing a level of indirection) is not sufficient
to address the build failure.

I'd prefer to use -fPIC for now.  If someone finds a way to make -fpic
work, than we can of course consider making that change.

* Florian Weimer:

> I don't know much about alpha unfortunately.  Running this
>
> $ objdump -d --reloc /lib/ld-linux.so.2  | grep -E  'lda\s.*gp,'
>
> on a default build (not mainline, so successful) gives me what I
> assume are GP-relative load instructions ranging from
>
>     9ff4:       40 80 bd 23     lda     gp,-32704(gp)
>
> to
>
>     a040:       f4 7f bd 23     lda     gp,32756(gp)
>
> That seems to be rather close to the 64 KiB displacement that alpha
> apparently supports.

The above isn't correct.  These instructions are half of the gp
register setup.  There are more bits available in a different
instruction.

I still beleive this is a current toolchain limitation.  I tried
linking with -Wl,--no-relax and also skipping the librtld.os stage,
but this didn't fix this.  I think we have no choice but to drop -fpic
at this point and use -fPIC.