[PATCH v3 0/5] Fix coverity bugs and add checks for elf values in grub-core

[PATCH v3 0/5] Fix coverity bugs and add checks for elf values in grub-core

[Alec Brown]

v3: Added check for e_shoff, made starting words lowercase in error messages,
and added comment to why return pointers are set to 0.

Coverity identified several untrusted loop bounds and untrusted allocation size
bugs in grub-core/loader/i386/bsdXX.c and grub-core/loader/multiboot_elfXX.c.
Upon review of these bugs, I found that specific checks weren't being made to
various elf header values based on the elf manual page. This patch series
addresses the coverity bugs, as well as adds functions to check for the correct
elf header values.

The Coverity bugs being addressed are:
CID 314018
CID 314030
CID 314031
CID 314039

Alec Brown (5):
      grub-core/loader/i386/bsdXX.c: Avoid downcasting (char *) to (Elf_Shdr *)
      elf: Validate number of elf section header table entries
      elf: Validate elf section header table index for section name string table
      elf: Validate number of elf program header table entries
      util/grub-module-verifierXX.c: Add e_shoff check in get_shdr()

 grub-core/kern/elf.c               |  15 +++++++++++++++
 grub-core/kern/elfXX.c             | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 grub-core/loader/i386/bsdXX.c      | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------
 grub-core/loader/multiboot_elfxx.c |  76 +++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------
 include/grub/elf.h                 |  18 ++++++++++++++++++
 util/grub-module-verifierXX.c      |   3 +++
 6 files changed, 273 insertions(+), 77 deletions(-)

[PATCH v3 1/5] grub-core/loader/i386/bsdXX.c: Avoid downcasting (char *) to (Elf_Shdr *)

[Alec Brown]

In bsdXX.c, a couple of untrusted loop bound and untrusted allocation size bugs
were flagged by Coverity in the functions grub_openbsd_find_ramdisk() and
grub_freebsd_load_elfmodule(). These bugs were flagged by coverity because the
variable shdr was downcasting from a char pointer to an Elf_Shdr pointer
whenever it was used to set the base value in for loops. To avoid this, we need
to set shdr as an Elf_Shdr pointer where it is initialized.

In the function read_headers(), the function is reading elf section header data
from a file and passing it to the variable shdr as data for a char pointer. If
we switch the type of shdr to an Elf_Shdr pointer in read_headers() as well as
other functions, then we won't need to downcast to an Elf_Shdr pointer and won't
have to worry about tainted data later in the code.

Fixes: CID 314018
Fixes: CID 314030
Fixes: CID 314031
Fixes: CID 314039

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/loader/i386/bsdXX.c | 66 +++++++++++++++--------------------
 1 file changed, 28 insertions(+), 38 deletions(-)

diff --git a/grub-core/loader/i386/bsdXX.c b/grub-core/loader/i386/bsdXX.c
index e4f4aa365..6946cecbb 100644
--- a/grub-core/loader/i386/bsdXX.c
+++ b/grub-core/loader/i386/bsdXX.c
@@ -24,7 +24,7 @@ load (grub_file_t file, const char *filename, void *where, grub_off_t off, grub_
 }
 
 static inline grub_err_t
-read_headers (grub_file_t file, const char *filename, Elf_Ehdr *e, char **shdr)
+read_headers (grub_file_t file, const char *filename, Elf_Ehdr *e, Elf_Shdr **shdr)
 {
  if (grub_file_seek (file, 0) == (grub_off_t) -1)
     return grub_errno;
@@ -78,7 +78,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
 {
   Elf_Ehdr e;
   Elf_Shdr *s;
-  char *shdr = 0;
+  Elf_Shdr *shdr = NULL;
   grub_addr_t curload, module;
   grub_err_t err;
   grub_size_t chunk_size = 0;
@@ -90,9 +90,8 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
   if (err)
     goto out;
 
-  for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) ((char *) shdr
-						+ e.e_shnum * e.e_shentsize);
-       s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+       s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
 	continue;
@@ -113,9 +112,8 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
     chunk_src = get_virtual_current_address (ch);
   }
 
-  for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) ((char *) shdr
-						+ e.e_shnum * e.e_shentsize);
-       s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+       s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
 	continue;
@@ -173,7 +171,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
 {
   Elf_Ehdr e;
   Elf_Shdr *s;
-  char *shdr = 0;
+  Elf_Shdr *shdr = NULL;
   grub_addr_t curload, module;
   grub_err_t err;
   grub_size_t chunk_size = 0;
@@ -185,9 +183,8 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
   if (err)
     goto out;
 
-  for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) ((char *) shdr
-						+ e.e_shnum * e.e_shentsize);
-       s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+       s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
 	continue;
@@ -214,9 +211,8 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
     chunk_src = get_virtual_current_address (ch);
   }
 
-  for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) ((char *) shdr
-						+ e.e_shnum * e.e_shentsize);
-       s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+       s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
 	continue;
@@ -285,7 +281,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct grub_relocator *relocator,
   grub_err_t err;
   Elf_Ehdr e;
   Elf_Shdr *s;
-  char *shdr = 0;
+  Elf_Shdr *shdr = NULL;
   unsigned symoff, stroff, symsize, strsize;
   grub_freebsd_addr_t symstart, symend, symentsize, dynamic;
   Elf_Sym *sym;
@@ -306,13 +302,11 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct grub_relocator *relocator,
   if (err)
     goto out;
 
-  for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) (shdr
-						+ e.e_shnum * e.e_shentsize);
-       s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+       s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
       if (s->sh_type == SHT_SYMTAB)
 	break;
-  if (s >= (Elf_Shdr *) ((char *) shdr
-			+ e.e_shnum * e.e_shentsize))
+  if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize))
     {
       err = grub_error (GRUB_ERR_BAD_OS, N_("no symbol table"));
       goto out;
@@ -320,7 +314,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct grub_relocator *relocator,
   symoff = s->sh_offset;
   symsize = s->sh_size;
   symentsize = s->sh_entsize;
-  s = (Elf_Shdr *) (shdr + e.e_shentsize * s->sh_link);
+  s = (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shentsize * s->sh_link);
   stroff = s->sh_offset;
   strsize = s->sh_size;
 
@@ -427,7 +421,7 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator *relocator,
   grub_err_t err;
   Elf_Ehdr e;
   Elf_Shdr *s, *symsh, *strsh;
-  char *shdr = NULL;
+  Elf_Shdr *shdr = NULL;
   unsigned symsize, strsize;
   void *sym_chunk;
   grub_uint8_t *curload;
@@ -443,20 +437,18 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator *relocator,
       return grub_errno;
     }
 
-  for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) (shdr
-						+ e.e_shnum * e.e_shentsize);
-       s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+       s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
       if (s->sh_type == SHT_SYMTAB)
 	break;
-  if (s >= (Elf_Shdr *) ((char *) shdr
-			+ e.e_shnum * e.e_shentsize))
+  if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize))
     {
       grub_free (shdr);
       return GRUB_ERR_NONE;
     }
   symsize = s->sh_size;
   symsh = s;
-  s = (Elf_Shdr *) (shdr + e.e_shentsize * s->sh_link);
+  s = (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shentsize * s->sh_link);
   strsize = s->sh_size;
   strsh = s;
 
@@ -490,9 +482,8 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator *relocator,
 
   curload += sizeof (e);
 
-  for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) (shdr
-						+ e.e_shnum * e.e_shentsize);
-       s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+       s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       Elf_Shdr *s2;
       s2 = (Elf_Shdr *) curload;
@@ -565,7 +556,7 @@ SUFFIX(grub_openbsd_find_ramdisk) (grub_file_t file,
     grub_err_t err;
     Elf_Ehdr e;
     Elf_Shdr *s;
-    char *shdr = NULL;
+    Elf_Shdr *shdr = NULL;
 
     err = read_headers (file, filename, &e, &shdr);
     if (err)
@@ -574,12 +565,11 @@ SUFFIX(grub_openbsd_find_ramdisk) (grub_file_t file,
 	return err;
       }
 
-    for (s = (Elf_Shdr *) shdr; s < (Elf_Shdr *) (shdr
-						  + e.e_shnum * e.e_shentsize);
-	 s = (Elf_Shdr *) ((char *) s + e.e_shentsize))
+    for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+	 s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
       if (s->sh_type == SHT_SYMTAB)
 	break;
-    if (s >= (Elf_Shdr *) ((char *) shdr + e.e_shnum * e.e_shentsize))
+    if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize))
       {
 	grub_free (shdr);
 	return GRUB_ERR_NONE;
@@ -589,7 +579,7 @@ SUFFIX(grub_openbsd_find_ramdisk) (grub_file_t file,
     symentsize = s->sh_entsize;
     symoff = s->sh_offset;
 
-    s = (Elf_Shdr *) (shdr + e.e_shentsize * s->sh_link);
+    s = (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shentsize * s->sh_link);
     stroff = s->sh_offset;
     strsize = s->sh_size;
     grub_free (shdr);
-- 
2.27.0

[PATCH v3 2/5] elf: Validate number of elf section header table entries

[Alec Brown]

In bsdXX.c and multiboot_elfXX.c, e_shnum is used to obtain the number of
section header table entries, but it wasn't being checked if the value was
there.

According to the elf(5) manual page,
"If the number of entries in the section header table is larger than or equal to
SHN_LORESERVE (0xff00), e_shnum holds the value zero and the real number of
entries in the section header table is held in the sh_size member of the intial
entry in section header table. Otherwise, the sh_size member of the initial
entry in the section header table holds the value zero."

Since this check wasn't being made, grub_elfXX_get_shnum() is being added to
elfXX.c to make this check and use whichever member doesn't have a value of
zero. If both are zero, then we must return an error. We also need to make sure
that e_shnum doesn't have a value greater than or equal to SHN_LORESERVE and
sh_size isn't less than SHN_LORESERVE.

Note that even though elf.c and elfXX.c are located in grub-core/kern, they are
compiled as modules and don't need the EXPORT_FUNC macro to define the functions
in elf.h.

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/kern/elf.c               |  9 ++++
 grub-core/kern/elfXX.c             | 34 +++++++++++++
 grub-core/loader/i386/bsdXX.c      | 82 ++++++++++++++++++++++--------
 grub-core/loader/multiboot_elfxx.c | 49 +++++++++++-------
 include/grub/elf.h                 |  9 ++++
 5 files changed, 142 insertions(+), 41 deletions(-)

diff --git a/grub-core/kern/elf.c b/grub-core/kern/elf.c
index 9d7149b38..5be770583 100644
--- a/grub-core/kern/elf.c
+++ b/grub-core/kern/elf.c
@@ -167,11 +167,14 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #define grub_elfXX_load_phdrs grub_elf32_load_phdrs
 #define ElfXX_Phdr Elf32_Phdr
 #define ElfXX_Ehdr Elf32_Ehdr
+#define ElfXX_Shdr Elf32_Shdr
+#define ElfXX_Word Elf32_Word
 #define grub_uintXX_t grub_uint32_t
 #define grub_swap_bytes_addrXX grub_swap_bytes32
 #define grub_swap_bytes_offXX grub_swap_bytes32
 #define grub_swap_bytes_XwordXX grub_swap_bytes32
 #define grub_elfXX_check_endianess_and_bswap_ehdr grub_elf32_check_endianess_and_bswap_ehdr
+#define grub_elfXX_get_shnum grub_elf32_get_shnum
 
 #include "elfXX.c"
 
@@ -185,11 +188,14 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #undef grub_elfXX_load_phdrs
 #undef ElfXX_Phdr
 #undef ElfXX_Ehdr
+#undef ElfXX_Shdr
+#undef ElfXX_Word
 #undef grub_uintXX_t
 #undef grub_swap_bytes_addrXX
 #undef grub_swap_bytes_offXX
 #undef grub_swap_bytes_XwordXX
 #undef grub_elfXX_check_endianess_and_bswap_ehdr
+#undef grub_elfXX_get_shnum
 
 \f
 /* 64-bit */
@@ -203,10 +209,13 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #define grub_elfXX_load_phdrs grub_elf64_load_phdrs
 #define ElfXX_Phdr Elf64_Phdr
 #define ElfXX_Ehdr Elf64_Ehdr
+#define ElfXX_Shdr Elf64_Shdr
+#define ElfXX_Word Elf64_Word
 #define grub_uintXX_t grub_uint64_t
 #define grub_swap_bytes_addrXX grub_swap_bytes64
 #define grub_swap_bytes_offXX grub_swap_bytes64
 #define grub_swap_bytes_XwordXX grub_swap_bytes64
 #define grub_elfXX_check_endianess_and_bswap_ehdr grub_elf64_check_endianess_and_bswap_ehdr
+#define grub_elfXX_get_shnum grub_elf64_get_shnum
 
 #include "elfXX.c"
diff --git a/grub-core/kern/elfXX.c b/grub-core/kern/elfXX.c
index 1859d1880..ed2dc7075 100644
--- a/grub-core/kern/elfXX.c
+++ b/grub-core/kern/elfXX.c
@@ -205,3 +205,37 @@ grub_elfXX_check_endianess_and_bswap_ehdr (grub_elf_t elf)
 
   return 0;
 }
+
+grub_err_t
+grub_elfXX_get_shnum (ElfXX_Ehdr *e, ElfXX_Word *shnum)
+{
+  ElfXX_Shdr *s;
+
+  if (shnum == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("NULL pointer passed for shnum"));
+
+  /* Set *shnum to 0 so that shnum doesn't return junk on error */
+  *shnum = 0;
+
+  if (e == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("NULL pointer passed for elf header"));
+
+  *shnum = e->e_shnum;
+  if (*shnum == SHN_UNDEF)
+    {
+      if (e->e_shoff == 0)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid section header table offset in e_shoff"));
+
+      s = (ElfXX_Shdr *) ((grub_uint8_t *) e + e->e_shoff);
+      *shnum = s->sh_size;
+      if (*shnum < SHN_LORESERVE)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid number of section header table entries in sh_size: %d"), *shnum);
+    }
+  else
+    {
+      if (*shnum >= SHN_LORESERVE)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid number of section header table entries in e_shnum: %d"), *shnum);
+    }
+
+  return GRUB_ERR_NONE;
+}
diff --git a/grub-core/loader/i386/bsdXX.c b/grub-core/loader/i386/bsdXX.c
index 6946cecbb..7d9b65d1a 100644
--- a/grub-core/loader/i386/bsdXX.c
+++ b/grub-core/loader/i386/bsdXX.c
@@ -26,7 +26,10 @@ load (grub_file_t file, const char *filename, void *where, grub_off_t off, grub_
 static inline grub_err_t
 read_headers (grub_file_t file, const char *filename, Elf_Ehdr *e, Elf_Shdr **shdr)
 {
- if (grub_file_seek (file, 0) == (grub_off_t) -1)
+  Elf_Word shnum;
+  grub_err_t err;
+
+  if (grub_file_seek (file, 0) == (grub_off_t) -1)
     return grub_errno;
 
   if (grub_file_read (file, (char *) e, sizeof (*e)) != sizeof (*e))
@@ -48,15 +51,19 @@ read_headers (grub_file_t file, const char *filename, Elf_Ehdr *e, Elf_Shdr **sh
   if (e->e_ident[EI_CLASS] != SUFFIX (ELFCLASS))
     return grub_error (GRUB_ERR_BAD_OS, N_("invalid arch-dependent ELF magic"));
 
-  *shdr = grub_calloc (e->e_shnum, e->e_shentsize);
+  err = grub_elf_get_shnum (e, &shnum);
+  if (err)
+    return err;
+
+  *shdr = grub_calloc (shnum, e->e_shentsize);
   if (! *shdr)
     return grub_errno;
 
   if (grub_file_seek (file, e->e_shoff) == (grub_off_t) -1)
     return grub_errno;
 
-  if (grub_file_read (file, *shdr, (grub_uint32_t) e->e_shnum * e->e_shentsize)
-      != (grub_ssize_t) ((grub_uint32_t) e->e_shnum * e->e_shentsize))
+  if (grub_file_read (file, *shdr, (grub_ssize_t) shnum * e->e_shentsize)
+      != (grub_ssize_t) ((grub_ssize_t) shnum * e->e_shentsize))
     {
       if (grub_errno)
 	return grub_errno;
@@ -79,6 +86,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
   Elf_Ehdr e;
   Elf_Shdr *s;
   Elf_Shdr *shdr = NULL;
+  Elf_Word shnum;
   grub_addr_t curload, module;
   grub_err_t err;
   grub_size_t chunk_size = 0;
@@ -90,7 +98,11 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
   if (err)
     goto out;
 
-  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+  err = grub_elf_get_shnum (&e, &shnum);
+  if (err != GRUB_ERR_NONE)
+    goto out;
+
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
@@ -112,7 +124,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
     chunk_src = get_virtual_current_address (ch);
   }
 
-  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
@@ -155,7 +167,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
   if (! err)
     err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA
 			     | FREEBSD_MODINFOMD_SHDR,
-			     shdr, e.e_shnum * e.e_shentsize);
+			     shdr, shnum * e.e_shentsize);
 
 out:
   grub_free (shdr);
@@ -172,6 +184,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
   Elf_Ehdr e;
   Elf_Shdr *s;
   Elf_Shdr *shdr = NULL;
+  Elf_Word shnum;
   grub_addr_t curload, module;
   grub_err_t err;
   grub_size_t chunk_size = 0;
@@ -183,7 +196,11 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
   if (err)
     goto out;
 
-  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+  err = grub_elf_get_shnum (&e, &shnum);
+  if (err != GRUB_ERR_NONE)
+    goto out;
+
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
@@ -198,7 +215,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
   if (chunk_size < sizeof (e))
     chunk_size = sizeof (e);
   chunk_size += (grub_uint32_t) e.e_phnum * e.e_phentsize;
-  chunk_size += (grub_uint32_t) e.e_shnum * e.e_shentsize;
+  chunk_size += (grub_size_t) shnum * e.e_shentsize;
 
   {
     grub_relocator_chunk_t ch;
@@ -211,7 +228,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
     chunk_src = get_virtual_current_address (ch);
   }
 
-  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       if (s->sh_size == 0)
@@ -249,9 +266,9 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
     curload = module + sizeof (e);
 
   load (file, argv[0], (grub_uint8_t *) chunk_src + curload - *kern_end, e.e_shoff,
-	(grub_uint32_t) e.e_shnum * e.e_shentsize);
+	(grub_size_t) shnum * e.e_shentsize);
   e.e_shoff = curload - module;
-  curload +=  (grub_uint32_t) e.e_shnum * e.e_shentsize;
+  curload +=  (grub_addr_t) shnum * e.e_shentsize;
 
   load (file, argv[0], (grub_uint8_t *) chunk_src + curload - *kern_end, e.e_phoff,
 	(grub_uint32_t) e.e_phnum * e.e_phentsize);
@@ -282,6 +299,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct grub_relocator *relocator,
   Elf_Ehdr e;
   Elf_Shdr *s;
   Elf_Shdr *shdr = NULL;
+  Elf_Word shnum;
   unsigned symoff, stroff, symsize, strsize;
   grub_freebsd_addr_t symstart, symend, symentsize, dynamic;
   Elf_Sym *sym;
@@ -296,17 +314,21 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct grub_relocator *relocator,
   if (err)
     goto out;
 
+  err = grub_elf_get_shnum (&e, &shnum);
+  if (err != GRUB_ERR_NONE)
+    goto out;
+
   err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA |
 			   FREEBSD_MODINFOMD_ELFHDR, &e,
 			   sizeof (e));
   if (err)
     goto out;
 
-  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
       if (s->sh_type == SHT_SYMTAB)
 	break;
-  if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize))
+  if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize))
     {
       err = grub_error (GRUB_ERR_BAD_OS, N_("no symbol table"));
       goto out;
@@ -422,6 +444,7 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator *relocator,
   Elf_Ehdr e;
   Elf_Shdr *s, *symsh, *strsh;
   Elf_Shdr *shdr = NULL;
+  Elf_Word shnum;
   unsigned symsize, strsize;
   void *sym_chunk;
   grub_uint8_t *curload;
@@ -437,11 +460,18 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator *relocator,
       return grub_errno;
     }
 
-  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+  err = grub_elf_get_shnum (&e, &shnum);
+  if (err != GRUB_ERR_NONE)
+    {
+      grub_free (shdr);
+      return err;
+    }
+
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
       if (s->sh_type == SHT_SYMTAB)
 	break;
-  if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize))
+  if (s >= (Elf_Shdr *) ((char *) shdr + shnum * e.e_shentsize))
     {
       grub_free (shdr);
       return GRUB_ERR_NONE;
@@ -454,7 +484,7 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator *relocator,
 
   chunk_size = ALIGN_UP (symsize, sizeof (grub_freebsd_addr_t))
     + ALIGN_UP (strsize, sizeof (grub_freebsd_addr_t))
-    + sizeof (e) + (grub_uint32_t) e.e_shnum * e.e_shentsize;
+    + sizeof (e) + (grub_uint32_t) shnum * e.e_shentsize;
 
   symtarget = ALIGN_UP (*kern_end, sizeof (grub_freebsd_addr_t));
   {
@@ -482,17 +512,17 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator *relocator,
 
   curload += sizeof (e);
 
-  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+  for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
       Elf_Shdr *s2;
       s2 = (Elf_Shdr *) curload;
       grub_memcpy (curload, s, e.e_shentsize);
       if (s == symsh)
-	s2->sh_offset = sizeof (e) + (grub_uint32_t) e.e_shnum * e.e_shentsize;
+	s2->sh_offset = sizeof (e) + (grub_uint32_t) shnum * e.e_shentsize;
       else if (s == strsh)
 	s2->sh_offset = ALIGN_UP (symsize, sizeof (grub_freebsd_addr_t))
-	  + sizeof (e) + (grub_uint32_t) e.e_shnum * e.e_shentsize;
+	  + sizeof (e) + (grub_uint32_t) shnum * e.e_shentsize;
       else
 	s2->sh_offset = 0;
       s2->sh_addr = s2->sh_offset;
@@ -557,6 +587,7 @@ SUFFIX(grub_openbsd_find_ramdisk) (grub_file_t file,
     Elf_Ehdr e;
     Elf_Shdr *s;
     Elf_Shdr *shdr = NULL;
+    Elf_Word shnum;
 
     err = read_headers (file, filename, &e, &shdr);
     if (err)
@@ -565,11 +596,18 @@ SUFFIX(grub_openbsd_find_ramdisk) (grub_file_t file,
 	return err;
       }
 
-    for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize);
+    err = grub_elf_get_shnum (&e, &shnum);
+    if (err != GRUB_ERR_NONE)
+      {
+	grub_free (shdr);
+	return err;
+      }
+
+    for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
 	 s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
       if (s->sh_type == SHT_SYMTAB)
 	break;
-    if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + e.e_shnum * e.e_shentsize))
+    if (s >= (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize))
       {
 	grub_free (shdr);
 	return GRUB_ERR_NONE;
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 6801f0ddf..1fe9be91b 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -17,19 +17,23 @@
  */
 
 #if defined(MULTIBOOT_LOAD_ELF32)
-# define XX		32
-# define E_MACHINE	MULTIBOOT_ELF32_MACHINE
-# define ELFCLASSXX	ELFCLASS32
-# define Elf_Ehdr	Elf32_Ehdr
-# define Elf_Phdr	Elf32_Phdr
-# define Elf_Shdr	Elf32_Shdr
+# define XX			32
+# define E_MACHINE		MULTIBOOT_ELF32_MACHINE
+# define ELFCLASSXX		ELFCLASS32
+# define Elf_Ehdr		Elf32_Ehdr
+# define Elf_Phdr		Elf32_Phdr
+# define Elf_Shdr		Elf32_Shdr
+# define Elf_Word		Elf32_Word
+# define grub_elf_get_shnum	grub_elf32_get_shnum
 #elif defined(MULTIBOOT_LOAD_ELF64)
-# define XX		64
-# define E_MACHINE	MULTIBOOT_ELF64_MACHINE
-# define ELFCLASSXX	ELFCLASS64
-# define Elf_Ehdr	Elf64_Ehdr
-# define Elf_Phdr	Elf64_Phdr
-# define Elf_Shdr	Elf64_Shdr
+# define XX			64
+# define E_MACHINE		MULTIBOOT_ELF64_MACHINE
+# define ELFCLASSXX		ELFCLASS64
+# define Elf_Ehdr		Elf64_Ehdr
+# define Elf_Phdr		Elf64_Phdr
+# define Elf_Shdr		Elf64_Shdr
+# define Elf_Word		Elf64_Word
+# define grub_elf_get_shnum	grub_elf64_get_shnum
 #else
 #error "I'm confused"
 #endif
@@ -58,7 +62,8 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
   grub_err_t err;
   grub_relocator_chunk_t ch;
   grub_uint32_t load_offset = 0, load_size;
-  int i;
+  Elf_Word shnum;
+  unsigned int i;
   void *source = NULL;
 
   if (ehdr->e_ident[EI_MAG0] != ELFMAG0
@@ -75,6 +80,10 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
   if (ehdr->e_type != ET_EXEC && ehdr->e_type != ET_DYN)
     return grub_error (GRUB_ERR_UNKNOWN_OS, N_("this ELF file is not of the right type"));
 
+  err = grub_elf_get_shnum (ehdr, &shnum);
+  if (err != GRUB_ERR_NONE)
+    return err;
+
   /* FIXME: Should we support program headers at strange locations?  */
   if (ehdr->e_phoff + (grub_uint32_t) ehdr->e_phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
     return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset");
@@ -213,11 +222,11 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 #error Please complete this
 #endif
 
-  if (ehdr->e_shnum)
+  if (shnum)
     {
       grub_uint8_t *shdr, *shdrptr;
 
-      shdr = grub_calloc (ehdr->e_shnum, ehdr->e_shentsize);
+      shdr = grub_calloc (shnum, ehdr->e_shentsize);
       if (!shdr)
 	return grub_errno;
 
@@ -227,8 +236,8 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 	  return grub_errno;
 	}
 
-      if (grub_file_read (mld->file, shdr, (grub_uint32_t) ehdr->e_shnum * ehdr->e_shentsize)
-              != (grub_ssize_t) ehdr->e_shnum * ehdr->e_shentsize)
+      if (grub_file_read (mld->file, shdr, (grub_ssize_t) shnum * ehdr->e_shentsize)
+              != (grub_ssize_t) shnum * ehdr->e_shentsize)
 	{
 	  if (!grub_errno)
 	    grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file %s"),
@@ -236,7 +245,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 	  return grub_errno;
 	}
 
-      for (shdrptr = shdr, i = 0; i < ehdr->e_shnum;
+      for (shdrptr = shdr, i = 0; i < shnum;
 	   shdrptr += ehdr->e_shentsize, i++)
 	{
 	  Elf_Shdr *sh = (Elf_Shdr *) shdrptr;
@@ -281,7 +290,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 	    }
 	  sh->sh_addr = target;
 	}
-      GRUB_MULTIBOOT (add_elfsyms) (ehdr->e_shnum, ehdr->e_shentsize,
+      GRUB_MULTIBOOT (add_elfsyms) (shnum, ehdr->e_shentsize,
 				    ehdr->e_shstrndx, shdr);
     }
 
@@ -296,3 +305,5 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 #undef Elf_Ehdr
 #undef Elf_Phdr
 #undef Elf_Shdr
+#undef Elf_Word
+#undef grub_elf_get_shnum
diff --git a/include/grub/elf.h b/include/grub/elf.h
index c478933ee..229efd349 100644
--- a/include/grub/elf.h
+++ b/include/grub/elf.h
@@ -23,6 +23,7 @@
 /* Standard ELF types.  */
 
 #include <grub/types.h>
+#include <grub/err.h>
 
 /* Type for a 16-bit quantity.  */
 typedef grub_uint16_t Elf32_Half;
@@ -2531,6 +2532,10 @@ typedef Elf32_Addr Elf32_Conflict;
 #define R_RISCV_SET32           56
 #define R_RISCV_32_PCREL        57
 
+extern grub_err_t grub_elf32_get_shnum (Elf32_Ehdr *e, Elf32_Word *shnum);
+
+extern grub_err_t grub_elf64_get_shnum (Elf64_Ehdr *e, Elf64_Word *shnum);
+
 #ifdef GRUB_TARGET_WORDSIZE
 #if GRUB_TARGET_WORDSIZE == 32
 
@@ -2557,6 +2562,8 @@ typedef Elf32_Xword Elf_Xword;
 #define ELF_R_TYPE(val)		ELF32_R_TYPE(val)
 #define ELF_R_INFO(sym, type)	ELF32_R_INFO(sym, type)
 
+#define grub_elf_get_shnum	grub_elf32_get_shnum
+
 #elif GRUB_TARGET_WORDSIZE == 64
 
 typedef Elf64_Addr Elf_Addr;
@@ -2581,6 +2588,8 @@ typedef Elf64_Xword Elf_Xword;
 #define ELF_R_TYPE(val)		ELF64_R_TYPE(val)
 #define ELF_R_INFO(sym, type)	ELF64_R_INFO(sym, type)
 
+#define grub_elf_get_shnum	grub_elf64_get_shnum
+
 #endif /* GRUB_TARGET_WORDSIZE == 64 */
 #endif
 
-- 
2.27.0

[PATCH v3 3/5] elf: Validate elf section header table index for section name string table

[Alec Brown]

In multiboot_elfXX.c, e_shstrndx is used to obtain the section header table
index of the section name string table, but it wasn't being checked if the value
was there.

According to the elf(5) manual page,
"If the index of section name string table section is larger than or equal to
SHN_LORESERVE (0xff00), this member holds SHN_XINDEX (0xffff) and the real
index of the section name string table section is held in the sh_link member of
the initial entry in section header table. Otherwise, the sh_link member of the
initial entry in section header table contains the value zero."

Since this check wasn't being made, grub_elfXX_get_shstrndx() is being added to
elfXX.c to make this check and use e_shstrndx if it doesn't have SHN_XINDEX as a
value, else use sh_link. We also need to make sure e_shstrndx isn't greater than
or equal to SHN_LORESERVE and sh_link isn't less than SHN_LORESERVE.

Note that even though elf.c and elfXX.c are located in grub-core/kern, they are
compiled as modules and don't need the EXPORT_FUNC macro to define the functions
in elf.h.

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/kern/elf.c               |  3 +++
 grub-core/kern/elfXX.c             | 33 ++++++++++++++++++++++++++++++
 grub-core/loader/multiboot_elfxx.c | 10 ++++++++-
 include/grub/elf.h                 |  4 ++++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/elf.c b/grub-core/kern/elf.c
index 5be770583..19440a318 100644
--- a/grub-core/kern/elf.c
+++ b/grub-core/kern/elf.c
@@ -175,6 +175,7 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #define grub_swap_bytes_XwordXX grub_swap_bytes32
 #define grub_elfXX_check_endianess_and_bswap_ehdr grub_elf32_check_endianess_and_bswap_ehdr
 #define grub_elfXX_get_shnum grub_elf32_get_shnum
+#define grub_elfXX_get_shstrndx grub_elf32_get_shstrndx
 
 #include "elfXX.c"
 
@@ -196,6 +197,7 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #undef grub_swap_bytes_XwordXX
 #undef grub_elfXX_check_endianess_and_bswap_ehdr
 #undef grub_elfXX_get_shnum
+#undef grub_elfXX_get_shstrndx
 
 \f
 /* 64-bit */
@@ -217,5 +219,6 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #define grub_swap_bytes_XwordXX grub_swap_bytes64
 #define grub_elfXX_check_endianess_and_bswap_ehdr grub_elf64_check_endianess_and_bswap_ehdr
 #define grub_elfXX_get_shnum grub_elf64_get_shnum
+#define grub_elfXX_get_shstrndx grub_elf64_get_shstrndx
 
 #include "elfXX.c"
diff --git a/grub-core/kern/elfXX.c b/grub-core/kern/elfXX.c
index ed2dc7075..d09b37213 100644
--- a/grub-core/kern/elfXX.c
+++ b/grub-core/kern/elfXX.c
@@ -239,3 +239,36 @@ grub_elfXX_get_shnum (ElfXX_Ehdr *e, ElfXX_Word *shnum)
 
   return GRUB_ERR_NONE;
 }
+
+grub_err_t
+grub_elfXX_get_shstrndx (ElfXX_Ehdr *e, grub_uint32_t *shstrndx)
+{
+  ElfXX_Shdr *s;
+
+  if (shstrndx == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("NULL pointer passed for shstrndx"));
+
+  /* Set *shstrndx to 0 so that shstrndx doesn't return junk on error */
+  *shstrndx = 0;
+
+  if (e == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("NULL pointer passed for elf header"));
+
+  *shstrndx = e->e_shstrndx;
+  if (*shstrndx == SHN_XINDEX)
+    {
+      if (e->e_shoff == 0)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid section header table offset in e_shoff"));
+
+      s = (ElfXX_Shdr *) ((grub_uint8_t *) e + e->e_shoff);
+      *shstrndx = s->sh_link;
+      if (*shstrndx < SHN_LORESERVE)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid section header table index in sh_link: %d"), *shstrndx);
+    }
+  else
+    {
+      if (*shstrndx >= SHN_LORESERVE)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid section header table index in e_shstrndx: %d"), *shstrndx);
+    }
+  return GRUB_ERR_NONE;
+}
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 1fe9be91b..54aaa24aa 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -25,6 +25,7 @@
 # define Elf_Shdr		Elf32_Shdr
 # define Elf_Word		Elf32_Word
 # define grub_elf_get_shnum	grub_elf32_get_shnum
+# define grub_elf_get_shstrndx	grub_elf32_get_shstrndx
 #elif defined(MULTIBOOT_LOAD_ELF64)
 # define XX			64
 # define E_MACHINE		MULTIBOOT_ELF64_MACHINE
@@ -34,6 +35,7 @@
 # define Elf_Shdr		Elf64_Shdr
 # define Elf_Word		Elf64_Word
 # define grub_elf_get_shnum	grub_elf64_get_shnum
+# define grub_elf_get_shstrndx	grub_elf64_get_shstrndx
 #else
 #error "I'm confused"
 #endif
@@ -62,6 +64,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
   grub_err_t err;
   grub_relocator_chunk_t ch;
   grub_uint32_t load_offset = 0, load_size;
+  grub_uint32_t shstrndx;
   Elf_Word shnum;
   unsigned int i;
   void *source = NULL;
@@ -84,6 +87,10 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
   if (err != GRUB_ERR_NONE)
     return err;
 
+  err = grub_elf_get_shstrndx (ehdr, &shstrndx);
+  if (err != GRUB_ERR_NONE)
+    return err;
+
   /* FIXME: Should we support program headers at strange locations?  */
   if (ehdr->e_phoff + (grub_uint32_t) ehdr->e_phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
     return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset");
@@ -291,7 +298,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 	  sh->sh_addr = target;
 	}
       GRUB_MULTIBOOT (add_elfsyms) (shnum, ehdr->e_shentsize,
-				    ehdr->e_shstrndx, shdr);
+				    shstrndx, shdr);
     }
 
 #undef phdr
@@ -307,3 +314,4 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 #undef Elf_Shdr
 #undef Elf_Word
 #undef grub_elf_get_shnum
+#undef grub_elf_get_shstrndx
diff --git a/include/grub/elf.h b/include/grub/elf.h
index 229efd349..9c94fd6d3 100644
--- a/include/grub/elf.h
+++ b/include/grub/elf.h
@@ -2533,8 +2533,10 @@ typedef Elf32_Addr Elf32_Conflict;
 #define R_RISCV_32_PCREL        57
 
 extern grub_err_t grub_elf32_get_shnum (Elf32_Ehdr *e, Elf32_Word *shnum);
+extern grub_err_t grub_elf32_get_shstrndx (Elf32_Ehdr *e, grub_uint32_t *shstrndx);
 
 extern grub_err_t grub_elf64_get_shnum (Elf64_Ehdr *e, Elf64_Word *shnum);
+extern grub_err_t grub_elf64_get_shstrndx (Elf64_Ehdr *e, grub_uint32_t *shstrndx);
 
 #ifdef GRUB_TARGET_WORDSIZE
 #if GRUB_TARGET_WORDSIZE == 32
@@ -2563,6 +2565,7 @@ typedef Elf32_Xword Elf_Xword;
 #define ELF_R_INFO(sym, type)	ELF32_R_INFO(sym, type)
 
 #define grub_elf_get_shnum	grub_elf32_get_shnum
+#define grub_elf_get_shstrndx	grub_elf32_get_shstrndx
 
 #elif GRUB_TARGET_WORDSIZE == 64
 
@@ -2589,6 +2592,7 @@ typedef Elf64_Xword Elf_Xword;
 #define ELF_R_INFO(sym, type)	ELF64_R_INFO(sym, type)
 
 #define grub_elf_get_shnum	grub_elf64_get_shnum
+#define grub_elf_get_shstrndx	grub_elf64_get_shstrndx
 
 #endif /* GRUB_TARGET_WORDSIZE == 64 */
 #endif
-- 
2.27.0

[PATCH v3 4/5] elf: Validate number of elf program header table entries

[Alec Brown]

In bsdXX.c and multiboot_elfXX.c, e_phnum is used to obtain the number of
program header table entries, but it wasn't being checked if the value was
there.

According to the elf(5) manual page,
"If the number of entries in the program header table is larger than or equal to
PN_XNUM (0xffff), this member holds PN_XNUM (0xffff) and the real number of
entries in the program header table is held in the sh_info member of the
initial entry in section header table.  Otherwise, the sh_info member of the
initial entry contains the value zero."

Since this check wasn't being made, grub_elfXX_get_phnum() is being added to
elfXX.c to make this check and use e_phnum if it doesn't have PN_XNUM as a
value, else use sh_info. We also need to make sure e_phnum isn't greater than
PN_XNUM and sh_info isn't less than PN_XNUM.

Note that even though elf.c and elfXX.c are located in grub-core/kern, they are
compiled as modules and don't need the EXPORT_FUNC macro to define the functions
in elf.h.

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/kern/elf.c               |  3 +++
 grub-core/kern/elfXX.c             | 34 ++++++++++++++++++++++++++++++
 grub-core/loader/i386/bsdXX.c      | 11 +++++++---
 grub-core/loader/multiboot_elfxx.c | 19 +++++++++++------
 include/grub/elf.h                 |  5 +++++
 5 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/grub-core/kern/elf.c b/grub-core/kern/elf.c
index 19440a318..ff5655206 100644
--- a/grub-core/kern/elf.c
+++ b/grub-core/kern/elf.c
@@ -176,6 +176,7 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #define grub_elfXX_check_endianess_and_bswap_ehdr grub_elf32_check_endianess_and_bswap_ehdr
 #define grub_elfXX_get_shnum grub_elf32_get_shnum
 #define grub_elfXX_get_shstrndx grub_elf32_get_shstrndx
+#define grub_elfXX_get_phnum grub_elf32_get_phnum
 
 #include "elfXX.c"
 
@@ -198,6 +199,7 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #undef grub_elfXX_check_endianess_and_bswap_ehdr
 #undef grub_elfXX_get_shnum
 #undef grub_elfXX_get_shstrndx
+#undef grub_elfXX_get_phnum
 
 \f
 /* 64-bit */
@@ -220,5 +222,6 @@ grub_elf_open (const char *name, enum grub_file_type type)
 #define grub_elfXX_check_endianess_and_bswap_ehdr grub_elf64_check_endianess_and_bswap_ehdr
 #define grub_elfXX_get_shnum grub_elf64_get_shnum
 #define grub_elfXX_get_shstrndx grub_elf64_get_shstrndx
+#define grub_elfXX_get_phnum grub_elf64_get_phnum
 
 #include "elfXX.c"
diff --git a/grub-core/kern/elfXX.c b/grub-core/kern/elfXX.c
index d09b37213..a917e7cce 100644
--- a/grub-core/kern/elfXX.c
+++ b/grub-core/kern/elfXX.c
@@ -272,3 +272,37 @@ grub_elfXX_get_shstrndx (ElfXX_Ehdr *e, grub_uint32_t *shstrndx)
     }
   return GRUB_ERR_NONE;
 }
+
+grub_err_t
+grub_elfXX_get_phnum (ElfXX_Ehdr *e, grub_uint32_t *phnum)
+{
+  ElfXX_Shdr *s;
+
+  if (phnum == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("NULL pointer passed for phnum"));
+
+  /* Set *phnum to 0 so that phnum doesn't return junk on error */
+  *phnum = 0;
+
+  if (e == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("NULL pointer passed for elf header"));
+
+  *phnum = e->e_phnum;
+  if (*phnum == PN_XNUM)
+    {
+      if (e->e_shoff == 0)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid section header table offset in e_shoff"));
+
+      s = (ElfXX_Shdr *) ((grub_uint8_t *) e + e->e_shoff);
+      *phnum = s->sh_info;
+      if (*phnum < PN_XNUM)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid number of program header table entries in sh_info: %d"), *phnum);
+    }
+  else
+    {
+      if (*phnum >= PN_XNUM)
+	return grub_error (GRUB_ERR_BAD_NUMBER, N_("invalid number of program header table entries in e_phnum: %d"), *phnum);
+    }
+
+  return GRUB_ERR_NONE;
+}
diff --git a/grub-core/loader/i386/bsdXX.c b/grub-core/loader/i386/bsdXX.c
index 7d9b65d1a..eb1164dd7 100644
--- a/grub-core/loader/i386/bsdXX.c
+++ b/grub-core/loader/i386/bsdXX.c
@@ -185,6 +185,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
   Elf_Shdr *s;
   Elf_Shdr *shdr = NULL;
   Elf_Word shnum;
+  grub_uint32_t phnum;
   grub_addr_t curload, module;
   grub_err_t err;
   grub_size_t chunk_size = 0;
@@ -200,6 +201,10 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
   if (err != GRUB_ERR_NONE)
     goto out;
 
+  err = grub_elf_get_phnum (&e, &phnum);
+  if (err != GRUB_ERR_NONE)
+    goto out;
+
   for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * e.e_shentsize);
        s = (Elf_Shdr *) ((grub_uint8_t *) s + e.e_shentsize))
     {
@@ -214,7 +219,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
 
   if (chunk_size < sizeof (e))
     chunk_size = sizeof (e);
-  chunk_size += (grub_uint32_t) e.e_phnum * e.e_phentsize;
+  chunk_size += (grub_size_t) phnum * e.e_phentsize;
   chunk_size += (grub_size_t) shnum * e.e_shentsize;
 
   {
@@ -271,9 +276,9 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
   curload +=  (grub_addr_t) shnum * e.e_shentsize;
 
   load (file, argv[0], (grub_uint8_t *) chunk_src + curload - *kern_end, e.e_phoff,
-	(grub_uint32_t) e.e_phnum * e.e_phentsize);
+	(grub_size_t) phnum * e.e_phentsize);
   e.e_phoff = curload - module;
-  curload +=  (grub_uint32_t) e.e_phnum * e.e_phentsize;
+  curload +=  (grub_addr_t) phnum * e.e_phentsize;
 
   *kern_end = curload;
 
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 54aaa24aa..87eb9639a 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -26,6 +26,7 @@
 # define Elf_Word		Elf32_Word
 # define grub_elf_get_shnum	grub_elf32_get_shnum
 # define grub_elf_get_shstrndx	grub_elf32_get_shstrndx
+# define grub_elf_get_phnum	grub_elf32_get_phnum
 #elif defined(MULTIBOOT_LOAD_ELF64)
 # define XX			64
 # define E_MACHINE		MULTIBOOT_ELF64_MACHINE
@@ -36,6 +37,7 @@
 # define Elf_Word		Elf64_Word
 # define grub_elf_get_shnum	grub_elf64_get_shnum
 # define grub_elf_get_shstrndx	grub_elf64_get_shstrndx
+# define grub_elf_get_phnum	grub_elf64_get_phnum
 #else
 #error "I'm confused"
 #endif
@@ -64,7 +66,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
   grub_err_t err;
   grub_relocator_chunk_t ch;
   grub_uint32_t load_offset = 0, load_size;
-  grub_uint32_t shstrndx;
+  grub_uint32_t shstrndx, phnum;
   Elf_Word shnum;
   unsigned int i;
   void *source = NULL;
@@ -91,8 +93,12 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
   if (err != GRUB_ERR_NONE)
     return err;
 
+  err = grub_elf_get_phnum (ehdr, &phnum);
+  if (err != GRUB_ERR_NONE)
+    return err;
+
   /* FIXME: Should we support program headers at strange locations?  */
-  if (ehdr->e_phoff + (grub_uint32_t) ehdr->e_phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
+  if (ehdr->e_phoff + phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
     return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset");
 
   phdr_base = (char *) mld->buffer + ehdr->e_phoff;
@@ -101,7 +107,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
   mld->link_base_addr = ~0;
 
   /* Calculate lowest and highest load address.  */
-  for (i = 0; i < ehdr->e_phnum; i++)
+  for (i = 0; i < phnum; i++)
     if (phdr(i)->p_type == PT_LOAD)
       {
 	mld->link_base_addr = grub_min (mld->link_base_addr, phdr(i)->p_paddr);
@@ -147,7 +153,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 		mld->link_base_addr, mld->load_base_addr);
 
   /* Load every loadable segment in memory.  */
-  for (i = 0; i < ehdr->e_phnum; i++)
+  for (i = 0; i < phnum; i++)
     {
       if (phdr(i)->p_type == PT_LOAD)
         {
@@ -196,7 +202,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
         }
     }
 
-  for (i = 0; i < ehdr->e_phnum; i++)
+  for (i = 0; i < phnum; i++)
     if (phdr(i)->p_vaddr <= ehdr->e_entry
 	&& phdr(i)->p_vaddr + phdr(i)->p_memsz > ehdr->e_entry)
       {
@@ -218,7 +224,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 	break;
       }
 
-  if (i == ehdr->e_phnum)
+  if (i == phnum)
     return grub_error (GRUB_ERR_BAD_OS, "entry point isn't in a segment");
 
 #if defined (__i386__) || defined (__x86_64__)
@@ -315,3 +321,4 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 #undef Elf_Word
 #undef grub_elf_get_shnum
 #undef grub_elf_get_shstrndx
+#undef grub_elf_get_phnum
diff --git a/include/grub/elf.h b/include/grub/elf.h
index 9c94fd6d3..80dbd577b 100644
--- a/include/grub/elf.h
+++ b/include/grub/elf.h
@@ -567,6 +567,7 @@ typedef struct
 #define PT_HIOS		0x6fffffff	/* End of OS-specific */
 #define PT_LOPROC	0x70000000	/* Start of processor-specific */
 #define PT_HIPROC	0x7fffffff	/* End of processor-specific */
+#define PN_XNUM		0xffff
 
 /* Legal values for p_flags (segment flags).  */
 
@@ -2534,9 +2535,11 @@ typedef Elf32_Addr Elf32_Conflict;
 
 extern grub_err_t grub_elf32_get_shnum (Elf32_Ehdr *e, Elf32_Word *shnum);
 extern grub_err_t grub_elf32_get_shstrndx (Elf32_Ehdr *e, grub_uint32_t *shstrndx);
+extern grub_err_t grub_elf32_get_phnum (Elf32_Ehdr *e, grub_uint32_t *phnum);
 
 extern grub_err_t grub_elf64_get_shnum (Elf64_Ehdr *e, Elf64_Word *shnum);
 extern grub_err_t grub_elf64_get_shstrndx (Elf64_Ehdr *e, grub_uint32_t *shstrndx);
+extern grub_err_t grub_elf64_get_phnum (Elf64_Ehdr *e, Elf64_Word *phnum);
 
 #ifdef GRUB_TARGET_WORDSIZE
 #if GRUB_TARGET_WORDSIZE == 32
@@ -2566,6 +2569,7 @@ typedef Elf32_Xword Elf_Xword;
 
 #define grub_elf_get_shnum	grub_elf32_get_shnum
 #define grub_elf_get_shstrndx	grub_elf32_get_shstrndx
+#define grub_elf_get_phnum	grub_elf32_get_phnum
 
 #elif GRUB_TARGET_WORDSIZE == 64
 
@@ -2593,6 +2597,7 @@ typedef Elf64_Xword Elf_Xword;
 
 #define grub_elf_get_shnum	grub_elf64_get_shnum
 #define grub_elf_get_shstrndx	grub_elf64_get_shstrndx
+#define grub_elf_get_phnum	grub_elf64_get_phnum
 
 #endif /* GRUB_TARGET_WORDSIZE == 64 */
 #endif
-- 
2.27.0

[PATCH v3 5/5] util/grub-module-verifierXX.c: Add e_shoff check in get_shdr()

[Alec Brown]

In util/grub-module-verifierXX.c, the function get_shdr() is used to obtain the
section header at a given index but isn't checking that there is an offset for
the section header table. To validate that there is, we can check that e_shoff
isn't 0.

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 util/grub-module-verifierXX.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util/grub-module-verifierXX.c b/util/grub-module-verifierXX.c
index 4e6cf133f..cf3ff0dfa 100644
--- a/util/grub-module-verifierXX.c
+++ b/util/grub-module-verifierXX.c
@@ -134,6 +134,9 @@ grub_target_to_host_real (const struct grub_module_verifier_arch *arch, grub_uin
 static Elf_Shdr *
 get_shdr (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word index)
 {
+  if (grub_target_to_host (e->e_shoff) == 0)
+    grub_util_error ("Invalid section header offset");
+
   return (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff) +
 		       index * grub_target_to_host16 (e->e_shentsize));
 }
-- 
2.27.0

Re: [PATCH v3 0/5] Fix coverity bugs and add checks for elf values in grub-core

[Darren Kenny]

Hi Alec,

This all looks good to me, so for the series:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

On Wednesday, 2022-04-20 at 22:23:12 -04, Alec Brown wrote:
> v3: Added check for e_shoff, made starting words lowercase in error messages,
> and added comment to why return pointers are set to 0.
>
> Coverity identified several untrusted loop bounds and untrusted allocation size
> bugs in grub-core/loader/i386/bsdXX.c and grub-core/loader/multiboot_elfXX.c.
> Upon review of these bugs, I found that specific checks weren't being made to
> various elf header values based on the elf manual page. This patch series
> addresses the coverity bugs, as well as adds functions to check for the correct
> elf header values.
>
> The Coverity bugs being addressed are:
> CID 314018
> CID 314030
> CID 314031
> CID 314039
>
> Alec Brown (5):
>       grub-core/loader/i386/bsdXX.c: Avoid downcasting (char *) to (Elf_Shdr *)
>       elf: Validate number of elf section header table entries
>       elf: Validate elf section header table index for section name string table
>       elf: Validate number of elf program header table entries
>       util/grub-module-verifierXX.c: Add e_shoff check in get_shdr()
>
>  grub-core/kern/elf.c               |  15 +++++++++++++++
>  grub-core/kern/elfXX.c             | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  grub-core/loader/i386/bsdXX.c      | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------
>  grub-core/loader/multiboot_elfxx.c |  76 +++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------
>  include/grub/elf.h                 |  18 ++++++++++++++++++
>  util/grub-module-verifierXX.c      |   3 +++
>  6 files changed, 273 insertions(+), 77 deletions(-)