[PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[H.J. Lu]

PLT entries aren't required when -z now used.  Linker on master with:

commit 25070364b0ce33eed46aa5d78ebebbec6accec7e
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Sat May 16 07:00:21 2015 -0700

    Don't generate PLT relocations for now binding

    There is no need for PLT relocations with -z now. We can use GOT
    relocations, which take less space, instead and replace 16-byte .plt
    entres with 8-byte .plt.got entries.

    bfd/

      * elf32-i386.c (elf_i386_check_relocs): Create .plt.got section
      for now binding.
      (elf_i386_allocate_dynrelocs): Use .plt.got section for now
      binding.
      * elf64-x86-64.c (elf_x86_64_check_relocs): Create .plt.got
      section for now binding.
      (elf_x86_64_allocate_dynrelocs): Use .plt.got section for now
      binding.

won't generate PLT entries with -z now.  elf/tst-audit2.c has

/* This calloc definition will be called by the dynamic linker itself.
   We test that it has initialized our TLS block by the time it does so.
*/

void *
calloc (size_t n, size_t m)
{
  if (magic[0] != MAGIC1 || magic[1] != MAGIC2)
    {
      printf ("{%x, %x} != {%x, %x}\n", magic[0], magic[1], MAGIC1, MAGIC2);
      abort ();
    }
  magic[0] = MAGIC2;
  magic[1] = MAGIC1;

Since ld.so is built with -z now, there are no PLT relocations and this
calloc won't be used:

Relocation section '.rela.dyn' at offset 0x66c contains 16 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00222e88  00000008 R_X86_64_RELATIVE            10970
00222e8c  00000008 R_X86_64_RELATIVE            f8f0
00222e90  00000008 R_X86_64_RELATIVE            f6c0
00222e94  00000008 R_X86_64_RELATIVE            11bd0
00222e98  00000008 R_X86_64_RELATIVE            a1b0
00222e9c  00000008 R_X86_64_RELATIVE            130c0
00222ea0  00000008 R_X86_64_RELATIVE            13c50
00222ea4  00000008 R_X86_64_RELATIVE            15fc0
00222ea8  00000008 R_X86_64_RELATIVE            12cd0
00222eac  00000008 R_X86_64_RELATIVE            17f90
00222fbc  00000a06 R_X86_64_GLOB_DAT 00018230	__libc_memalign@@GLIBC_2.16 + 0
00222fc4  00001506 R_X86_64_GLOB_DAT 00018300   malloc@@GLIBC_2.16 + 0
00222fcc  00000d06 R_X86_64_GLOB_DAT 00018310   calloc@@GLIBC_2.16 + 0
00222fd4  00000506 R_X86_64_GLOB_DAT 000184a0   realloc@@GLIBC_2.16 + 0
00222fdc  00000706 R_X86_64_GLOB_DAT 002239a0   _r_debug@@GLIBC_2.16 + 0
00222fe4  00000406 R_X86_64_GLOB_DAT 00018340   free@@GLIBC_2.16 + 0

Assuming we do want to keep PLT relocations in ld.so so that malloc
functions in ld.so can be overridden, ld.so should be built with -z now.
There is no reason to build ld.so with -z now since ld.so is the one
doing BIND_NOW.  The only thing we get with -z now on ld.so is DT tag:

 0x0000000000000018 (BIND_NOW)
 0x000000006ffffffb (FLAGS_1)            Flags: NOW

This patch removes -Wl,-z,now from ld.so build.

OK for master?

H.J.
	[BZ #18422]
	* elf/Makefile (z-now-yes): Removed.
	($(objpfx)ld.so): Remove $(z-now-$(bind-now)).
---
 elf/Makefile | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/elf/Makefile b/elf/Makefile
index 34450ea..324b4a2 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -349,13 +349,11 @@ $(objpfx)librtld.os: $(objpfx)dl-allobjs.os $(objpfx)rtld-libc.a
 
 generated += librtld.map librtld.mk rtld-libc.a librtld.os.map
 
-z-now-yes = -Wl,-z,now
-
 $(objpfx)ld.so: $(objpfx)librtld.os $(ld-map)
 # Link into a temporary file so that we don't touch $@ at all
 # if the sanity check below fails.
 	$(LINK.o) -nostdlib -nostartfiles -shared -o $@.new		\
-		  $(LDFLAGS-rtld) -Wl,-z,defs $(z-now-$(bind-now))	\
+		  $(LDFLAGS-rtld) -Wl,-z,defs				\
 		  $(filter-out $(map-file),$^) $(load-map-file)		\
 		  -Wl,-soname=$(rtld-installed-name)			\
 		  -Wl,-defsym=_begin=0
-- 
2.1.0

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[Carlos O'Donell]

On 05/23/2015 09:14 AM, H.J. Lu wrote:
> Since ld.so is built with -z now, there are no PLT relocations and this
> calloc won't be used:

Which is bad. We always want these functions to be interposable by all
of the analysis tools that want and need to track memory allocations.
Thus it is not just the test that matters.

> Relocation section '.rela.dyn' at offset 0x66c contains 16 entries:
>  Offset     Info    Type            Sym.Value  Sym. Name + Addend
> 00222e88  00000008 R_X86_64_RELATIVE            10970
> 00222e8c  00000008 R_X86_64_RELATIVE            f8f0
> 00222e90  00000008 R_X86_64_RELATIVE            f6c0
> 00222e94  00000008 R_X86_64_RELATIVE            11bd0
> 00222e98  00000008 R_X86_64_RELATIVE            a1b0
> 00222e9c  00000008 R_X86_64_RELATIVE            130c0
> 00222ea0  00000008 R_X86_64_RELATIVE            13c50
> 00222ea4  00000008 R_X86_64_RELATIVE            15fc0
> 00222ea8  00000008 R_X86_64_RELATIVE            12cd0
> 00222eac  00000008 R_X86_64_RELATIVE            17f90
> 00222fbc  00000a06 R_X86_64_GLOB_DAT 00018230	__libc_memalign@@GLIBC_2.16 + 0
> 00222fc4  00001506 R_X86_64_GLOB_DAT 00018300   malloc@@GLIBC_2.16 + 0
> 00222fcc  00000d06 R_X86_64_GLOB_DAT 00018310   calloc@@GLIBC_2.16 + 0
> 00222fd4  00000506 R_X86_64_GLOB_DAT 000184a0   realloc@@GLIBC_2.16 + 0
> 00222fdc  00000706 R_X86_64_GLOB_DAT 002239a0   _r_debug@@GLIBC_2.16 + 0
> 00222fe4  00000406 R_X86_64_GLOB_DAT 00018340   free@@GLIBC_2.16 + 0

Doesn't this also cause check-localplt to fail for ld.so given
that calloc and others are no longer R_X86_64_JUMP_SLOT?

> Assuming we do want to keep PLT relocations in ld.so so that malloc
> functions in ld.so can be overridden, ld.so should be built with -z now.
> There is no reason to build ld.so with -z now since ld.so is the one
> doing BIND_NOW.  The only thing we get with -z now on ld.so is DT tag:
> 
>  0x0000000000000018 (BIND_NOW)
>  0x000000006ffffffb (FLAGS_1)            Flags: NOW
> 
> This patch removes -Wl,-z,now from ld.so build.
> 
> OK for master?

No. I'd like to see more discussion on this.

I don't see any other way forward, and I agree that DT_BIND_NOW seems
a bit silly for the linker since it itself is the component responsible
for that binding.

My worry is that the missing DT tag is going to have security implications.
I'm including several other distro people on the TO.

The first thing I'll have to explain is "Why doesn't ld.so meet full RELRO?"
Since full RELRO requires DT_BIND_NOW + RO segments. Does this mean ld.so
with your patch will by lazily bound and not mark it's own PLT immediately RO?
That seems wrong.

Given the wrongness I'd like to see more discussion, and it's late in my TZ
so I'm not up for a detailed response yet.

However, the first thing that pops into mind is that this wrong, but wrong
in the sense that I don't know if the static linker can even make this choice
(removing the PLT) without information from the user.

The notion of -z,now means "Bind symbols now, not lazily", but that doesn't
give you enough information to elide the PLT and use the GOT directly since
the interposition is still useful and relied upon semantic in ELF.

Cheers,
Carlos.

> H.J.
> 	[BZ #18422]
> 	* elf/Makefile (z-now-yes): Removed.
> 	($(objpfx)ld.so): Remove $(z-now-$(bind-now)).
> ---
>  elf/Makefile | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 34450ea..324b4a2 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -349,13 +349,11 @@ $(objpfx)librtld.os: $(objpfx)dl-allobjs.os $(objpfx)rtld-libc.a
>  
>  generated += librtld.map librtld.mk rtld-libc.a librtld.os.map
>  
> -z-now-yes = -Wl,-z,now
> -
>  $(objpfx)ld.so: $(objpfx)librtld.os $(ld-map)
>  # Link into a temporary file so that we don't touch $@ at all
>  # if the sanity check below fails.
>  	$(LINK.o) -nostdlib -nostartfiles -shared -o $@.new		\
> -		  $(LDFLAGS-rtld) -Wl,-z,defs $(z-now-$(bind-now))	\
> +		  $(LDFLAGS-rtld) -Wl,-z,defs				\
>  		  $(filter-out $(map-file),$^) $(load-map-file)		\
>  		  -Wl,-soname=$(rtld-installed-name)			\
>  		  -Wl,-defsym=_begin=0
> 

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[H.J. Lu]

On Sat, May 23, 2015 at 8:37 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 05/23/2015 09:14 AM, H.J. Lu wrote:
>> Since ld.so is built with -z now, there are no PLT relocations and this
>> calloc won't be used:
>
> Which is bad. We always want these functions to be interposable by all
> of the analysis tools that want and need to track memory allocations.
> Thus it is not just the test that matters.
>
...
>> 00222fc4  00001506 R_X86_64_GLOB_DAT 00018300   malloc@@GLIBC_2.16 + 0
>> 00222fcc  00000d06 R_X86_64_GLOB_DAT 00018310   calloc@@GLIBC_2.16 + 0
>> 00222fd4  00000506 R_X86_64_GLOB_DAT 000184a0   realloc@@GLIBC_2.16 + 0
>> 00222fdc  00000706 R_X86_64_GLOB_DAT 002239a0   _r_debug@@GLIBC_2.16 + 0
>> 00222fe4  00000406 R_X86_64_GLOB_DAT 00018340   free@@GLIBC_2.16 + 0
>
> Doesn't this also cause check-localplt to fail for ld.so given
> that calloc and others are no longer R_X86_64_JUMP_SLOT?

Yes.

>> Assuming we do want to keep PLT relocations in ld.so so that malloc
>> functions in ld.so can be overridden, ld.so should be built with -z now.
>> There is no reason to build ld.so with -z now since ld.so is the one
>> doing BIND_NOW.  The only thing we get with -z now on ld.so is DT tag:
>>
>>  0x0000000000000018 (BIND_NOW)
>>  0x000000006ffffffb (FLAGS_1)            Flags: NOW
>>
>> This patch removes -Wl,-z,now from ld.so build.
>>
>> OK for master?
>
> No. I'd like to see more discussion on this.
>
> I don't see any other way forward, and I agree that DT_BIND_NOW seems
> a bit silly for the linker since it itself is the component responsible
> for that binding.
>
> My worry is that the missing DT tag is going to have security implications.
> I'm including several other distro people on the TO.
>
> The first thing I'll have to explain is "Why doesn't ld.so meet full RELRO?"
> Since full RELRO requires DT_BIND_NOW + RO segments. Does this mean ld.so
> with your patch will by lazily bound and not mark it's own PLT immediately RO?
> That seems wrong.

My patch just doesn't use -z now to build ld.so when --enable-bind-now is
used and ld.so always gets PLT with/without  --enable-bind-now.  It is the
same as before when building glibc with the older ld.  The only difference now
is we no long generate DT_BIND_NOW even when --enable-bind-now is used.
I don't believe DT_BIND_NOW should make a difference on ld.so.  If it is, it is
a separate bug and we should fix it.

> Given the wrongness I'd like to see more discussion, and it's late in my TZ
> so I'm not up for a detailed response yet.
>
> However, the first thing that pops into mind is that this wrong, but wrong
> in the sense that I don't know if the static linker can even make this choice
> (removing the PLT) without information from the user.

Passing -z now tells ld that we don't do lazy bind, the only thing which PLT
is used for.  ld also implemented an optimization to generate .plt.got, which
uses the GOT slot, instead of the GOTPLT slot, when we need the PLT entry
without PLT relocation.

https://groups.google.com/forum/#!topic/x86-64-abi/LDuYGdXGskY

> The notion of -z,now means "Bind symbols now, not lazily", but that doesn't
> give you enough information to elide the PLT and use the GOT directly since
> the interposition is still useful and relied upon semantic in ELF.

You still get the interposition with GOT, just not lazy bind, which needs PLT
relocation. GCC 6 even has a new option, -fno-plt, to avoid PLT:

https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00231.html

ld.so is a special case and needs PLT for the interposition unless
ld.so re-applies
GOT relocations on itself after all modules are loaded.

-- 
H.J.

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[Carlos O'Donell]

On 05/24/2015 08:45 AM, H.J. Lu wrote:
> On Sat, May 23, 2015 at 8:37 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 05/23/2015 09:14 AM, H.J. Lu wrote:
>>> Since ld.so is built with -z now, there are no PLT relocations and this
>>> calloc won't be used:
>>
>> Which is bad. We always want these functions to be interposable by all
>> of the analysis tools that want and need to track memory allocations.
>> Thus it is not just the test that matters.
>>
> ...
>>> 00222fc4  00001506 R_X86_64_GLOB_DAT 00018300   malloc@@GLIBC_2.16 + 0
>>> 00222fcc  00000d06 R_X86_64_GLOB_DAT 00018310   calloc@@GLIBC_2.16 + 0
>>> 00222fd4  00000506 R_X86_64_GLOB_DAT 000184a0   realloc@@GLIBC_2.16 + 0
>>> 00222fdc  00000706 R_X86_64_GLOB_DAT 002239a0   _r_debug@@GLIBC_2.16 + 0
>>> 00222fe4  00000406 R_X86_64_GLOB_DAT 00018340   free@@GLIBC_2.16 + 0
>>
>> Doesn't this also cause check-localplt to fail for ld.so given
>> that calloc and others are no longer R_X86_64_JUMP_SLOT?
> 
> Yes.

Good.

>>> Assuming we do want to keep PLT relocations in ld.so so that malloc
>>> functions in ld.so can be overridden, ld.so should be built with -z now.
>>> There is no reason to build ld.so with -z now since ld.so is the one
>>> doing BIND_NOW.  The only thing we get with -z now on ld.so is DT tag:
>>>
>>>  0x0000000000000018 (BIND_NOW)
>>>  0x000000006ffffffb (FLAGS_1)            Flags: NOW
>>>
>>> This patch removes -Wl,-z,now from ld.so build.
>>>
>>> OK for master?
>>
>> No. I'd like to see more discussion on this.
>>
>> I don't see any other way forward, and I agree that DT_BIND_NOW seems
>> a bit silly for the linker since it itself is the component responsible
>> for that binding.
>>
>> My worry is that the missing DT tag is going to have security implications.
>> I'm including several other distro people on the TO.
>>
>> The first thing I'll have to explain is "Why doesn't ld.so meet full RELRO?"
>> Since full RELRO requires DT_BIND_NOW + RO segments. Does this mean ld.so
>> with your patch will by lazily bound and not mark it's own PLT immediately RO?
>> That seems wrong.
> 
> My patch just doesn't use -z now to build ld.so when --enable-bind-now is
> used and ld.so always gets PLT with/without  --enable-bind-now.  It is the
> same as before when building glibc with the older ld.  The only difference now
> is we no long generate DT_BIND_NOW even when --enable-bind-now is used.
> I don't believe DT_BIND_NOW should make a difference on ld.so.  If it is, it is
> a separate bug and we should fix it.

The patch is a hack.

- ld.so should continue to have DT_BIND_NOW since we are requesting that
  any symbol binding done in ld.so be done at startup and not lazily, and
  generic security tools should be able to check all binaries including
  ld.so to see that this is enabled and requested without making special
  exceptions.

- ld.so can have no PLT if that is what happens as a result of a dynamic
  loader optimization triggered by -Wl,-z,now. That seems like a useful
  optimization which would be disabled by your suggested patch?

- ld.so should still support ELF symbol interposition, which is orthogonal
  to lazy binding. Lazy binding and ELF symbol interposition should not be
  conflated.

>> The notion of -z,now means "Bind symbols now, not lazily", but that doesn't
>> give you enough information to elide the PLT and use the GOT directly since
>> the interposition is still useful and relied upon semantic in ELF.
> 
> You still get the interposition with GOT, just not lazy bind, which needs PLT
> relocation.

Good point.

> GCC 6 even has a new option, -fno-plt, to avoid PLT:
> 
> https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00231.html

GCC has many options that must be used with care and foresight.
 
> ld.so is a special case and needs PLT for the interposition unless
> ld.so re-applies GOT relocations on itself after all modules are loaded.

And that is the *real* bug. Please fix that instead of removing -Wl,-z,now.
It should be a net-zero performance chance since either you are doing
relocations for the GOT and then the PLT, or the GOT twice?

I think it is important we continue to build ld.so with -Wl,-z,now, both
for the optimizations, and for consistency among our security tooling.

I'm happy to hear input from others.

Have you checked to see how your patch to remove the PLT impacts analysis
tooling like Asan and Valgrind?

Cheers,
Carlos.

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 4836 bytes --]

On Sun, May 24, 2015 at 7:06 PM, Carlos O'Donell <carlos@redhat.com> wrote:

> And that is the *real* bug. Please fix that instead of removing -Wl,-z,now.
> It should be a net-zero performance chance since either you are doing
> relocations for the GOT and then the PLT, or the GOT twice?

I did a closer look.  Nothing is wrong, except for that tst-audit2 expects
certain order in ld.so.  With JUMP_SLOT relocation, the GOTPLT entry of
calloc is update to calloc defined in tst-audit2:

(gdb) bt
#0  0xf7fe56bd in elf_machine_rel (reloc=<optimized out>,
    skip_ifunc=<optimized out>, reloc_addr_arg=<optimized out>,
    version=<optimized out>, sym=<optimized out>, map=<optimized out>)
    at ../sysdeps/i386/dl-machine.h:329
#1  elf_dynamic_do_Rel (skip_ifunc=<optimized out>, lazy=<optimized out>,
    nrelative=<optimized out>, relsize=<optimized out>,
    reladdr=<optimized out>, map=<optimized out>) at do-rel.h:137
#2  _dl_relocate_object (scope=0xf7ffdab8, reloc_mode=reloc_mode@entry=0,
    consider_profiling=1, consider_profiling@entry=0) at dl-reloc.c:258
#3  0xf7fdc648 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
    user_entry=0xffffcf1c, auxv=0xffffd0a8) at rtld.c:2133
#4  0xf7ff0de7 in _dl_sysdep_start (
    start_argptr=start_argptr@entry=0xffffcfb0,
    dl_main=dl_main@entry=0xf7fda6f0 <dl_main>) at ../elf/dl-sysdep.c:249
#5  0xf7fddd05 in _dl_start_final (arg=0xffffcfb0) at rtld.c:308
#6  _dl_start (arg=0xffffcfb0) at rtld.c:414
#7  0xf7fd9a87 in _start ()
   from /export/build/gnu/glibc-32bit/build-i686-linux/elf/ld.so
(gdb)

and then calloc is called:

(gdb) c
Continuing.

Breakpoint 4, calloc (n=n@entry=20, m=4) at tst-audit2.c:18
18 {
(gdb) bt
#0  calloc (n=n@entry=20, m=4) at tst-audit2.c:18
#1  0xf7fe668d in _dl_relocate_object (scope=0xf7ffdab8,
    reloc_mode=reloc_mode@entry=0, consider_profiling=1,
    consider_profiling@entry=0) at dl-reloc.c:272
#2  0xf7fdc648 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
    user_entry=0xffffcf1c, auxv=0xffffd0a8) at rtld.c:2133
#3  0xf7ff0de7 in _dl_sysdep_start (
    start_argptr=start_argptr@entry=0xffffcfb0,
    dl_main=dl_main@entry=0xf7fda6f0 <dl_main>) at ../elf/dl-sysdep.c:249
#4  0xf7fddd05 in _dl_start_final (arg=0xffffcfb0) at rtld.c:308
#5  _dl_start (arg=0xffffcfb0) at rtld.c:414
#6  0xf7fd9a87 in _start ()
   from /export/build/gnu/glibc-32bit/build-i686-linux/elf/ld.so
(gdb)

With GLOB_DAT relocation, calloc in ld.so is called first:

(gdb) bt
#0  calloc (nmemb=20, size=4) at dl-minimal.c:102
#1  0xf7fe665d in _dl_relocate_object (scope=0xf7fcfb20, reloc_mode=1,
    consider_profiling=1) at dl-reloc.c:272
#2  0xf7fdc500 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
    user_entry=0xffffcf0c, auxv=0xffffd098) at rtld.c:2074
#3  0xf7ff0db7 in _dl_sysdep_start (
    start_argptr=start_argptr@entry=0xffffcfa0,
    dl_main=dl_main@entry=0xf7fda6c0 <dl_main>) at ../elf/dl-sysdep.c:249
#4  0xf7fddcd5 in _dl_start_final (arg=0xffffcfa0) at rtld.c:308
#5  _dl_start (arg=0xffffcfa0) at rtld.c:414
#6  0xf7fd9a57 in _start ()
   from /export/build/gnu/glibc-32bit-test/build-i686-linux/elf/ld.so
(gdb)

and then the GOT entry of calloc is updated:

(gdb) bt
#0  0xf7fe568d in elf_machine_rel (reloc=<optimized out>,
    skip_ifunc=<optimized out>, reloc_addr_arg=<optimized out>,
    version=<optimized out>, sym=<optimized out>, map=<optimized out>)
    at ../sysdeps/i386/dl-machine.h:329
#1  elf_dynamic_do_Rel (skip_ifunc=<optimized out>, lazy=<optimized out>,
    nrelative=<optimized out>, relsize=<optimized out>,
    reladdr=<optimized out>, map=<optimized out>) at do-rel.h:137
#2  _dl_relocate_object (scope=0xf7ffdab8, reloc_mode=reloc_mode@entry=0,
    consider_profiling=1, consider_profiling@entry=0) at dl-reloc.c:258
#3  0xf7fdc618 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
    user_entry=0xffffcf0c, auxv=0xffffd098) at rtld.c:2133
#4  0xf7ff0db7 in _dl_sysdep_start (
    start_argptr=start_argptr@entry=0xffffcfa0,
    dl_main=dl_main@entry=0xf7fda6c0 <dl_main>) at ../elf/dl-sysdep.c:249
#5  0xf7fddcd5 in _dl_start_final (arg=0xffffcfa0) at rtld.c:308
#6  _dl_start (arg=0xffffcfa0) at rtld.c:414
#7  0xf7fd9a57 in _start ()
   from /export/build/gnu/glibc-32bit-test/build-i686-linux/elf/ld.so
(gdb)

After, calloc isn't called and magic in tst-audit2 isn't updated.  Both
orders are correct.  Here is a patch to make sure that calloc in tst-audit2.c is
called at least once from ld.so.

> I think it is important we continue to build ld.so with -Wl,-z,now, both
> for the optimizations, and for consistency among our security tooling.
>
> I'm happy to hear input from others.
>
> Have you checked to see how your patch to remove the PLT impacts analysis
> tooling like Asan and Valgrind?
>

No, i didn't test them nor I expect any problems.



-- 
H.J.

[-- Attachment #2: 0001-Make-sure-that-calloc-is-called-at-least-once.patch --]
[-- Type: text/x-patch, Size: 6354 bytes --]

From 31d9cfee87edf3acf95640db262b950e3b0514f3 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 25 May 2015 11:30:57 -0700
Subject: [PATCH] Make sure that calloc is called at least once
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PLT relocations aren't required when -z now used.  Linker on master with:

commit 25070364b0ce33eed46aa5d78ebebbec6accec7e
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Sat May 16 07:00:21 2015 -0700

    Don't generate PLT relocations for now binding

    There is no need for PLT relocations with -z now. We can use GOT
    relocations, which take less space, instead and replace 16-byte .plt
    entres with 8-byte .plt.got entries.

    bfd/

      * elf32-i386.c (elf_i386_check_relocs): Create .plt.got section
      for now binding.
      (elf_i386_allocate_dynrelocs): Use .plt.got section for now
      binding.
      * elf64-x86-64.c (elf_x86_64_check_relocs): Create .plt.got
      section for now binding.
      (elf_x86_64_allocate_dynrelocs): Use .plt.got section for now
      binding.

won't generate PLT relocations with -z now.  elf/tst-audit2.c expect
certain order of execution in ld.so.  With PLT relocations, the GOTPLT
entry of calloc is update to calloc defined in tst-audit2:

(gdb) bt
    skip_ifunc=<optimized out>, reloc_addr_arg=<optimized out>,
    version=<optimized out>, sym=<optimized out>, map=<optimized out>)
    at ../sysdeps/i386/dl-machine.h:329
out>,
    nrelative=<optimized out>, relsize=<optimized out>,
    reladdr=<optimized out>, map=<optimized out>) at do-rel.h:137
reloc_mode=reloc_mode@entry=0,
    consider_profiling=1, consider_profiling@entry=0) at dl-reloc.c:258
    user_entry=0xffffcf1c, auxv=0xffffd0a8) at rtld.c:2133
    start_argptr=start_argptr@entry=0xffffcfb0,
    dl_main=dl_main@entry=0xf7fda6f0 <dl_main>) at
../elf/dl-sysdep.c:249
   from /export/build/gnu/glibc-32bit/build-i686-linux/elf/ld.so
(gdb)

and then calloc is called:

(gdb) c
Continuing.

Breakpoint 4, calloc (n=n@entry=20, m=4) at tst-audit2.c:18
18 {
(gdb) bt
    reloc_mode=reloc_mode@entry=0, consider_profiling=1,
    consider_profiling@entry=0) at dl-reloc.c:272
    user_entry=0xffffcf1c, auxv=0xffffd0a8) at rtld.c:2133
    start_argptr=start_argptr@entry=0xffffcfb0,
    dl_main=dl_main@entry=0xf7fda6f0 <dl_main>) at
../elf/dl-sysdep.c:249
   from /export/build/gnu/glibc-32bit/build-i686-linux/elf/ld.so
(gdb)

With GOT relocation, calloc in ld.so is called first:

(gdb) bt
    consider_profiling=1) at dl-reloc.c:272
    user_entry=0xffffcf0c, auxv=0xffffd098) at rtld.c:2074
    start_argptr=start_argptr@entry=0xffffcfa0,
    dl_main=dl_main@entry=0xf7fda6c0 <dl_main>) at
../elf/dl-sysdep.c:249
   from /export/build/gnu/glibc-32bit-test/build-i686-linux/elf/ld.so
(gdb)

and then the GOT entry of calloc is updated:

(gdb) bt
    skip_ifunc=<optimized out>, reloc_addr_arg=<optimized out>,
    version=<optimized out>, sym=<optimized out>, map=<optimized out>)
    at ../sysdeps/i386/dl-machine.h:329
out>,
    nrelative=<optimized out>, relsize=<optimized out>,
    reladdr=<optimized out>, map=<optimized out>) at do-rel.h:137
reloc_mode=reloc_mode@entry=0,
    consider_profiling=1, consider_profiling@entry=0) at dl-reloc.c:258
    user_entry=0xffffcf0c, auxv=0xffffd098) at rtld.c:2133
    start_argptr=start_argptr@entry=0xffffcfa0,
    dl_main=dl_main@entry=0xf7fda6c0 <dl_main>) at
../elf/dl-sysdep.c:249
   from /export/build/gnu/glibc-32bit-test/build-i686-linux/elf/ld.so
(gdb)

After that, since calloc isn't called from ld.so nor any other modules,
magic in tst-audit2 isn't updated.  Both orders are correct.  This patch
makes sure that calloc in tst-audit2.c is called at least once from ld.so.

	[BZ #18422]
	* Makefile ($(objpfx)tst-audit2): Depend on $(libdl).
	($(objpfx)tst-audit2.out): Also depend on
	$(objpfx)tst-auditmod9b.so.
	* elf/tst-audit2.c: Include <dlfcn.h>.
	(calloc_called): New.
	(calloc): Allow to be called more than once.
	(do_test): dllopen/dlclose $ORIGIN/tst-auditmod9b.so.
---
 elf/Makefile     |  3 ++-
 elf/tst-audit2.c | 23 ++++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/elf/Makefile b/elf/Makefile
index b06e0a7..dedf3c7 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1034,7 +1034,8 @@ $(objpfx)tst-dlmopen3.out: $(objpfx)tst-dlmopen1mod.so
 $(objpfx)tst-audit1.out: $(objpfx)tst-auditmod1.so
 tst-audit1-ENV = LD_AUDIT=$(objpfx)tst-auditmod1.so
 
-$(objpfx)tst-audit2.out: $(objpfx)tst-auditmod1.so
+$(objpfx)tst-audit2: $(libdl)
+$(objpfx)tst-audit2.out: $(objpfx)tst-auditmod1.so $(objpfx)tst-auditmod9b.so
 # Prevent GCC-5 from translating a malloc/memset pair into calloc
 CFLAGS-tst-audit2.c += -fno-builtin
 tst-audit2-ENV = LD_AUDIT=$(objpfx)tst-auditmod1.so
diff --git a/elf/tst-audit2.c b/elf/tst-audit2.c
index acad1b0..44c74d4 100644
--- a/elf/tst-audit2.c
+++ b/elf/tst-audit2.c
@@ -3,10 +3,12 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <dlfcn.h>
 
 #define MAGIC1 0xabcdef72
 #define MAGIC2 0xd8675309
 static __thread unsigned int magic[] = { MAGIC1, MAGIC2 };
+static __thread int calloc_called;
 
 #undef calloc
 
@@ -16,13 +18,19 @@ static __thread unsigned int magic[] = { MAGIC1, MAGIC2 };
 void *
 calloc (size_t n, size_t m)
 {
-  if (magic[0] != MAGIC1 || magic[1] != MAGIC2)
+  if (!calloc_called)
     {
-      printf ("{%x, %x} != {%x, %x}\n", magic[0], magic[1], MAGIC1, MAGIC2);
-      abort ();
+      /* Allow our calloc to be called more than once.  */
+      calloc_called = 1;
+      if (magic[0] != MAGIC1 || magic[1] != MAGIC2)
+	{
+	  printf ("{%x, %x} != {%x, %x}\n",
+		  magic[0], magic[1], MAGIC1, MAGIC2);
+	  abort ();
+	}
+      magic[0] = MAGIC2;
+      magic[1] = MAGIC1;
     }
-  magic[0] = MAGIC2;
-  magic[1] = MAGIC1;
 
   n *= m;
   void *ptr = malloc (n);
@@ -34,6 +42,11 @@ calloc (size_t n, size_t m)
 static int
 do_test (void)
 {
+  /* Make sure that our calloc is called from the dynamic linker at least
+     once.  */
+  void *h = dlopen("$ORIGIN/tst-auditmod9b.so", RTLD_LAZY);
+  if (h != NULL)
+    dlclose (h);
   if (magic[1] != MAGIC1 || magic[0] != MAGIC2)
     {
       printf ("{%x, %x} != {%x, %x}\n", magic[0], magic[1], MAGIC2, MAGIC1);
-- 
2.1.0

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[Carlos O'Donell]

On 05/25/2015 02:54 PM, H.J. Lu wrote:
> On Sun, May 24, 2015 at 7:06 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> > And that is the *real* bug. Please fix that instead of removing -Wl,-z,now.
>> > It should be a net-zero performance chance since either you are doing
>> > relocations for the GOT and then the PLT, or the GOT twice?
> I did a closer look.  Nothing is wrong, except for that tst-audit2 expects
> certain order in ld.so.  With JUMP_SLOT relocation, the GOTPLT entry of
> calloc is update to calloc defined in tst-audit2:

Thank you very much for looking into this.

I'm glad to see that it does work, but that it is an ordering issue
between JUMP_SLOT and GOTPLT processing.

I have one question, please see below.

> 
> (gdb) bt
> #0  0xf7fe56bd in elf_machine_rel (reloc=<optimized out>,
>     skip_ifunc=<optimized out>, reloc_addr_arg=<optimized out>,
>     version=<optimized out>, sym=<optimized out>, map=<optimized out>)
>     at ../sysdeps/i386/dl-machine.h:329
> #1  elf_dynamic_do_Rel (skip_ifunc=<optimized out>, lazy=<optimized out>,
>     nrelative=<optimized out>, relsize=<optimized out>,
>     reladdr=<optimized out>, map=<optimized out>) at do-rel.h:137
> #2  _dl_relocate_object (scope=0xf7ffdab8, reloc_mode=reloc_mode@entry=0,
>     consider_profiling=1, consider_profiling@entry=0) at dl-reloc.c:258
> #3  0xf7fdc648 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
>     user_entry=0xffffcf1c, auxv=0xffffd0a8) at rtld.c:2133
> #4  0xf7ff0de7 in _dl_sysdep_start (
>     start_argptr=start_argptr@entry=0xffffcfb0,
>     dl_main=dl_main@entry=0xf7fda6f0 <dl_main>) at ../elf/dl-sysdep.c:249
> #5  0xf7fddd05 in _dl_start_final (arg=0xffffcfb0) at rtld.c:308
> #6  _dl_start (arg=0xffffcfb0) at rtld.c:414
> #7  0xf7fd9a87 in _start ()
>    from /export/build/gnu/glibc-32bit/build-i686-linux/elf/ld.so
> (gdb)
> 
> and then calloc is called:
> 
> (gdb) c
> Continuing.
> 
> Breakpoint 4, calloc (n=n@entry=20, m=4) at tst-audit2.c:18
> 18 {
> (gdb) bt
> #0  calloc (n=n@entry=20, m=4) at tst-audit2.c:18
> #1  0xf7fe668d in _dl_relocate_object (scope=0xf7ffdab8,
>     reloc_mode=reloc_mode@entry=0, consider_profiling=1,
>     consider_profiling@entry=0) at dl-reloc.c:272
> #2  0xf7fdc648 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
>     user_entry=0xffffcf1c, auxv=0xffffd0a8) at rtld.c:2133
> #3  0xf7ff0de7 in _dl_sysdep_start (
>     start_argptr=start_argptr@entry=0xffffcfb0,
>     dl_main=dl_main@entry=0xf7fda6f0 <dl_main>) at ../elf/dl-sysdep.c:249
> #4  0xf7fddd05 in _dl_start_final (arg=0xffffcfb0) at rtld.c:308
> #5  _dl_start (arg=0xffffcfb0) at rtld.c:414
> #6  0xf7fd9a87 in _start ()
>    from /export/build/gnu/glibc-32bit/build-i686-linux/elf/ld.so
> (gdb)
> 
> With GLOB_DAT relocation, calloc in ld.so is called first:

This results in one more calloc from dl-minimal.c which has to be tracked
and not freed.

> (gdb) bt
> #0  calloc (nmemb=20, size=4) at dl-minimal.c:102
> #1  0xf7fe665d in _dl_relocate_object (scope=0xf7fcfb20, reloc_mode=1,
>     consider_profiling=1) at dl-reloc.c:272
> #2  0xf7fdc500 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
>     user_entry=0xffffcf0c, auxv=0xffffd098) at rtld.c:2074
> #3  0xf7ff0db7 in _dl_sysdep_start (
>     start_argptr=start_argptr@entry=0xffffcfa0,
>     dl_main=dl_main@entry=0xf7fda6c0 <dl_main>) at ../elf/dl-sysdep.c:249
> #4  0xf7fddcd5 in _dl_start_final (arg=0xffffcfa0) at rtld.c:308
> #5  _dl_start (arg=0xffffcfa0) at rtld.c:414
> #6  0xf7fd9a57 in _start ()
>    from /export/build/gnu/glibc-32bit-test/build-i686-linux/elf/ld.so
> (gdb)
> 
> and then the GOT entry of calloc is updated:

OK.

> (gdb) bt
> #0  0xf7fe568d in elf_machine_rel (reloc=<optimized out>,
>     skip_ifunc=<optimized out>, reloc_addr_arg=<optimized out>,
>     version=<optimized out>, sym=<optimized out>, map=<optimized out>)
>     at ../sysdeps/i386/dl-machine.h:329
> #1  elf_dynamic_do_Rel (skip_ifunc=<optimized out>, lazy=<optimized out>,
>     nrelative=<optimized out>, relsize=<optimized out>,
>     reladdr=<optimized out>, map=<optimized out>) at do-rel.h:137
> #2  _dl_relocate_object (scope=0xf7ffdab8, reloc_mode=reloc_mode@entry=0,
>     consider_profiling=1, consider_profiling@entry=0) at dl-reloc.c:258
> #3  0xf7fdc618 in dl_main (phdr=<optimized out>, phnum=<optimized out>,
>     user_entry=0xffffcf0c, auxv=0xffffd098) at rtld.c:2133
> #4  0xf7ff0db7 in _dl_sysdep_start (
>     start_argptr=start_argptr@entry=0xffffcfa0,
>     dl_main=dl_main@entry=0xf7fda6c0 <dl_main>) at ../elf/dl-sysdep.c:249
> #5  0xf7fddcd5 in _dl_start_final (arg=0xffffcfa0) at rtld.c:308
> #6  _dl_start (arg=0xffffcfa0) at rtld.c:414
> #7  0xf7fd9a57 in _start ()
>    from /export/build/gnu/glibc-32bit-test/build-i686-linux/elf/ld.so
> (gdb)
> 
> After, calloc isn't called and magic in tst-audit2 isn't updated.  Both
> orders are correct.  Here is a patch to make sure that calloc in tst-audit2.c is
> called at least once from ld.so.

In the past before your ld optimization the JUMP_SLOT would have been
relocated to libc.so.6's version of calloc, or the test version of calloc,
correct?

The change from JUMP_SLOT -> GLOB_DAT for calloc means that we call calloc
one more time using the dl-minimal.c implementation. This seems dangerous to
me since in ld.so we take extreme caution not to attempt to free this result
because dl-minimal.c doesn't support freeing anything but the most recent
allocation. We also don't want to call free() from libc.so.6 with data that
was calloc'd from the dl-minimal.c implementation.

Are we certain that this additional calloc, now using dl-minimal.c, isn't
going to cause problems? When is it freed? What implementation of free is used?
 
>> > I think it is important we continue to build ld.so with -Wl,-z,now, both
>> > for the optimizations, and for consistency among our security tooling.
>> >
>> > I'm happy to hear input from others.
>> >
>> > Have you checked to see how your patch to remove the PLT impacts analysis
>> > tooling like Asan and Valgrind?
>> >
> No, i didn't test them nor I expect any problems.

Well, right away if we have one more call of calloc to dl-minimal.c, that's
another allocation the tool can't track. It's not huegely problematic because
they already can't track all the early allocations via dl-minimal.c.

In summary:
- My next worry is about free of calloc'd data that is now using dl-minimal.c

If we can answer that question, then I think this patch to adjust tst-audit2.c
is the best solution.

Cheers,
Carlos.

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[Andreas Schwab]

"Carlos O'Donell" <carlos@redhat.com> writes:

> In summary:
> - My next worry is about free of calloc'd data that is now using dl-minimal.c

Anything allocated with dl-minimal must strictly be kept inside ld.so
and never be freed.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[H.J. Lu]

On Tue, May 26, 2015 at 1:00 AM, Andreas Schwab <schwab@suse.de> wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>
>> In summary:
>> - My next worry is about free of calloc'd data that is now using dl-minimal.c
>
> Anything allocated with dl-minimal must strictly be kept inside ld.so
> and never be freed.

The calloc call is made at:

   if (__glibc_unlikely (consider_profiling)
        && l->l_info[DT_PLTRELSZ] != NULL)
      {
        /* Allocate the array which will contain the already found
           relocations.  If the shared object lacks a PLT (for example
           if it only contains lead function) the l_info[DT_PLTRELSZ]
           will be NULL.  */
        size_t sizeofrel = l->l_info[DT_PLTREL]->d_un.d_val == DT_RELA
                           ? sizeof (ElfW(Rela))
                           : sizeof (ElfW(Rel));
        size_t relcount = l->l_info[DT_PLTRELSZ]->d_un.d_val / sizeofrel;
        l->l_reloc_result = calloc (sizeof (l->l_reloc_result[0]), relcount);

        if (l->l_reloc_result == NULL)
          {
            errstring = N_("\
%s: out of memory to store relocation results for %s\n");
            _dl_fatal_printf (errstring, RTLD_PROGNAME, l->l_name);
          }
      }

ld.so never frees  l->l_reloc_result.

-- 
H.J.

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[Florian Weimer]

On 05/24/2015 05:37 AM, Carlos O'Donell wrote:

> My worry is that the missing DT tag is going to have security implications.
> I'm including several other distro people on the TO.

I tried to raise awareness of this change here:

  <http://www.openwall.com/lists/oss-security/2015/05/26/2>

Personally, I cannot offer much guidance on the potential impact of this
proposal at present, I'm afraid.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[Carlos O'Donell]

On 05/26/2015 04:00 AM, Andreas Schwab wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
> 
>> In summary:
>> - My next worry is about free of calloc'd data that is now using dl-minimal.c
> 
> Anything allocated with dl-minimal must strictly be kept inside ld.so
> and never be freed.

I agree.

However, now that we are delaying the interposition until we process the
GOT relocs, the tst-audit2 test fails because what was once a call to libc.so's
calloc is now a call to dl-minimal. I haven't debugged this so I don't know if
we're tracking that calloc correctly such that we don't attempt to free it.

Did I get something wrong?

Cheers,
Carlos.

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[H.J. Lu]

On Tue, May 26, 2015 at 4:54 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 05/26/2015 04:00 AM, Andreas Schwab wrote:
>> "Carlos O'Donell" <carlos@redhat.com> writes:
>>
>>> In summary:
>>> - My next worry is about free of calloc'd data that is now using dl-minimal.c
>>
>> Anything allocated with dl-minimal must strictly be kept inside ld.so
>> and never be freed.
>
> I agree.
>
> However, now that we are delaying the interposition until we process the
> GOT relocs, the tst-audit2 test fails because what was once a call to libc.so's
> calloc is now a call to dl-minimal. I haven't debugged this so I don't know if
> we're tracking that calloc correctly such that we don't attempt to free it.

See:

https://sourceware.org/ml/libc-alpha/2015-05/msg00632.html

-- 
H.J.

Re: [PATCH] [BZ #18422] elf/tst-audit tests fail without PLT entries

[Carlos O'Donell]

On 05/26/2015 07:19 AM, H.J. Lu wrote:
> On Tue, May 26, 2015 at 1:00 AM, Andreas Schwab <schwab@suse.de> wrote:
>> "Carlos O'Donell" <carlos@redhat.com> writes:
>>
>>> In summary:
>>> - My next worry is about free of calloc'd data that is now using dl-minimal.c
>>
>> Anything allocated with dl-minimal must strictly be kept inside ld.so
>> and never be freed.
> 
> The calloc call is made at:
> 
>    if (__glibc_unlikely (consider_profiling)
>         && l->l_info[DT_PLTRELSZ] != NULL)
>       {
>         /* Allocate the array which will contain the already found
>            relocations.  If the shared object lacks a PLT (for example
>            if it only contains lead function) the l_info[DT_PLTRELSZ]
>            will be NULL.  */
>         size_t sizeofrel = l->l_info[DT_PLTREL]->d_un.d_val == DT_RELA
>                            ? sizeof (ElfW(Rela))
>                            : sizeof (ElfW(Rel));
>         size_t relcount = l->l_info[DT_PLTRELSZ]->d_un.d_val / sizeofrel;
>         l->l_reloc_result = calloc (sizeof (l->l_reloc_result[0]), relcount);
> 
>         if (l->l_reloc_result == NULL)
>           {
>             errstring = N_("\
> %s: out of memory to store relocation results for %s\n");
>             _dl_fatal_printf (errstring, RTLD_PROGNAME, l->l_name);
>           }
>       }
> 
> ld.so never frees  l->l_reloc_result.

Thanks.

The only other place I was worried about was TLS data structures, but
there we already use dl_initial_tls to indicate the data structure was
allocated early (specifically for use by auditors) and we do not pass
it to realloc because it was allocated by dl-minimal, thus we are OK
there also.

After your changes in binutils is the test at all useful?

We are no longer able to interpose calloc to catch early TLS init,
therefore we are no longer testing early TLS init and the comments
in the test need to be changed to match.

The new test is:
"Test that calloc is called at least once after dlopen and initialization
 of TLS varibles in the DSO."

Why do we care about this?

Is there any way to still test that early TLS initialization has occurred
when using LD_AUDIT?

Would't such a test look like this?

- Create auditor that uses TLS in audit funciton.
- Interpose calloc.
- Check that things don't crash.

We already have a test for this, it's tst-audit9 (Bug 16613).

OK to checkin your change to tst-audit2 if you change the test comment
to reflect the change in what is being tested:

"Test that interposed calloc is called by the dynamic loader, and that
 TLS is fully initialized by then."

Thanks for working through this.

Cheers,
Carlos.