All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alec Brown <alec.r.brown@oracle.com>
To: grub-devel@gnu.org
Cc: daniel.kiper@oracle.com, darren.kenny@oracle.com
Subject: [PATCH 4/4] util/grub-module-verifierXX.c: Add module_size parameter to functions for sanity checking
Date: Wed,  2 Feb 2022 19:27:00 -0500	[thread overview]
Message-ID: <1643848020-8197-5-git-send-email-alec.r.brown@oracle.com> (raw)
In-Reply-To: <1643848020-8197-1-git-send-email-alec.r.brown@oracle.com>

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



  parent reply	other threads:[~2022-02-03  0:27 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-03  0:26 [PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c Alec Brown
2022-02-03  0:26 ` [PATCH 1/4] util/grub-module-verifierXX.c: Add function to calculate section headers Alec Brown
2022-02-03  0:26 ` [PATCH 2/4] util/grub-module-verifierXX.c: Validate number of elf section header table entries Alec Brown
2022-02-03  0:26 ` [PATCH 3/4] util/grub-module-verifierXX.c: Validate elf section header table index for section name string table Alec Brown
2022-02-03  0:27 ` Alec Brown [this message]
2022-02-08 16:28   ` [PATCH 4/4] util/grub-module-verifierXX.c: Add module_size parameter to functions for sanity checking Daniel Kiper
2022-02-03 11:56 ` [PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c Darren Kenny
2022-02-04 14:36   ` Daniel Kiper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1643848020-8197-5-git-send-email-alec.r.brown@oracle.com \
    --to=alec.r.brown@oracle.com \
    --cc=daniel.kiper@oracle.com \
    --cc=darren.kenny@oracle.com \
    --cc=grub-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.