failed reading "Smallest x86 ELF Hello World"

failed reading "Smallest x86 ELF Hello World"

[Josh Stone]

From http://timelessname.com/elfbin/

Elfutils completely fails to read the file:

$ eu-readelf --version
eu-readelf (elfutils) 0.169
Copyright [...]
$ eu-readelf -a hello
eu-readelf: failed reading 'hello': invalid file descriptor

It's not clear that we should care, since that page even says it's "a
completely corrupted x86 ELF Binary that still runs."  But since it's
good enough for the kernel to run it, I'd hope for *something* from tools.

It also seems weird to get the message "invalid file descriptor", from
ELF_E_INVALID_FILE, which makes it sound like more like EBADF.  The file
descriptor itself is fine - it just doesn't like the ELF within.
(DWARF_E_INVALID_FILE's message is just "invalid file".)


FWIW, binutils readelf does better:

$ readelf --version
GNU readelf version 2.27-24.fc26
Copyright [...]
$ readelf -a hello
ELF Header:
  Magic:   7f 45 4c 46 01 01 01 48 69 20 57 6f 72 6c 64 0a
  Class:                             ELF32
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            <unknown: 48>
  ABI Version:                       105
  Type:                              EXEC (Executable file)
  Machine:                           Intel 80386
  Version:                           0x1
  Entry point address:               0x8048080
  Start of program headers:          52 (bytes into file)
  Start of section headers:          309248 (bytes into file)
  Flags:                             0x80cd0000
  Size of this header:               22763 (bytes)
  Size of program headers:           32 (bytes)
  Number of program headers:         2
  Size of section headers:           40 (bytes)
  Number of section headers:         5
  Section header string table index: 4
readelf: hello: Error: Reading 0xc8 bytes extends past end of file for
section headers
readelf: hello: Error: Section headers are not available!

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0x08048000 0x08048000 0x000a2 0x000a2 R E 0x1000
  LOAD           0x0000a4 0x080490a4 0x080490a4 0x00009 0x00009  W
0x9007b900

There is no dynamic section in this file.

Re: failed reading "Smallest x86 ELF Hello World"

[Mark Wielaard]

On Thu, 2017-09-28 at 12:06 -0700, Josh Stone wrote:
> From http://timelessname.com/elfbin/
> 
> Elfutils completely fails to read the file:
> 
> $ eu-readelf --version
> eu-readelf (elfutils) 0.169
> Copyright [...]
> $ eu-readelf -a hello
> eu-readelf: failed reading 'hello': invalid file descriptor
> 
> It's not clear that we should care, since that page even says it's "a
> completely corrupted x86 ELF Binary that still runs."  But since it's
> good enough for the kernel to run it, I'd hope for *something* from
> tools.
> 
> It also seems weird to get the message "invalid file descriptor",
> from ELF_E_INVALID_FILE, which makes it sound like more like
> EBADF.  The file descriptor itself is fine - it just doesn't like the
> ELF within.

At first I though it was because eu-readelf uses libdwfl to get more
information about the ELF file. To get all the data about the
Dwfl_Modules we might hit some error that might not be relevant for the
initial showing of data. But that isn't really it in this case.

The first issue is indeed that almost anything that goes wrong when
setting up the initial Elf handle ends up being described as
ELF_E_INVALID_FILE. Which is not always the correct error code. So I
introduced ELF_E_INVALID_ELF which indicates it is bad ELF data being
encountered and not just the inability to read the data from the file
descriptor. Also in a couple of cases we didn't explicitly set the
libelf errno to indicate what really went wrong. I made sure we always
do now.

  libelf: Add ELF_E_INVALID_ELF error value.

This at least gives a nicer error message:

  eu-readelf: failed reading './hello': invalid ELF file data

But while auditing this code it is clear we go out of our way to get
the section (count) making sure we don't touch any bad data. If there
is a change we might read anything bad from the (mmapped) file then we
explicitly set the elf->state.elf[64|32].scns.cnt to zero. Which is
respected throughout libelf whenever we try to touch section headers.
Except... during the initial read we double check e_shoff is sane and
error out early. Even though the code right below it explicitly doesn't
use it when scncnt is zero. So we can fix this sanity check.

  libelf: Don't error out when sanity checking e_shoff if scncnt is
zero.

This then gives similar output to binutils readelf:

ELF Header:
  Magic:   7f 45 4c 46 01 01 01 48 69 20 57 6f 72 6c 64 0a
  Class:                             ELF32
  Data:                              2's complement, little endian
  Ident Version:                     1 (current)
  OS/ABI:                            <unknown>: 72
  ABI Version:                       105
  Type:                              EXEC (Executable file)
  Machine:                           Intel 80386
  Version:                           1 (current)
  Entry point address:               0x8048080
  Start of program headers:          52 (bytes into file)
  Start of section headers:          309248 (bytes into file)
  Flags:                             0x80cd0000
  Size of this header:               22763 (bytes)
  Size of program header entries:    32 (bytes)
  Number of program headers entries: 2
  Size of section header entries:    40 (bytes)
  Number of section headers entries: 5
  Section header string table index: 4

Section Headers:
[Nr] Name                 Type         Addr     Off    Size   ES Flags Lk Inf Al

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz  MemSiz   Flg Align
  LOAD           0x000000 0x08048000 0x08048000 0x0000a2 0x0000a2 R E 0x1000
  LOAD           0x0000a4 0x080490a4 0x080490a4 0x000009 0x000009  W  0x9007b900

 Section to Segment mapping:
  Segment Sections...
   00     
   01     

Maybe it could be improved a little more by adding a warning that 5
section [headers] were expected, but none could be read. But I leave it
at this for now.

Cheers,

Mark

[PATCH 2/2] libelf: Don't error out when sanity checking e_shoff if scncnt is zero.

[Mark Wielaard]

We won't use the e_shoff value in that case because we will set
elf->state.elf[64|32].scns.cnt to zero to indicate not to read
any section header data from the file.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog   | 5 +++++
 libelf/elf_begin.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 36b57dd..6aa1c6f 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,5 +1,10 @@
 2017-10-04  Mark Wielaard  <mark@klomp.org>
 
+	* elf_begin.c (file_read_elf): Skip sanity checking e_shoff if scncnt
+	is zero, we won't use it then.
+
+2017-10-04  Mark Wielaard  <mark@klomp.org>
+
 	* libelfP.h: Add ELF_E_INVALID_ELF to error values enum.
 	* elf_error.c (ELF_E_INVALID_ELF_IDX): New define. Use it as value
 	for ELF_E_INVALID_ELF in msgidx.
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 5545278..fb3a5b5 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -373,7 +373,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
 	      || (((uintptr_t) ((char *) ehdr + e_shoff)
 		   & (__alignof__ (Elf32_Shdr) - 1)) == 0)))
 	{
-	  if (unlikely (e_shoff >= maxsize)
+	  if (unlikely (scncnt > 0 && e_shoff >= maxsize)
 	      || unlikely (maxsize - e_shoff
 			   < scncnt * sizeof (Elf32_Shdr)))
 	    {
@@ -475,7 +475,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
 	      || (((uintptr_t) ((char *) ehdr + e_shoff)
 		   & (__alignof__ (Elf64_Shdr) - 1)) == 0)))
 	{
-	  if (unlikely (e_shoff >= maxsize)
+	  if (unlikely (scncnt > 0 && e_shoff >= maxsize)
 	      || unlikely (maxsize - e_shoff
 			   < scncnt * sizeof (Elf64_Shdr)))
 	    goto free_and_out;
-- 
1.8.3.1

[PATCH 1/2] libelf: Add ELF_E_INVALID_ELF error value.

[Mark Wielaard]

Add ELF_E_INVALID_ELF which is set when the ELF file data is bad.
This is different from ELF_E_INVALID_FILE which is set when the file
could not be read.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog           | 12 +++++++++
 libelf/elf_begin.c         | 61 ++++++++++++++++++++++++++++++++++------------
 libelf/elf_error.c         |  7 +++++-
 libelf/elf_getshdrstrndx.c | 20 ++++++++++-----
 libelf/libelfP.h           |  1 +
 tests/ChangeLog            |  4 +++
 tests/msg_tst.c            |  1 +
 7 files changed, 83 insertions(+), 23 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 7bd9e1b..36b57dd 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,15 @@
+2017-10-04  Mark Wielaard  <mark@klomp.org>
+
+	* libelfP.h: Add ELF_E_INVALID_ELF to error values enum.
+	* elf_error.c (ELF_E_INVALID_ELF_IDX): New define. Use it as value
+	for ELF_E_INVALID_ELF in msgidx.
+	* elf_getshdrstrndx.c (elf_getshdrstrndx): Distinquish between pread
+	failing and not having enough data.
+	* elf_begin.c (get_shnum): Likewise. Explicitly set libelf errno on
+	too large value.
+	(file_read_elf): Make sure to always set libelf errno when returning
+	NULL. Distinquish between i/o file and elf data errors.
+
 2017-08-18  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* gelf_xlate.c: Use attribute_packed.
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 6f85038..5545278 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -158,6 +158,7 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset,
 	  else
 	    {
 	      Elf32_Word size;
+	      ssize_t r;
 
 	      if (likely (map_address != NULL))
 		/* gcc will optimize the memcpy to a simple memory
@@ -167,11 +168,19 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset,
 						 + offset))->sh_size,
 			sizeof (Elf32_Word));
 	      else
-		if (unlikely (pread_retry (fildes, &size, sizeof (Elf32_Word),
-					   offset + ehdr.e32->e_shoff
-					   + offsetof (Elf32_Shdr, sh_size))
+		if (unlikely ((r = pread_retry (fildes, &size,
+						sizeof (Elf32_Word),
+						offset + ehdr.e32->e_shoff
+						+ offsetof (Elf32_Shdr,
+							    sh_size)))
 			      != sizeof (Elf32_Word)))
-		  return (size_t) -1l;
+		  {
+		    if (r < 0)
+		      __libelf_seterrno (ELF_E_INVALID_FILE);
+		    else
+		      __libelf_seterrno (ELF_E_INVALID_ELF);
+		    return (size_t) -1l;
+		  }
 
 	      if (e_ident[EI_DATA] != MY_ELFDATA)
 		CONVERT (size);
@@ -207,6 +216,7 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset,
 				    + offset))->sh_size;
 	  else
 	    {
+	      ssize_t r;
 	      if (likely (map_address != NULL))
 		/* gcc will optimize the memcpy to a simple memory
 		   access while taking care of alignment issues.  */
@@ -215,19 +225,30 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset,
 						 + offset))->sh_size,
 			sizeof (Elf64_Xword));
 	      else
-		if (unlikely (pread_retry (fildes, &size, sizeof (Elf64_Xword),
-					   offset + ehdr.e64->e_shoff
-					   + offsetof (Elf64_Shdr, sh_size))
+		if (unlikely ((r = pread_retry (fildes, &size,
+						sizeof (Elf64_Xword),
+						offset + ehdr.e64->e_shoff
+						+ offsetof (Elf64_Shdr,
+							    sh_size)))
 			      != sizeof (Elf64_Xword)))
-		  return (size_t) -1l;
+		  {
+		    if (r < 0)
+		      __libelf_seterrno (ELF_E_INVALID_FILE);
+		    else
+		      __libelf_seterrno (ELF_E_INVALID_ELF);
+		    return (size_t) -1l;
+		  }
 
 	      if (e_ident[EI_DATA] != MY_ELFDATA)
 		CONVERT (size);
 	    }
 
 	  if (size > ~((GElf_Word) 0))
-	    /* Invalid value, it is too large.  */
-	    return (size_t) -1l;
+	    {
+	      /* Invalid value, it is too large.  */
+	      __libelf_seterrno (ELF_E_INVALID_ELF);
+	      return (size_t) -1l;
+	    }
 
 	  result = size;
 	}
@@ -255,11 +276,13 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
 		    && e_ident[EI_DATA] != ELFDATA2MSB)))
     {
       /* Cannot handle this.  */
-      __libelf_seterrno (ELF_E_INVALID_FILE);
+      __libelf_seterrno (ELF_E_INVALID_ELF);
       return NULL;
     }
 
-  /* Determine the number of sections.  */
+  /* Determine the number of sections.  Returns -1 and sets libelf errno
+     if the file handle or elf file is invalid.  Returns zero if there
+     are no section headers (or they cannot be read).  */
   size_t scncnt = get_shnum (map_address, e_ident, fildes, offset, maxsize);
   if (scncnt == (size_t) -1l)
     /* Could not determine the number of sections.  */
@@ -269,10 +292,16 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
   if (e_ident[EI_CLASS] == ELFCLASS32)
     {
       if (scncnt > SIZE_MAX / (sizeof (Elf_Scn) + sizeof (Elf32_Shdr)))
-	return NULL;
+	{
+	  __libelf_seterrno (ELF_E_INVALID_ELF);
+	  return NULL;
+	}
     }
   else if (scncnt > SIZE_MAX / (sizeof (Elf_Scn) + sizeof (Elf64_Shdr)))
-    return NULL;
+    {
+      __libelf_seterrno (ELF_E_INVALID_ELF);
+      return NULL;
+    }
 
   /* We can now allocate the memory.  Even if there are no section headers,
      we allocate space for a zeroth section in case we need it later.  */
@@ -281,7 +310,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
   Elf *elf = allocate_elf (fildes, map_address, offset, maxsize, cmd, parent,
 			   ELF_K_ELF, scnmax * sizeof (Elf_Scn));
   if (elf == NULL)
-    /* Not enough memory.  */
+    /* Not enough memory.  allocate_elf will have set libelf errno.  */
     return NULL;
 
   assert ((unsigned int) scncnt == scncnt);
@@ -350,7 +379,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
 	    {
 	    free_and_out:
 	      free (elf);
-	      __libelf_seterrno (ELF_E_INVALID_FILE);
+	      __libelf_seterrno (ELF_E_INVALID_ELF);
 	      return NULL;
 	    }
 	  elf->state.elf32.shdr
diff --git a/libelf/elf_error.c b/libelf/elf_error.c
index 888b389..5364e68 100644
--- a/libelf/elf_error.c
+++ b/libelf/elf_error.c
@@ -94,8 +94,12 @@ static const char msgstr[] =
   (ELF_E_NOMEM_IDX + sizeof "out of memory")
   N_("invalid file descriptor")
   "\0"
-#define ELF_E_INVALID_OP_IDX \
+#define ELF_E_INVALID_ELF_IDX \
   (ELF_E_INVALID_FILE_IDX + sizeof "invalid file descriptor")
+  N_("invalid ELF file data")
+  "\0"
+#define ELF_E_INVALID_OP_IDX \
+  (ELF_E_INVALID_ELF_IDX + sizeof "invalid ELF file data")
   N_("invalid operation")
   "\0"
 #define ELF_E_NO_VERSION_IDX \
@@ -280,6 +284,7 @@ static const uint_fast16_t msgidx[ELF_E_NUM] =
   [ELF_E_INVALID_ENCODING] = ELF_E_INVALID_ENCODING_IDX,
   [ELF_E_NOMEM] = ELF_E_NOMEM_IDX,
   [ELF_E_INVALID_FILE] = ELF_E_INVALID_FILE_IDX,
+  [ELF_E_INVALID_ELF] = ELF_E_INVALID_ELF_IDX,
   [ELF_E_INVALID_OP] = ELF_E_INVALID_OP_IDX,
   [ELF_E_NO_VERSION] = ELF_E_NO_VERSION_IDX,
   [ELF_E_INVALID_CMD] = ELF_E_INVALID_CMD_IDX,
diff --git a/libelf/elf_getshdrstrndx.c b/libelf/elf_getshdrstrndx.c
index aead2fe..ad884fd 100644
--- a/libelf/elf_getshdrstrndx.c
+++ b/libelf/elf_getshdrstrndx.c
@@ -133,13 +133,17 @@ elf_getshdrstrndx (Elf *elf, size_t *dst)
 		  /* We avoid reading in all the section headers.  Just read
 		     the first one.  */
 		  Elf32_Shdr shdr_mem;
+		  ssize_t r;
 
-		  if (unlikely (pread_retry (elf->fildes, &shdr_mem,
-					     sizeof (Elf32_Shdr), offset)
+		  if (unlikely ((r = pread_retry (elf->fildes, &shdr_mem,
+						  sizeof (Elf32_Shdr), offset))
 				!= sizeof (Elf32_Shdr)))
 		    {
 		      /* We must be able to read this ELF section header.  */
-		      __libelf_seterrno (ELF_E_INVALID_FILE);
+		      if (r < 0)
+			__libelf_seterrno (ELF_E_INVALID_FILE);
+		      else
+			__libelf_seterrno (ELF_E_INVALID_ELF);
 		      result = -1;
 		      goto out;
 		    }
@@ -194,13 +198,17 @@ elf_getshdrstrndx (Elf *elf, size_t *dst)
 		  /* We avoid reading in all the section headers.  Just read
 		     the first one.  */
 		  Elf64_Shdr shdr_mem;
+		  ssize_t r;
 
-		  if (unlikely (pread_retry (elf->fildes, &shdr_mem,
-					     sizeof (Elf64_Shdr), offset)
+		  if (unlikely ((r = pread_retry (elf->fildes, &shdr_mem,
+						  sizeof (Elf64_Shdr), offset))
 				!= sizeof (Elf64_Shdr)))
 		    {
 		      /* We must be able to read this ELF section header.  */
-		      __libelf_seterrno (ELF_E_INVALID_FILE);
+		      if (r < 0)
+			__libelf_seterrno (ELF_E_INVALID_FILE);
+		      else
+			__libelf_seterrno (ELF_E_INVALID_ELF);
 		      result = -1;
 		      goto out;
 		    }
diff --git a/libelf/libelfP.h b/libelf/libelfP.h
index a4a0a3a..ca805ac 100644
--- a/libelf/libelfP.h
+++ b/libelf/libelfP.h
@@ -102,6 +102,7 @@ enum
   ELF_E_INVALID_ENCODING,
   ELF_E_NOMEM,
   ELF_E_INVALID_FILE,
+  ELF_E_INVALID_ELF,
   ELF_E_INVALID_OP,
   ELF_E_NO_VERSION,
   ELF_E_INVALID_CMD,
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 7b6bf30..35688dc 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,7 @@
+2017-10-04  Mark Wielaard  <mark@klomp.org>
+
+	* msg_tst.c: Handle ELF_E_INVALID_ELF.
+
 2017-09-10  Mark Wielaard  <mark@klomp.org>
 
 	* run-ar.sh: New test.
diff --git a/tests/msg_tst.c b/tests/msg_tst.c
index 7baea0a..aa974d0 100644
--- a/tests/msg_tst.c
+++ b/tests/msg_tst.c
@@ -39,6 +39,7 @@ static struct
     { ELF_E_INVALID_ENCODING, "invalid encoding" },
     { ELF_E_NOMEM, "out of memory" },
     { ELF_E_INVALID_FILE, "invalid file descriptor" },
+    { ELF_E_INVALID_ELF, "invalid ELF file data" },
     { ELF_E_INVALID_OP, "invalid operation" },
     { ELF_E_NO_VERSION, "ELF version not set" },
     { ELF_E_INVALID_CMD, "invalid command" },
-- 
1.8.3.1

Re: failed reading "Smallest x86 ELF Hello World"

[Mark Wielaard]

On Wed, 2017-10-04 at 20:55 +0200, Mark Wielaard wrote:
> The first issue is indeed that almost anything that goes wrong when
> setting up the initial Elf handle ends up being described as
> ELF_E_INVALID_FILE. Which is not always the correct error code. So I
> introduced ELF_E_INVALID_ELF which indicates it is bad ELF data being
> encountered and not just the inability to read the data from the file
> descriptor. Also in a couple of cases we didn't explicitly set the
> libelf errno to indicate what really went wrong. I made sure we
> always
> do now.
> 
>   libelf: Add ELF_E_INVALID_ELF error value.
>
> [...]
>
> But while auditing this code it is clear we go out of our way to get
> the section (count) making sure we don't touch any bad data. If there
> is a change we might read anything bad from the (mmapped) file then
> we
> explicitly set the elf->state.elf[64|32].scns.cnt to zero. Which is
> respected throughout libelf whenever we try to touch section headers.
> Except... during the initial read we double check e_shoff is sane and
> error out early. Even though the code right below it explicitly
> doesn't
> use it when scncnt is zero. So we can fix this sanity check.
> 
>   libelf: Don't error out when sanity checking e_shoff if scncnt is
> zero.

I pushed both these commits to master now.

Cheers,

Mark