grub-core/loader/i386/bsdXX: Avoid downcasting (char *) to (Elf_Shdr *)

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. By doing
this, the issue becomes masked from Coverity's view. In the following patches,
we check limits to ensure the data isn't tainted.

Also, switched use of (char *) to (grub_uint8_t *) to give a better indication
of pointer arithmetic and not suggest use of a C string.

Fixes: CID 314018
Fixes: CID 314030
Fixes: CID 314031
Fixes: CID 314039
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

diff --git a/grub-core/loader/i386/bsdXX.c b/grub-core/loader/i386/bsdXX.c
index e4f4aa36576fe3ec5dc7252f2d5cb5065df8af1d..e6edc6fb620c26c66c697119fe22d1c513623434 100644 (file)
--- 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;
@@ -77,8 +77,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct grub_relocator *relocator,
                                          char *argv[], grub_addr_t *kern_end)
 {
   Elf_Ehdr e;
-  Elf_Shdr *s;
-  char *shdr = 0;
+  Elf_Shdr *s, *shdr = NULL;
   grub_addr_t curload, module;
   grub_err_t err;
   grub_size_t chunk_size = 0;
@@ -90,9 +89,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 +111,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;
@@ -172,8 +169,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct grub_relocator *relocator,
                                      grub_addr_t *kern_end)
 {
   Elf_Ehdr e;
-  Elf_Shdr *s;
-  char *shdr = 0;
+  Elf_Shdr *s, *shdr = NULL;
   grub_addr_t curload, module;
   grub_err_t err;
   grub_size_t chunk_size = 0;
@@ -185,9 +181,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 +209,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;
@@ -284,8 +278,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 *s, *shdr = NULL;
   unsigned symoff, stroff, symsize, strsize;
   grub_freebsd_addr_t symstart, symend, symentsize, dynamic;
   Elf_Sym *sym;
@@ -306,13 +299,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 +311,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;
 
@@ -426,8 +417,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 *s, *symsh, *strsh, *shdr = NULL;
   unsigned symsize, strsize;
   void *sym_chunk;
   grub_uint8_t *curload;
@@ -443,20 +433,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 +478,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;
@@ -564,8 +551,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 *s, *shdr = NULL;
 
     err = read_headers (file, filename, &e, &shdr);
     if (err)
@@ -574,12 +560,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 +574,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);