[PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c

[PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c

[Alec Brown]

Coverity identified several untrusted loop bounds in
util/grub-module-verifierXX.c. This patch series addresses these bugs, cleans up
lengthy equations, and makes checks to values based on the elf manual page.

The Coverity Bugs being addressed are:
CID 314021
CID 314027
CID 314033

Alec Brown (4):
      util/grub-module-verifierXX.c: Add function to calculate section headers
      util/grub-module-verifierXX.c: Validate number of elf section header table entries
      util/grub-module-verifierXX.c: Validate elf section header table index for section name string table
      util/grub-module-verifierXX.c: Add module_size parameter to functions for sanity checking

 util/grub-module-verifierXX.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 93 insertions(+), 31 deletions(-)

[PATCH 1/4] util/grub-module-verifierXX.c: Add function to calculate section headers

[Alec Brown]

Added the function get_shdr() which returns the section header at a given index
parameter passed into this function. This helps traverse the section header
table and reduces repeated calls to lengthy equations used to obtain section
headers.

Note that it may look as though the argument *arch isn't being used, it's
actually required in order to use the macros grub_target_to_host*(x) which are
unwinded to grub_target_to_host*_real(arch, (x)) based on defines earlier in the
file.

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

diff --git a/util/grub-module-verifierXX.c b/util/grub-module-verifierXX.c
index ceb24309a..92f9ae2a4 100644
--- a/util/grub-module-verifierXX.c
+++ b/util/grub-module-verifierXX.c
@@ -131,6 +131,12 @@ grub_target_to_host_real (const struct grub_module_verifier_arch *arch, grub_uin
     return grub_target_to_host32_real (arch, in);
 }
 
+static Elf_Shdr *
+get_shdr (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word index)
+{
+  return (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff) +
+	 	       index * grub_target_to_host16 (e->e_shentsize));
+}
 
 static Elf_Shdr *
 find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const char *name)
@@ -139,12 +145,12 @@ find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const c
   const char *str;
   unsigned i;
 
-  s = (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff) + grub_target_to_host16 (e->e_shstrndx) * grub_target_to_host16 (e->e_shentsize));
+  s = get_shdr (arch, e, grub_target_to_host16 (e->e_shstrndx));
   str = (char *) e + grub_target_to_host (s->sh_offset);
 
-  for (i = 0, s = (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff));
+  for (i = 0, s = get_shdr (arch, e, 0);
        i < grub_target_to_host16 (e->e_shnum);
-       i++, s = (Elf_Shdr *) ((char *) s + grub_target_to_host16 (e->e_shentsize)))
+       i++, s = get_shdr (arch, e, i))
     if (strcmp (str + grub_target_to_host32 (s->sh_name), name) == 0)
       return s;
   return NULL;
@@ -166,13 +172,12 @@ static Elf_Sym *
 get_symtab (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word *size, Elf_Word *entsize)
 {
   unsigned i;
-  Elf_Shdr *s, *sections;
+  Elf_Shdr *s;
   Elf_Sym *sym;
 
-  sections = (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff));
-  for (i = 0, s = sections;
+  for (i = 0, s = get_shdr (arch, e, 0);
        i < grub_target_to_host16 (e->e_shnum);
-       i++, s = (Elf_Shdr *) ((char *) s + grub_target_to_host16 (e->e_shentsize)))
+       i++, s = get_shdr (arch, e, i))
     if (grub_target_to_host32 (s->sh_type) == SHT_SYMTAB)
       break;
 
@@ -364,9 +369,9 @@ check_relocations (const char * const modname,
   Elf_Shdr *s;
   unsigned i;
 
-  for (i = 0, s = (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff));
+  for (i = 0, s = get_shdr (arch, e, 0);
        i < grub_target_to_host16 (e->e_shnum);
-       i++, s = (Elf_Shdr *) ((char *) s + grub_target_to_host16 (e->e_shentsize)))
+       i++, s = get_shdr (arch, e, i))
     if (grub_target_to_host32 (s->sh_type) == SHT_REL || grub_target_to_host32 (s->sh_type) == SHT_RELA)
       {
 	Elf_Shdr *ts;
@@ -379,7 +384,7 @@ check_relocations (const char * const modname,
 	/* Find the target segment.  */
 	if (grub_target_to_host32 (s->sh_info) >= grub_target_to_host16 (e->e_shnum))
 	  grub_util_error ("%s: orphaned reloc section", modname);
-	ts = (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff) + grub_target_to_host32 (s->sh_info) * grub_target_to_host16 (e->e_shentsize));
+	ts = get_shdr (arch, e, grub_target_to_host32 (s->sh_info));
 
 	section_check_relocations (modname, arch, e, s, grub_target_to_host (ts->sh_size));
       }
-- 
2.27.0

[PATCH 2/4] util/grub-module-verifierXX.c: Validate number of elf section header table entries

[Alec Brown]

In grub-module-verifierXX.c, grub_target_to_host16 (e->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, the function get_shnum() is being added 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 it may look as though the argument *arch isn't being used, it's
actually required in order to use the macros grub_target_to_host*(x) which are
unwinded to grub_target_to_host*_real(arch, (x)) based on defines earlier in the
file.

Fixes: CID 314021
Fixes: CID 314027
Fixes: CID 314033

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

diff --git a/util/grub-module-verifierXX.c b/util/grub-module-verifierXX.c
index 92f9ae2a4..61b82141f 100644
--- a/util/grub-module-verifierXX.c
+++ b/util/grub-module-verifierXX.c
@@ -138,6 +138,29 @@ get_shdr (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word in
 	 	       index * grub_target_to_host16 (e->e_shentsize));
 }
 
+static Elf_Word
+get_shnum (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
+{
+  Elf_Shdr *s;
+  Elf_Word shnum;
+
+  shnum = grub_target_to_host16 (e->e_shnum);
+  if (shnum == SHN_UNDEF)
+    {
+      s = get_shdr (arch, e, 0);
+      shnum = grub_target_to_host (s->sh_size);
+      if (shnum < SHN_LORESERVE)
+	grub_util_error ("Invalid number of section header table entries in sh_size: %d", shnum);
+    }
+  else
+    {
+      if (shnum >= SHN_LORESERVE)
+	grub_util_error ("Invalid number of section header table entries in e_shnum: %d", shnum);
+    }
+
+  return shnum;
+}
+
 static Elf_Shdr *
 find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const char *name)
 {
@@ -149,7 +172,7 @@ find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const c
   str = (char *) e + grub_target_to_host (s->sh_offset);
 
   for (i = 0, s = get_shdr (arch, e, 0);
-       i < grub_target_to_host16 (e->e_shnum);
+       i < get_shnum (arch, e);
        i++, s = get_shdr (arch, e, i))
     if (strcmp (str + grub_target_to_host32 (s->sh_name), name) == 0)
       return s;
@@ -176,12 +199,12 @@ get_symtab (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word
   Elf_Sym *sym;
 
   for (i = 0, s = get_shdr (arch, e, 0);
-       i < grub_target_to_host16 (e->e_shnum);
+       i < get_shnum (arch, e);
        i++, s = get_shdr (arch, e, i))
     if (grub_target_to_host32 (s->sh_type) == SHT_SYMTAB)
       break;
 
-  if (i == grub_target_to_host16 (e->e_shnum))
+  if (i == get_shnum (arch, e))
     return NULL;
 
   sym = (Elf_Sym *) ((char *) e + grub_target_to_host (s->sh_offset));
@@ -370,7 +393,7 @@ check_relocations (const char * const modname,
   unsigned i;
 
   for (i = 0, s = get_shdr (arch, e, 0);
-       i < grub_target_to_host16 (e->e_shnum);
+       i < get_shnum (arch, e);
        i++, s = get_shdr (arch, e, i))
     if (grub_target_to_host32 (s->sh_type) == SHT_REL || grub_target_to_host32 (s->sh_type) == SHT_RELA)
       {
@@ -382,7 +405,7 @@ check_relocations (const char * const modname,
 	  grub_util_error ("%s: unsupported SHT_RELA", modname);
 
 	/* Find the target segment.  */
-	if (grub_target_to_host32 (s->sh_info) >= grub_target_to_host16 (e->e_shnum))
+	if (grub_target_to_host32 (s->sh_info) >= get_shnum (arch, e))
 	  grub_util_error ("%s: orphaned reloc section", modname);
 	ts = get_shdr (arch, e, grub_target_to_host32 (s->sh_info));
 
@@ -423,7 +446,7 @@ SUFFIX(grub_module_verify) (const char * const filename,
 
   /* Make sure that every section is within the core.  */
   if (size < grub_target_to_host (e->e_shoff)
-      + (grub_uint32_t) grub_target_to_host16 (e->e_shentsize) * grub_target_to_host16(e->e_shnum))
+      + (grub_uint32_t) grub_target_to_host16 (e->e_shentsize) * get_shnum (arch, e))
     {
       grub_util_error ("%s: ELF sections outside core", filename);
     }
-- 
2.27.0

[PATCH 3/4] util/grub-module-verifierXX.c: Validate elf section header table index for section name string table

[Alec Brown]

In grub-module-verifierXX.c, the function find_section() uses the value from
grub_target_to_host16 (e->e_shstrndx) 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, the function get_shstrndx() is being added
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 it may look as though the argument *arch isn't being used, it's
actually required in order to use the macros grub_target_to_host*(x) which are
unwinded to grub_target_to_host*_real(arch, (x)) based on defines earlier in the
file.

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

diff --git a/util/grub-module-verifierXX.c b/util/grub-module-verifierXX.c
index 61b82141f..3a5265aff 100644
--- a/util/grub-module-verifierXX.c
+++ b/util/grub-module-verifierXX.c
@@ -161,6 +161,29 @@ get_shnum (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
   return shnum;
 }
 
+static Elf_Word
+get_shstrndx (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
+{
+  Elf_Shdr *s;
+  Elf_Word shstrndx;
+
+  shstrndx = grub_target_to_host16 (e->e_shstrndx);
+  if (shstrndx == SHN_XINDEX)
+    {
+      s = get_shdr (arch, e, 0);
+      shstrndx = grub_target_to_host (s->sh_link);
+      if (shstrndx < SHN_LORESERVE)
+	grub_util_error ("Invalid section header table index in sh_link: %d", shstrndx);
+    }
+  else
+    {
+      if (shstrndx >= SHN_LORESERVE)
+	grub_util_error ("Invalid section header table index in e_shstrndx: %d", shstrndx);
+    }
+
+  return shstrndx;
+}
+
 static Elf_Shdr *
 find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const char *name)
 {
@@ -168,7 +191,7 @@ find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const c
   const char *str;
   unsigned i;
 
-  s = get_shdr (arch, e, grub_target_to_host16 (e->e_shstrndx));
+  s = get_shdr (arch, e, get_shstrndx (arch, e));
   str = (char *) e + grub_target_to_host (s->sh_offset);
 
   for (i = 0, s = get_shdr (arch, e, 0);
-- 
2.27.0

[PATCH 4/4] util/grub-module-verifierXX.c: Add module_size parameter to functions for sanity checking

[Alec Brown]

In grub-module-verifierXX.c, the function grub_module_verifyXX() performs an
initial check that the ELF section headers are within the module's size but
doesn't check if the sections being accessed have contents that are within the
module's size. In particular, we need to check that sh_offset and sh_size are
less than the module's size. However, for some section header types we don't
need to make these checks. For the type SHT_NULL, the section header is marked
as inactive and the rest of the members within the section header have undefined
values, so we don't need to check for sh_offset or sh_size. In the case of the
type SHT_NOBITS, sh_offset has a conceptual offset which may be beyond the
module size. Also, this type's sh_size may have a non-zero size, but a section
of this type will take up no space in the module. This can all be checked in the
function get_shdr(), but in order to do so, the parameter module_size must be
added to functions so that the value of the module size can be used in
get_shdr() from grub_module_verifyXX().

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

diff --git a/util/grub-module-verifierXX.c b/util/grub-module-verifierXX.c
index 3a5265aff..ac7b6fa78 100644
--- a/util/grub-module-verifierXX.c
+++ b/util/grub-module-verifierXX.c
@@ -132,10 +132,21 @@ 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)
+get_shdr (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word index, size_t module_size)
 {
-  return (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff) +
-	 	       index * grub_target_to_host16 (e->e_shentsize));
+  Elf_Shdr *s;
+
+  s = (Elf_Shdr *) ((char *) e + grub_target_to_host (e->e_shoff) +
+		    index * grub_target_to_host16 (e->e_shentsize));
+  /* Check that the header being accessed isn't outside the module size */
+  if (grub_target_to_host32 (s->sh_type) != SHT_NULL && grub_target_to_host32 (s->sh_type) != SHT_NOBITS)
+    {
+      if (grub_target_to_host (s->sh_offset) > module_size)
+	grub_util_error ("Section %d starts after the end of the module", index);
+      if (grub_target_to_host (s->sh_offset) + grub_target_to_host (s->sh_size) > module_size)
+	grub_util_error ("Section %d ends after the end of the module", index);
+    }
+  return s;
 }
 
 static Elf_Word
@@ -147,7 +158,7 @@ get_shnum (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
   shnum = grub_target_to_host16 (e->e_shnum);
   if (shnum == SHN_UNDEF)
     {
-      s = get_shdr (arch, e, 0);
+      s = get_shdr (arch, e, 0, 0);
       shnum = grub_target_to_host (s->sh_size);
       if (shnum < SHN_LORESERVE)
 	grub_util_error ("Invalid number of section header table entries in sh_size: %d", shnum);
@@ -162,7 +173,7 @@ get_shnum (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
 }
 
 static Elf_Word
-get_shstrndx (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
+get_shstrndx (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, size_t module_size)
 {
   Elf_Shdr *s;
   Elf_Word shstrndx;
@@ -170,7 +181,7 @@ get_shstrndx (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
   shstrndx = grub_target_to_host16 (e->e_shstrndx);
   if (shstrndx == SHN_XINDEX)
     {
-      s = get_shdr (arch, e, 0);
+      s = get_shdr (arch, e, 0, 0);
       shstrndx = grub_target_to_host (s->sh_link);
       if (shstrndx < SHN_LORESERVE)
 	grub_util_error ("Invalid section header table index in sh_link: %d", shstrndx);
@@ -185,18 +196,18 @@ get_shstrndx (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
 }
 
 static Elf_Shdr *
-find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const char *name)
+find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const char *name, size_t module_size)
 {
   Elf_Shdr *s;
   const char *str;
   unsigned i;
 
-  s = get_shdr (arch, e, get_shstrndx (arch, e));
+  s = get_shdr (arch, e, get_shstrndx (arch, e, module_size), module_size);
   str = (char *) e + grub_target_to_host (s->sh_offset);
 
-  for (i = 0, s = get_shdr (arch, e, 0);
+  for (i = 0, s = get_shdr (arch, e, 0, 0);
        i < get_shnum (arch, e);
-       i++, s = get_shdr (arch, e, i))
+       i++, s = get_shdr (arch, e, i, module_size))
     if (strcmp (str + grub_target_to_host32 (s->sh_name), name) == 0)
       return s;
   return NULL;
@@ -204,9 +215,9 @@ find_section (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, const c
 
 static void
 check_license (const char * const filename,
-	       const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
+	       const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, size_t module_size)
 {
-  Elf_Shdr *s = find_section (arch, e, ".module_license");
+  Elf_Shdr *s = find_section (arch, e, ".module_license", module_size);
   if (s && (strcmp ((char *) e + grub_target_to_host(s->sh_offset), "LICENSE=GPLv3") == 0
 	    || strcmp ((char *) e + grub_target_to_host(s->sh_offset), "LICENSE=GPLv3+") == 0
 	    || strcmp ((char *) e + grub_target_to_host(s->sh_offset), "LICENSE=GPLv2+") == 0))
@@ -215,15 +226,15 @@ check_license (const char * const filename,
 }
 
 static Elf_Sym *
-get_symtab (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word *size, Elf_Word *entsize)
+get_symtab (const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, Elf_Word *size, Elf_Word *entsize, size_t module_size)
 {
   unsigned i;
   Elf_Shdr *s;
   Elf_Sym *sym;
 
-  for (i = 0, s = get_shdr (arch, e, 0);
+  for (i = 0, s = get_shdr (arch, e, 0, 0);
        i < get_shnum (arch, e);
-       i++, s = get_shdr (arch, e, i))
+       i++, s = get_shdr (arch, e, i, module_size))
     if (grub_target_to_host32 (s->sh_type) == SHT_SYMTAB)
       break;
 
@@ -253,7 +264,7 @@ is_whitelisted (const char *modname, const char **whitelist)
 static void
 check_symbols (const struct grub_module_verifier_arch *arch,
 	       Elf_Ehdr *e, const char *modname,
-	       const char **whitelist_empty)
+	       const char **whitelist_empty, size_t module_size)
 {
   Elf_Sym *sym;
   Elf_Word size, entsize;
@@ -261,7 +272,7 @@ check_symbols (const struct grub_module_verifier_arch *arch,
 
   /* Module without symbol table and without .moddeps section is useless
      at boot time, so catch it early to prevent build errors */
-  sym = get_symtab (arch, e, &size, &entsize);
+  sym = get_symtab (arch, e, &size, &entsize, module_size);
   if (!sym)
     {
       Elf_Shdr *s;
@@ -273,7 +284,7 @@ check_symbols (const struct grub_module_verifier_arch *arch,
       if (is_whitelisted (modname, whitelist_empty))
 	return;
 
-      s = find_section (arch, e, ".moddeps");
+      s = find_section (arch, e, ".moddeps", module_size);
 
       if (!s)
 	grub_util_error ("%s: no symbol table and no .moddeps section", modname);
@@ -328,13 +339,13 @@ is_symbol_local(Elf_Sym *sym)
 static void
 section_check_relocations (const char * const modname,
 			   const struct grub_module_verifier_arch *arch, void *ehdr,
-			   Elf_Shdr *s, size_t target_seg_size)
+			   Elf_Shdr *s, size_t target_seg_size, size_t module_size)
 {
   Elf_Rel *rel, *max;
   Elf_Sym *symtab;
   Elf_Word symtabsize, symtabentsize;
 
-  symtab = get_symtab (arch, ehdr, &symtabsize, &symtabentsize);
+  symtab = get_symtab (arch, ehdr, &symtabsize, &symtabentsize, module_size);
   if (!symtab)
     grub_util_error ("%s: relocation without symbol table", modname);
 
@@ -410,14 +421,14 @@ section_check_relocations (const char * const modname,
 
 static void
 check_relocations (const char * const modname,
-		   const struct grub_module_verifier_arch *arch, Elf_Ehdr *e)
+		   const struct grub_module_verifier_arch *arch, Elf_Ehdr *e, size_t module_size)
 {
   Elf_Shdr *s;
   unsigned i;
 
-  for (i = 0, s = get_shdr (arch, e, 0);
+  for (i = 0, s = get_shdr (arch, e, 0, 0);
        i < get_shnum (arch, e);
-       i++, s = get_shdr (arch, e, i))
+       i++, s = get_shdr (arch, e, i, module_size))
     if (grub_target_to_host32 (s->sh_type) == SHT_REL || grub_target_to_host32 (s->sh_type) == SHT_RELA)
       {
 	Elf_Shdr *ts;
@@ -430,9 +441,9 @@ check_relocations (const char * const modname,
 	/* Find the target segment.  */
 	if (grub_target_to_host32 (s->sh_info) >= get_shnum (arch, e))
 	  grub_util_error ("%s: orphaned reloc section", modname);
-	ts = get_shdr (arch, e, grub_target_to_host32 (s->sh_info));
+	ts = get_shdr (arch, e, grub_target_to_host32 (s->sh_info), module_size);
 
-	section_check_relocations (modname, arch, e, s, grub_target_to_host (ts->sh_size));
+	section_check_relocations (modname, arch, e, s, grub_target_to_host (ts->sh_size), module_size);
       }
 }
 
@@ -474,17 +485,17 @@ SUFFIX(grub_module_verify) (const char * const filename,
       grub_util_error ("%s: ELF sections outside core", filename);
     }
 
-  check_license (filename, arch, e);
+  check_license (filename, arch, e, size);
 
   Elf_Shdr *s;
   const char *modname;
 
-  s = find_section (arch, e, ".modname");
+  s = find_section (arch, e, ".modname", size);
   if (!s)
     grub_util_error ("%s: no module name found", filename);
 
   modname = (const char *) e + grub_target_to_host (s->sh_offset);
 
-  check_symbols(arch, e, modname, whitelist_empty);
-  check_relocations(modname, arch, e);
+  check_symbols(arch, e, modname, whitelist_empty, size);
+  check_relocations(modname, arch, e, size);
 }
-- 
2.27.0

Re: [PATCH 4/4] util/grub-module-verifierXX.c: Add module_size parameter to functions for sanity checking

[Daniel Kiper]

On Wed, Feb 02, 2022 at 07:27:00PM -0500, Alec Brown wrote:
> In grub-module-verifierXX.c, the function grub_module_verifyXX() performs an
> initial check that the ELF section headers are within the module's size but
> doesn't check if the sections being accessed have contents that are within the
> module's size. In particular, we need to check that sh_offset and sh_size are
> less than the module's size. However, for some section header types we don't
> need to make these checks. For the type SHT_NULL, the section header is marked
> as inactive and the rest of the members within the section header have undefined
> values, so we don't need to check for sh_offset or sh_size. In the case of the
> type SHT_NOBITS, sh_offset has a conceptual offset which may be beyond the
> module size. Also, this type's sh_size may have a non-zero size, but a section
> of this type will take up no space in the module. This can all be checked in the
> function get_shdr(), but in order to do so, the parameter module_size must be
> added to functions so that the value of the module size can be used in
> get_shdr() from grub_module_verifyXX().
>
> Signed-off-by: Alec Brown <alec.r.brown@oracle.com>

Sadly this patch breaks one of ARM builds:
  build-grub-module-verifier: error: Section 12 starts after the end of the module.
  Makefile:47473: recipe for target 'disk.mod' failed
  make[3]: *** [disk.mod] Error 1
  make[3]: *** Waiting for unfinished jobs....
  build-grub-module-verifier: error: Section 12 starts after the end of the module.
  Makefile:47473: recipe for target 'boot.mod' failed
  make[3]: *** [boot.mod] Error 1
  ...

You can reproduce this by doing:
  ./configure --target=arm-linux-gnueabihf --with-platform=coreboot --enable-grub-mkfont --prefix="`pwd`/grub-dist"
  make install

I have taken the rest of patches and skipped this one.

Daniel

Re: [PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c

[Darren Kenny]

Hi Alec,

These look good to me, thanks for handling the Coverity issues here.

For the series:

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

Thanks,

Darren.

On Wednesday, 2022-02-02 at 19:26:56 -05, Alec Brown wrote:
> Coverity identified several untrusted loop bounds in
> util/grub-module-verifierXX.c. This patch series addresses these bugs, cleans up
> lengthy equations, and makes checks to values based on the elf manual page.
>
> The Coverity Bugs being addressed are:
> CID 314021
> CID 314027
> CID 314033
>
> Alec Brown (4):
>       util/grub-module-verifierXX.c: Add function to calculate section headers
>       util/grub-module-verifierXX.c: Validate number of elf section header table entries
>       util/grub-module-verifierXX.c: Validate elf section header table index for section name string table
>       util/grub-module-verifierXX.c: Add module_size parameter to functions for sanity checking
>
>  util/grub-module-verifierXX.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 93 insertions(+), 31 deletions(-)

Re: [PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c

[Daniel Kiper]

On Thu, Feb 03, 2022 at 11:56:49AM +0000, Darren Kenny wrote:
> Hi Alec,
>
> These look good to me, thanks for handling the Coverity issues here.
>
> For the series:
>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel