Re: [PATCH] libelf: Allow updating phdrs for any e_type.

Re: [PATCH] libelf: Allow updating phdrs for any e_type.

[Mark Wielaard]

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

On Wed, 2016-07-06 at 15:46 +0200, Mark Wielaard wrote:
> elf[32|64]_updatenull would sanity check the e_type before allowing to
> update the phdrs. This prevents creating an ET_REL file with phdrs. It
> also prevents creating any vendor specific ELF file having phdrs. We
> only check this when updating/writing out the file. But we would just
> in such file. Don't prevent people from creating unexpected ELF files.
> elflint will warn for such files.
> 
> While writing a new testcase for this another bug was found that
> prevented updating a just created phdr because elf_getphdrnum would
> sanity check the phdr offset in the file (which doesn't exist yet).
> Fix that by only doing such a sanity check if the phdrs haven't been
> read in or created yet.
> 
> This second bug should have been found by the existing elfshphehdr
> test, but that test contained a typo checking elf_getphdrnum.
> It tested that the called failed when there were no phdrs, but then
> elf_getphdrnum should simply succeed and return zero.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1352232

I pushed this to master.

[PATCH] libelf: Allow updating phdrs for any e_type.

[Mark Wielaard]

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

elf[32|64]_updatenull would sanity check the e_type before allowing to
update the phdrs. This prevents creating an ET_REL file with phdrs. It
also prevents creating any vendor specific ELF file having phdrs. We
only check this when updating/writing out the file. But we would just
in such file. Don't prevent people from creating unexpected ELF files.
elflint will warn for such files.

While writing a new testcase for this another bug was found that
prevented updating a just created phdr because elf_getphdrnum would
sanity check the phdr offset in the file (which doesn't exist yet).
Fix that by only doing such a sanity check if the phdrs haven't been
read in or created yet.

This second bug should have been found by the existing elfshphehdr
test, but that test contained a typo checking elf_getphdrnum.
It tested that the called failed when there were no phdrs, but then
elf_getphdrnum should simply succeed and return zero.

https://bugzilla.redhat.com/show_bug.cgi?id=1352232

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libelf/ChangeLog          |   7 ++
 libelf/elf32_updatenull.c |  15 +---
 libelf/elf_getphdrnum.c   |  58 +++++++-------
 tests/ChangeLog           |   8 ++
 tests/Makefile.am         |   5 +-
 tests/elfshphehdr.c       |   2 +-
 tests/vendorelf.c         | 197 ++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 249 insertions(+), 43 deletions(-)
 create mode 100644 tests/vendorelf.c

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 82a2a9f..d445fe6 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,10 @@
+2016-07-06  Mark Wielaard  <mjw@redhat.com>
+
+	* elf32_updatenull.c (updatenull_wrlock): Ignore e_type when
+	updating phdrs.
+	* elf_getphdrnum.c (__elf_getphdrnum_chk_rdlock): Only do sanity
+	checking if phdrs haven't been read in yet.
+
 2016-06-24  John Ogness  <john.ogness@linutronix.de>
 
 	* elf32_updatenull.c (updatenull_wrlock): Find first section.
diff --git a/libelf/elf32_updatenull.c b/libelf/elf32_updatenull.c
index 7507062..939aa13 100644
--- a/libelf/elf32_updatenull.c
+++ b/libelf/elf32_updatenull.c
@@ -1,5 +1,5 @@
 /* Update data structures for changes.
-   Copyright (C) 2000-2010, 2015 Red Hat, Inc.
+   Copyright (C) 2000-2010, 2015, 2016 Red Hat, Inc.
    This file is part of elfutils.
    Written by Ulrich Drepper <drepper@redhat.com>, 2000.
 
@@ -140,21 +140,10 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int *change_bop, size_t shnum)
   off_t size = elf_typesize (LIBELFBITS, ELF_T_EHDR, 1);
 
   /* Set the program header position.  */
-  if (elf->state.ELFW(elf,LIBELFBITS).phdr == NULL
-      && (ehdr->e_type == ET_EXEC || ehdr->e_type == ET_DYN
-	  || ehdr->e_type == ET_CORE))
+  if (elf->state.ELFW(elf,LIBELFBITS).phdr == NULL)
     (void) __elfw2(LIBELFBITS,getphdr_wrlock) (elf);
   if (elf->state.ELFW(elf,LIBELFBITS).phdr != NULL)
     {
-      /* Only executables, shared objects, and core files have a program
-	 header.  */
-      if (ehdr->e_type != ET_EXEC && ehdr->e_type != ET_DYN
-	  && unlikely (ehdr->e_type != ET_CORE))
-	{
-	  __libelf_seterrno (ELF_E_INVALID_PHDR);
-	  return -1;
-	}
-
       size_t phnum;
       if (unlikely (__elf_getphdrnum_rdlock (elf, &phnum) != 0))
 	return -1;
diff --git a/libelf/elf_getphdrnum.c b/libelf/elf_getphdrnum.c
index 061183b..f91cba9 100644
--- a/libelf/elf_getphdrnum.c
+++ b/libelf/elf_getphdrnum.c
@@ -1,5 +1,5 @@
 /* Return number of program headers in the ELF file.
-   Copyright (C) 2010, 2014, 2015 Red Hat, Inc.
+   Copyright (C) 2010, 2014, 2015, 2016 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -84,35 +84,39 @@ __elf_getphdrnum_chk_rdlock (Elf *elf, size_t *dst)
 {
   int result = __elf_getphdrnum_rdlock (elf, dst);
 
-  /* Do some sanity checking to make sure phnum and phoff are consistent.  */
-  Elf64_Off off = (elf->class == ELFCLASS32
-		   ? elf->state.elf32.ehdr->e_phoff
-		   : elf->state.elf64.ehdr->e_phoff);
-  if (unlikely (off == 0))
+  /* If the phdrs haven't been created or read in yet then do some
+     sanity checking to make sure phnum and phoff are consistent.  */
+  if (elf->state.elf.phdr == NULL)
     {
-      *dst = 0;
-      return result;
+      Elf64_Off off = (elf->class == ELFCLASS32
+		       ? elf->state.elf32.ehdr->e_phoff
+		       : elf->state.elf64.ehdr->e_phoff);
+      if (unlikely (off == 0))
+	{
+	  *dst = 0;
+	  return result;
+	}
+
+      if (unlikely (off >= elf->maximum_size))
+	{
+	  __libelf_seterrno (ELF_E_INVALID_DATA);
+	  return -1;
+	}
+
+      /* Check for too many sections.  */
+      size_t phdr_size = (elf->class == ELFCLASS32
+			  ? sizeof (Elf32_Phdr) : sizeof (Elf64_Phdr));
+      if (unlikely (*dst > SIZE_MAX / phdr_size))
+	{
+	  __libelf_seterrno (ELF_E_INVALID_DATA);
+	  return -1;
+	}
+
+      /* Truncated file?  Don't return more than can be indexed.  */
+      if (unlikely (elf->maximum_size - off < *dst * phdr_size))
+	*dst = (elf->maximum_size - off) / phdr_size;
     }
 
-  if (unlikely (off >= elf->maximum_size))
-    {
-      __libelf_seterrno (ELF_E_INVALID_DATA);
-      return -1;
-    }
-
-  /* Check for too many sections.  */
-  size_t phdr_size = (elf->class == ELFCLASS32
-		      ? sizeof (Elf32_Phdr) : sizeof (Elf64_Phdr));
-  if (unlikely (*dst > SIZE_MAX / phdr_size))
-    {
-      __libelf_seterrno (ELF_E_INVALID_DATA);
-      return -1;
-    }
-
-  /* Truncated file?  Don't return more than can be indexed.  */
-  if (unlikely (elf->maximum_size - off < *dst * phdr_size))
-    *dst = (elf->maximum_size - off) / phdr_size;
-
   return result;
 }
 
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 73aad09..cd0d2fe 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,11 @@
+2016-07-06  Mark Wielaard  <mjw@redhat.com>
+
+	* Makefile.am (check_PROGRAMS): Add vendorelf.
+	(TESTS): Likewise.
+	(vendorelf_LDADD): New variable.
+	* vendorelf.c: New test.
+	* elfshphehdr.c (test): Check elf_getphdrnum succeeds.
+
 2016-06-24  Mark Wielaard  <mjw@redhat.com>
 
 	* Makefile.am (check_PROGRAMS): Add emptyfile.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9c22c4d..f80436a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -53,7 +53,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  buildid deleted deleted-lib.so aggregate_size vdsosyms \
 		  getsrc_die strptr newdata elfstrtab dwfl-proc-attach \
 		  elfshphehdr elfstrmerge dwelfgnucompressed elfgetchdr \
-		  elfgetzdata elfputzdata zstrptr emptyfile
+		  elfgetzdata elfputzdata zstrptr emptyfile vendorelf
 
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
 	    asm-tst6 asm-tst7 asm-tst8 asm-tst9
@@ -127,7 +127,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
 	run-elfgetzdata.sh run-elfputzdata.sh run-zstrptr.sh \
 	run-compress-test.sh \
 	run-readelf-zdebug.sh run-readelf-zdebug-rel.sh \
-	emptyfile
+	emptyfile vendorelf
 
 if !BIARCH
 export ELFUTILS_DISABLE_BIARCH = 1
@@ -483,6 +483,7 @@ elfgetzdata_LDADD = $(libelf)
 elfputzdata_LDADD = $(libelf)
 zstrptr_LDADD = $(libelf)
 emptyfile_LDADD = $(libelf)
+vendorelf_LDADD = $(libelf)
 
 # We want to test the libelf header against the system elf.h header.
 # Don't include any -I CPPFLAGS.
diff --git a/tests/elfshphehdr.c b/tests/elfshphehdr.c
index 0d92934..5a297e0 100644
--- a/tests/elfshphehdr.c
+++ b/tests/elfshphehdr.c
@@ -126,7 +126,7 @@ test (Elf *elf, int class, bool layout)
   check_elf ("elf_getshdrnum", elf_getshdrnum (elf, &shnum) == 0);
   check ("shnum == 1", shnum == 2); /* section zero is also created.  */
 
-  check_elf ("elf_getphdrnum", elf_getphdrnum (elf, &phnum) != 0);
+  check_elf ("elf_getphdrnum", elf_getphdrnum (elf, &phnum) == 0);
   check ("phnum == 1", phnum == 1);
 
   check_elf ("gelf_getehdr", gelf_getehdr (elf, &ehdr) != NULL);
diff --git a/tests/vendorelf.c b/tests/vendorelf.c
new file mode 100644
index 0000000..bc13cce
--- /dev/null
+++ b/tests/vendorelf.c
@@ -0,0 +1,197 @@
+/* Test program for adding a program header to a vendor specific ELF file.
+   Copyright (C) 2016 Red Hat, Inc.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   elfutils 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include ELFUTILS_HEADER(elf)
+#include <gelf.h>
+
+void
+check_elf (const char *fname, int class, int use_mmap)
+{
+  printf ("\nfname: %s\n", fname);
+
+  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s': %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  Elf *elf = elf_begin (fd, use_mmap ? ELF_C_WRITE_MMAP : ELF_C_WRITE, NULL);
+  if (elf == NULL)
+    {
+      printf ("cannot create ELF descriptor: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Create an ELF header.
+  if (gelf_newehdr (elf, class) == 0)
+    {
+      printf ("cannot create ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  GElf_Ehdr ehdr_mem;
+  GElf_Ehdr *ehdr = gelf_getehdr (elf, &ehdr_mem);
+  if (ehdr == NULL)
+    {
+      printf ("cannot get ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Initialize header.
+  ehdr->e_ident[EI_DATA] = class == ELFCLASS64 ? ELFDATA2LSB : ELFDATA2MSB;
+  ehdr->e_ident[EI_OSABI] = ELFOSABI_GNU;
+  ehdr->e_type = ET_LOOS + 1;
+  ehdr->e_machine = EM_X86_64;
+  ehdr->e_version = EV_CURRENT;
+
+  if (gelf_update_ehdr (elf, ehdr) == 0)
+    {
+      printf ("cannot update ELF header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Create a program header.
+  if (gelf_newphdr (elf, 1) == 0)
+    {
+      printf ("cannot create program header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  GElf_Phdr phdr;
+  if (gelf_getphdr (elf, 0, &phdr) == NULL)
+    {
+      printf ("cannot get program header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Some random values to check later.
+  phdr.p_type = PT_NULL;
+  phdr.p_offset = 0;
+  phdr.p_vaddr = 0;
+  phdr.p_paddr = 1;
+  phdr.p_filesz = 0;
+  phdr.p_memsz = 1024;
+  phdr.p_flags = PF_R;
+  phdr.p_align = 16;
+
+  if (gelf_update_phdr (elf, 0, &phdr) == 0)
+    {
+      printf ("cannot update program header: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Write everything to disk.
+  if (elf_update (elf, ELF_C_WRITE) < 0)
+    {
+      printf ("failure in elf_update(WRITE): %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  /* Reread the ELF from disk now.  */
+  fd = open (fname, O_RDONLY, 0666);
+  if (fd == -1)
+    {
+      printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
+      exit (1);
+    }
+
+  elf = elf_begin (fd, use_mmap ? ELF_C_READ_MMAP : ELF_C_READ, NULL);
+  if (elf == NULL)
+    {
+      printf ("cannot create ELF descriptor read-only: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  // Is our phdr there?
+  size_t phnum;
+  if (elf_getphdrnum (elf, &phnum) != 0)
+    {
+      printf ("cannot get phdr num: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (phnum != 1)
+    {
+      printf ("Expected just 1 phdr, got: %zd\n", phnum);
+      exit (1);
+    }
+
+  if (gelf_getphdr (elf, 0, &phdr) == NULL)
+    {
+      printf ("cannot get program header from file: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  if (phdr.p_type != PT_NULL
+      || phdr.p_offset != 0
+      || phdr.p_vaddr != 0
+      || phdr.p_paddr != 1
+      || phdr.p_filesz != 0
+      || phdr.p_memsz != 1024
+      || phdr.p_flags != PF_R
+      || phdr.p_align != 16)
+    {
+      printf ("Unexpected phdr values\n");
+      exit (1);
+    }
+
+  if (elf_end (elf) != 0)
+    {
+      printf ("failure in elf_end: %s\n", elf_errmsg (-1));
+      exit (1);
+    }
+
+  close (fd);
+
+  unlink (fname);
+}
+
+int
+main (int argc __attribute__ ((unused)),
+      char *argv[] __attribute__ ((unused)))
+{
+  elf_version (EV_CURRENT);
+
+  check_elf ("vendor.elf.32", ELFCLASS32, 0);
+  check_elf ("vendor.elf.32.mmap", ELFCLASS32, 1);
+  check_elf ("vendor.elf.64", ELFCLASS64, 0);
+  check_elf ("vendor.elf.64.mmap", ELFCLASS64, 1);
+
+  return 0;
+}
-- 
1.8.3.1