Skip PT_DYNAMIC segment if its p_filesz == 0 [BZ #22101]

  On 9/25/17, Carlos O'Donell <carlos@redhat.com> wrote:
> On 09/25/2017 06:33 PM, H.J. Lu wrote:
>> ELF object generated with "objcopy --only-keep-debug" has
>>
>> Type     Offset  VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>> DYNAMIC  0x0+e28 0x0+200e40 0x0+200e40 0x0+    0x0+1a0 RW  0x8
>>
>> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>>
>> Tested on x86-64.  OK for master?
>
> Are all such `objcopy --only-kee--debug` objects left with 0 file size?

Yes, that is correct.

> After your patch what happens when you run ldd on such an object?

Before:

 [hjl@gnu-efi-2 elf]$ /lib64/ld-2.25.so --list ./tst-debug1mod1.so
	statically linked

After:

[hjl@gnu-efi-2 elf]$ ./ld.so --list ./tst-debug1mod1.so
./tst-debug1mod1.so: error while loading shared libraries:
./tst-debug1mod1.so: object file has no dynamic section
[hjl@gnu-efi-2 elf]$

> The idea in bug 22101 is to add minimal code early in the dynamic
> loader to identify specially marked objects and ignore them. This
> way we put an end to the guessing game of what constitutes a valid
> ELF object.

I am not sure if this is necessary.  Any such changes won't work with
debug only objects generated by the current objcopy.

> Granted, the code you've added is quite small, so it looks like
> an interesting short term solution. It needs a more verbose
> comment for the PT_DYNAMIC case explaining why we check if
> ph->p_filesz is zero and what the consequences of that are since
> we *never* add such defensive checks in ld.so because they would

This is not entirely true.   There are plenty of sanity checks in ld.so.
For example, a few lines below, there are

       case PT_LOAD:
          /* A load command tells us to map in part of the file.
             We record the load commands and process them all later.  */
          if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
            {
              errstring = N_("ELF load command alignment not page-aligned");
              goto call_lose;
            }
          if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
                                 & (ph->p_align - 1)) != 0))
            {
              errstring
                = N_("ELF load command address/offset not properly aligned");
              goto call_lose;
            }

> slow down the average case of a correctly formed binary (and
> thus need a hefty comment).
>

Here is the updated patch with comments:

        case PT_DYNAMIC:
          if (ph->p_filesz)
            {
              /* Debuginfo only file from "objcopy --only-keep-debug"
                 contains PT_DYNAMIC segment with p_filesz == 0.  Skip
                 such segment to avoid crash later.  */
              l->l_ld = (void *) ph->p_vaddr;
              l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn));
            }
          break;
  

On 09/26/2017 01:55 AM, H.J. Lu wrote:
> On 9/25/17, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 09/25/2017 06:33 PM, H.J. Lu wrote:
>>> ELF object generated with "objcopy --only-keep-debug" has
>>>
>>> Type     Offset  VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>>> DYNAMIC  0x0+e28 0x0+200e40 0x0+200e40 0x0+    0x0+1a0 RW  0x8
>>>
>>> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>>>
>>> Tested on x86-64.  OK for master?
>> Are all such `objcopy --only-kee--debug` objects left with 0 file size?
> Yes, that is correct.
> 
>> After your patch what happens when you run ldd on such an object?
> Before:
> 
>  [hjl@gnu-efi-2 elf]$ /lib64/ld-2.25.so --list ./tst-debug1mod1.so
> 	statically linked

Right, or it will crash.

> After:
> 
> [hjl@gnu-efi-2 elf]$ ./ld.so --list ./tst-debug1mod1.so
> ./tst-debug1mod1.so: error while loading shared libraries:
> ./tst-debug1mod1.so: object file has no dynamic section

That is a good error. Thanks for checking this.

> [hjl@gnu-efi-2 elf]$
> 
>> The idea in bug 22101 is to add minimal code early in the dynamic
>> loader to identify specially marked objects and ignore them. This
>> way we put an end to the guessing game of what constitutes a valid
>> ELF object.
> I am not sure if this is necessary.  Any such changes won't work with
> debug only objects generated by the current objcopy.

I agree that such changes won't work with the current objcopy.

However, it is still something that will give value as we move forward
to avoid needing heuristic checks for modifications made by the
stripping that creates invalid ELF object files.

For example the above error could be changed to explicitly say:

"error the file is a stripped debug file and cannot be loaded"

Which is better information and costs about as much, and allows us
to potentially exit earlier.

>> Granted, the code you've added is quite small, so it looks like
>> an interesting short term solution. It needs a more verbose
>> comment for the PT_DYNAMIC case explaining why we check if
>> ph->p_filesz is zero and what the consequences of that are since
>> we *never* add such defensive checks in ld.so because they would
> This is not entirely true.   There are plenty of sanity checks in ld.so.
> For example, a few lines below, there are

There are *some* checks, weighed against the cost of doing the checks.

Other than these we don't do much validation and expect all of the
values to be valid.

I certainly don't want to add much more.
 
>        case PT_LOAD:
>           /* A load command tells us to map in part of the file.
>              We record the load commands and process them all later.  */
>           if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
>             {
>               errstring = N_("ELF load command alignment not page-aligned");
>               goto call_lose;
>             }
>           if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
>                                  & (ph->p_align - 1)) != 0))
>             {
>               errstring
>                 = N_("ELF load command address/offset not properly aligned");
>               goto call_lose;
>             }
> 
>> slow down the average case of a correctly formed binary (and
>> thus need a hefty comment).
>>
> Here is the updated patch with comments:
> 
>         case PT_DYNAMIC:
>           if (ph->p_filesz)
>             {
>               /* Debuginfo only file from "objcopy --only-keep-debug"
>                  contains PT_DYNAMIC segment with p_filesz == 0.  Skip
>                  such segment to avoid crash later.  */
>               l->l_ld = (void *) ph->p_vaddr;
>               l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn));
>             }
>           break;

Your patch is small, is a very specific check with a low cost, and as I
mentioned before I think is a solution that can go in right now to master.

The patch below looks good to me.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks for adding the commit message to your patch for review.

> From 5b9b19d9ee5040cad920e8e31e4c2100fcf8d25f Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Mon, 25 Sep 2017 17:16:52 -0700
> Subject: [PATCH] Skip PT_DYNAMIC segment with p_filesz == 0 [BZ #22101]
> 
> ELF object generated with "objcopy --only-keep-debug" has

s/object/objects/g s/has/have/g

> 
> Type     Offset  VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> DYNAMIC  0x0+e28 0x0+200e40 0x0+200e40 0x0+    0x0+1a0 RW  0x8
> 
> with 0 file size. ld.so should skip such PT_DYNAMIC segments.

Suggest adding:

Without a PT_DYNAMIC segment the loading of the shared object will
fail, and therefore ldd on such objects will also fail instead of
crashing. This provides better diagnostics for tooling that is
attempting to inspect the invalid shared objects which may just
contain debug information.

> 
> 	[BZ #22101]
> 	* elf/Makefile (tests): Add tst-debug1.
> 	($(objpfx)tst-debug1): New.
> 	($(objpfx)tst-debug1.out): Likewise.
> 	($(objpfx)tst-debug1mod1.so): Likewise.
> 	* elf/dl-load.c (_dl_map_object_from_fd): Skip PT_DYNAMIC segment
> 	with p_filesz == 0.
> 	* elf/tst-debug1.c: New file.
> ---
>  elf/Makefile     |  9 ++++++++-
>  elf/dl-load.c    | 10 ++++++++--
>  elf/tst-debug1.c | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 50 insertions(+), 3 deletions(-)
>  create mode 100644 elf/tst-debug1.c
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 7cf959aabd..e21f37e30b 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -181,7 +181,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
>  	 tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
>  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
> -	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose
> +	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
> +	 tst-debug1

OK.

>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \
> @@ -1417,3 +1418,9 @@ tst-env-setuid-ENV = MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 \
>  		     LD_HWCAP_MASK=0x1
>  tst-env-setuid-tunables-ENV = \
>  	GLIBC_TUNABLES=glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096
> +
> +$(objpfx)tst-debug1: $(libdl)
> +$(objpfx)tst-debug1.out: $(objpfx)tst-debug1mod1.so
> +
> +$(objpfx)tst-debug1mod1.so: $(objpfx)testobj1.so
> +	$(OBJCOPY) --only-keep-debug $< $@

OK.

> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a067760cc6..d43bb2dd5e 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1052,8 +1052,14 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	     segments are mapped in.  We record the addresses it says
>  	     verbatim, and later correct for the run-time load address.  */
>  	case PT_DYNAMIC:
> -	  l->l_ld = (void *) ph->p_vaddr;
> -	  l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn));
> +	  if (ph->p_filesz)
> +	    {
> +	      /* Debuginfo only file from "objcopy --only-keep-debug"

s/file/files/g

> +		 contains PT_DYNAMIC segment with p_filesz == 0.  Skip

s/contains/contain a/g

> +		 such segment to avoid crash later.  */

s/such/such a/g
s/avoid/avoid a/g

> +	      l->l_ld = (void *) ph->p_vaddr;
> +	      l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn));

OK.

> +	    }
>  	  break;
>  
>  	case PT_PHDR:
> diff --git a/elf/tst-debug1.c b/elf/tst-debug1.c
> new file mode 100644
> index 0000000000..aa2f4886bf
> --- /dev/null
> +++ b/elf/tst-debug1.c
> @@ -0,0 +1,34 @@
> +/* Unit test for dlopen on ELF object from "objcopy --only-keep-debug".

OK.

> +   Copyright (C) 2017 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 <dlfcn.h>
> +#include <stdio.h>
> +
> +static int
> +do_test (void)
> +{
> +  void *h = dlopen ("tst-debug1mod1.so", RTLD_LAZY);
> +  if (h != NULL)
> +    {
> +      puts ("shouldn't load tst-debug1mod1.so");
> +      return 1;
> +    }
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> -- 2.13.5

Thanks for the new test.