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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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;


-- 
H.J.

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

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.

	[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
 #	 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 $< $@
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"
+		 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;
 
 	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".
+   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