libelf: Split checks for ehdr and shdr, drop phdr check in file_read_elf.

There are various places in the code that check whether mmapped structures
are correctly aligned (or ALLOW_UNALIGNED is set). Some of these checks
are asserts. Like the one in elf(32|64)_getshdr. We should not get into
that part of the code if the shdr scn structure was cached in elf_begin
because it was mmapped in and properly aligned.

These asserts could trigger because in elf_begin.c file_read_elf ()
all alignment checks were combined. So even though only one of the ehdr,
shdr or phdr structures were not properly aligned all structures would be
copied. Also the phdr structure was not even read in elf_begin, so the
alignment check was unnecessary.

This patch splits the alignment checks and reading of ehdr and shdr
structures into separate code paths. It also drops the phdr alignment
checks in elf_begin. Those phdr checks are done in elf(32|64)_getphdr
already.

Signed-off-by: Mark Wielaard <mjw@redhat.com>

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 79308fe8f9ab5eee7d05c2d6ef3166e0de579dc1..fd2fc53e0980bed4bfd1d17f1d3843758cb23129 100644 (file)
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-02  Mark Wielaard  <mjw@redhat.com>
+
+       * elf_begin.c (file_read_elf): Split checks for ehdr and shdr
+       alignment, drop phdr alignment check.
+
 2015-05-31  Mark Wielaard  <mjw@redhat.com>
 
        * elf32_getshdr.c (load_shdr_wrlock): Allocate shdrs with malloc,

diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index ae1e7124a6a1349b31288bab9736564007276554..e2e3b6b4dc00cc680fa5502624c77a3d08a0757e 100644 (file)
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -1,5 +1,5 @@
 /* Create descriptor for processing file.
-   Copyright (C) 1998-2010, 2012, 2014 Red Hat, Inc.
+   Copyright (C) 1998-2010, 2012, 2014, 2015 Red Hat, Inc.
    This file is part of elfutils.
    Written by Ulrich Drepper <drepper@redhat.com>, 1998.
 
@@ -306,17 +306,46 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
       /* This is a 32-bit binary.  */
       if (map_address != NULL && e_ident[EI_DATA] == MY_ELFDATA
          && (ALLOW_UNALIGNED
-             || ((((uintptr_t) ehdr) & (__alignof__ (Elf32_Ehdr) - 1)) == 0
-                 && ((uintptr_t) ((char *) ehdr + ehdr->e_shoff)
-                     & (__alignof__ (Elf32_Shdr) - 1)) == 0
-                 && ((uintptr_t) ((char *) ehdr + ehdr->e_phoff)
-                     & (__alignof__ (Elf32_Phdr) - 1)) == 0)))
+             || (((uintptr_t) ehdr) & (__alignof__ (Elf32_Ehdr) - 1)) == 0))
        {
          /* We can use the mmapped memory.  */
          elf->state.elf32.ehdr = ehdr;
+       }
+      else
+       {
+         /* Copy the ELF header.  */
+         elf->state.elf32.ehdr = memcpy (&elf->state.elf32.ehdr_mem, e_ident,
+                                         sizeof (Elf32_Ehdr));
+
+         if (e_ident[EI_DATA] != MY_ELFDATA)
+           {
+             CONVERT (elf->state.elf32.ehdr_mem.e_type);
+             CONVERT (elf->state.elf32.ehdr_mem.e_machine);
+             CONVERT (elf->state.elf32.ehdr_mem.e_version);
+             CONVERT (elf->state.elf32.ehdr_mem.e_entry);
+             CONVERT (elf->state.elf32.ehdr_mem.e_phoff);
+             CONVERT (elf->state.elf32.ehdr_mem.e_shoff);
+             CONVERT (elf->state.elf32.ehdr_mem.e_flags);
+             CONVERT (elf->state.elf32.ehdr_mem.e_ehsize);
+             CONVERT (elf->state.elf32.ehdr_mem.e_phentsize);
+             CONVERT (elf->state.elf32.ehdr_mem.e_phnum);
+             CONVERT (elf->state.elf32.ehdr_mem.e_shentsize);
+             CONVERT (elf->state.elf32.ehdr_mem.e_shnum);
+             CONVERT (elf->state.elf32.ehdr_mem.e_shstrndx);
+           }
+       }
+
+      /* Don't precache the phdr pointer here.
+        elf32_getphdr will validate it against the size when asked.  */
 
-         if (unlikely (ehdr->e_shoff >= maxsize)
-             || unlikely (maxsize - ehdr->e_shoff
+      Elf32_Off e_shoff = elf->state.elf32.ehdr->e_shoff;
+      if (map_address != NULL && e_ident[EI_DATA] == MY_ELFDATA
+         && (ALLOW_UNALIGNED
+             || (((uintptr_t) ((char *) ehdr + e_shoff)
+                  & (__alignof__ (Elf32_Shdr) - 1)) == 0)))
+       {
+         if (unlikely (e_shoff >= maxsize)
+             || unlikely (maxsize - e_shoff
                           < scncnt * sizeof (Elf32_Shdr)))
            {
            free_and_out:
@@ -325,10 +354,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
              return NULL;
            }
          elf->state.elf32.shdr
-           = (Elf32_Shdr *) ((char *) ehdr + ehdr->e_shoff);
-
-         /* Don't precache the phdr pointer here.
-            elf32_getphdr will validate it against the size when asked.  */
+           = (Elf32_Shdr *) ((char *) ehdr + e_shoff);
 
          for (size_t cnt = 0; cnt < scncnt; ++cnt)
            {
@@ -361,27 +387,6 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
        }
       else
        {
-         /* Copy the ELF header.  */
-         elf->state.elf32.ehdr = memcpy (&elf->state.elf32.ehdr_mem, e_ident,
-                                         sizeof (Elf32_Ehdr));
-
-         if (e_ident[EI_DATA] != MY_ELFDATA)
-           {
-             CONVERT (elf->state.elf32.ehdr_mem.e_type);
-             CONVERT (elf->state.elf32.ehdr_mem.e_machine);
-             CONVERT (elf->state.elf32.ehdr_mem.e_version);
-             CONVERT (elf->state.elf32.ehdr_mem.e_entry);
-             CONVERT (elf->state.elf32.ehdr_mem.e_phoff);
-             CONVERT (elf->state.elf32.ehdr_mem.e_shoff);
-             CONVERT (elf->state.elf32.ehdr_mem.e_flags);
-             CONVERT (elf->state.elf32.ehdr_mem.e_ehsize);
-             CONVERT (elf->state.elf32.ehdr_mem.e_phentsize);
-             CONVERT (elf->state.elf32.ehdr_mem.e_phnum);
-             CONVERT (elf->state.elf32.ehdr_mem.e_shentsize);
-             CONVERT (elf->state.elf32.ehdr_mem.e_shnum);
-             CONVERT (elf->state.elf32.ehdr_mem.e_shstrndx);
-           }
-
          for (size_t cnt = 0; cnt < scncnt; ++cnt)
            {
              elf->state.elf32.scns.data[cnt].index = cnt;
@@ -402,24 +407,50 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
       /* This is a 64-bit binary.  */
       if (map_address != NULL && e_ident[EI_DATA] == MY_ELFDATA
          && (ALLOW_UNALIGNED
-             || ((((uintptr_t) ehdr) & (__alignof__ (Elf64_Ehdr) - 1)) == 0
-                 && ((uintptr_t) ((char *) ehdr + ehdr->e_shoff)
-                     & (__alignof__ (Elf64_Shdr) - 1)) == 0
-                 && ((uintptr_t) ((char *) ehdr + ehdr->e_phoff)
-                     & (__alignof__ (Elf64_Phdr) - 1)) == 0)))
+             || (((uintptr_t) ehdr) & (__alignof__ (Elf64_Ehdr) - 1)) == 0))
        {
          /* We can use the mmapped memory.  */
          elf->state.elf64.ehdr = ehdr;
+       }
+      else
+       {
+         /* Copy the ELF header.  */
+         elf->state.elf64.ehdr = memcpy (&elf->state.elf64.ehdr_mem, e_ident,
+                                         sizeof (Elf64_Ehdr));
+
+         if (e_ident[EI_DATA] != MY_ELFDATA)
+           {
+             CONVERT (elf->state.elf64.ehdr_mem.e_type);
+             CONVERT (elf->state.elf64.ehdr_mem.e_machine);
+             CONVERT (elf->state.elf64.ehdr_mem.e_version);
+             CONVERT (elf->state.elf64.ehdr_mem.e_entry);
+             CONVERT (elf->state.elf64.ehdr_mem.e_phoff);
+             CONVERT (elf->state.elf64.ehdr_mem.e_shoff);
+             CONVERT (elf->state.elf64.ehdr_mem.e_flags);
+             CONVERT (elf->state.elf64.ehdr_mem.e_ehsize);
+             CONVERT (elf->state.elf64.ehdr_mem.e_phentsize);
+             CONVERT (elf->state.elf64.ehdr_mem.e_phnum);
+             CONVERT (elf->state.elf64.ehdr_mem.e_shentsize);
+             CONVERT (elf->state.elf64.ehdr_mem.e_shnum);
+             CONVERT (elf->state.elf64.ehdr_mem.e_shstrndx);
+           }
+       }
+
+      /* Don't precache the phdr pointer here.
+        elf64_getphdr will validate it against the size when asked.  */
 
-         if (unlikely (ehdr->e_shoff >= maxsize)
-             || unlikely (maxsize - ehdr->e_shoff
+      Elf64_Off e_shoff = elf->state.elf64.ehdr->e_shoff;
+      if (map_address != NULL && e_ident[EI_DATA] == MY_ELFDATA
+         && (ALLOW_UNALIGNED
+             || (((uintptr_t) ((char *) ehdr + e_shoff)
+                  & (__alignof__ (Elf64_Shdr) - 1)) == 0)))
+       {
+         if (unlikely (e_shoff >= maxsize)
+             || unlikely (maxsize - e_shoff
                           < scncnt * sizeof (Elf64_Shdr)))
            goto free_and_out;
          elf->state.elf64.shdr
-           = (Elf64_Shdr *) ((char *) ehdr + ehdr->e_shoff);
-
-         /* Don't precache the phdr pointer here.
-            elf64_getphdr will validate it against the size when asked.  */
+           = (Elf64_Shdr *) ((char *) ehdr + e_shoff);
 
          for (size_t cnt = 0; cnt < scncnt; ++cnt)
            {
@@ -452,27 +483,6 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
        }
       else
        {
-         /* Copy the ELF header.  */
-         elf->state.elf64.ehdr = memcpy (&elf->state.elf64.ehdr_mem, e_ident,
-                                         sizeof (Elf64_Ehdr));
-
-         if (e_ident[EI_DATA] != MY_ELFDATA)
-           {
-             CONVERT (elf->state.elf64.ehdr_mem.e_type);
-             CONVERT (elf->state.elf64.ehdr_mem.e_machine);
-             CONVERT (elf->state.elf64.ehdr_mem.e_version);
-             CONVERT (elf->state.elf64.ehdr_mem.e_entry);
-             CONVERT (elf->state.elf64.ehdr_mem.e_phoff);
-             CONVERT (elf->state.elf64.ehdr_mem.e_shoff);
-             CONVERT (elf->state.elf64.ehdr_mem.e_flags);
-             CONVERT (elf->state.elf64.ehdr_mem.e_ehsize);
-             CONVERT (elf->state.elf64.ehdr_mem.e_phentsize);
-             CONVERT (elf->state.elf64.ehdr_mem.e_phnum);
-             CONVERT (elf->state.elf64.ehdr_mem.e_shentsize);
-             CONVERT (elf->state.elf64.ehdr_mem.e_shnum);
-             CONVERT (elf->state.elf64.ehdr_mem.e_shstrndx);
-           }
-
          for (size_t cnt = 0; cnt < scncnt; ++cnt)
            {
              elf->state.elf64.scns.data[cnt].index = cnt;