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

[Darren Kenny]

Hi Alec,

These changes look good to me,

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

Thanks,

Darren.

On Friday, 2022-08-12 at 18:25:44 -04, Alec Brown wrote:
> Updates from v1:
>       - A few patches didn't work on 32-bit platforms. Fixed this by renaming
>       function definitions in multiboot_elfxx.c. The patches that did work 
> were
>       merged with the master branch.
>       - Added a patch to make error conditionals consistent in comparing to
>       GRUB_ERR_NONE.
>
> 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. The first version of
> the patch series contained the patches fixing the Coverity bugs, but those 
> have
> been merged with the master branch. The remaining patches add functions to
> check for the correct elf header values, update previous work done in
> util/grub-module-verifierXX.c that checks elf header values, and update error
> conditionals of the affected files.
>
> Also, I was able to verify that these patches work by using Multiboot and
> Multiboot2 to boot a Xen image.
>
> Alec Brown (5):
>       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: Changed get_shnum() return type
>       loader: Update error conditionals to use enums
>
>  grub-core/kern/elf.c               |  18 ++++++++++++++++++
>  grub-core/kern/elfXX.c             | 101 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  grub-core/loader/i386/bsdXX.c      | 131 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------
>  grub-core/loader/multiboot_elfxx.c |  85 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------
>  include/grub/elf.h                 |  23 +++++++++++++++++++++++
>  util/grub-module-verifierXX.c      |  10 ++++++----
>  6 files changed, 292 insertions(+), 76 deletions(-)
>
> Interdiff against v1:
> diff --git a/grub-core/loader/i386/bsdXX.c b/grub-core/loader/i386/bsdXX.c
> index 2cc1daa49..09d909f1b 100644
> --- a/grub-core/loader/i386/bsdXX.c
> +++ b/grub-core/loader/i386/bsdXX.c
> @@ -52,7 +52,7 @@ read_headers (grub_file_t file, const char *filename, 
> Elf_Ehdr *e, Elf_Shdr **sh
>      return grub_error (GRUB_ERR_BAD_OS, N_("invalid arch-dependent ELF 
> magic"));
>
>    err = grub_elf_get_shnum (e, &shnum);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      return err;
>
>    *shdr = grub_calloc (shnum, e->e_shentsize);
> @@ -94,7 +94,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct 
> grub_relocator *relocator,
>    curload = module = ALIGN_PAGE (*kern_end);
>
>    err = read_headers (file, argv[0], &e, &shdr);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      goto out;
>
>    err = grub_elf_get_shnum (&e, &shnum);
> @@ -118,7 +118,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct 
> grub_relocator *relocator,
>      grub_relocator_chunk_t ch;
>      err = grub_relocator_alloc_chunk_addr (relocator, &ch,
>                                            module, chunk_size);
> -    if (err)
> +    if (err != GRUB_ERR_NONE)
>        goto out;
>      chunk_src = get_virtual_current_address (ch);
>    }
> @@ -143,7 +143,7 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct 
> grub_relocator *relocator,
>         case SHT_PROGBITS:
>           err = load (file, argv[0], (grub_uint8_t *) chunk_src + curload - 
> *kern_end,
>                       s->sh_offset, s->sh_size);
> -         if (err)
> +         if (err != GRUB_ERR_NONE)
>             goto out;
>           break;
>         case SHT_NOBITS:
> @@ -159,11 +159,11 @@ SUFFIX (grub_freebsd_load_elfmodule_obj) (struct 
> grub_relocator *relocator,
>    err = grub_freebsd_add_meta_module (argv[0], 
> FREEBSD_MODTYPE_ELF_MODULE_OBJ,
>                                       argc - 1, argv + 1, module,
>                                       curload - module);
> -  if (! err)
> +  if (err == GRUB_ERR_NONE)
>      err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA
>                              | FREEBSD_MODINFOMD_ELFHDR,
>                              &e, sizeof (e));
> -  if (! err)
> +  if (err == GRUB_ERR_NONE)
>      err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA
>                              | FREEBSD_MODINFOMD_SHDR,
>                              shdr, shnum * e.e_shentsize);
> @@ -192,7 +192,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct 
> grub_relocator *relocator,
>    curload = module = ALIGN_PAGE (*kern_end);
>
>    err = read_headers (file, argv[0], &e, &shdr);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      goto out;
>
>    err = grub_elf_get_shnum (&e, &shnum);
> @@ -225,7 +225,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct 
> grub_relocator *relocator,
>
>      err = grub_relocator_alloc_chunk_addr (relocator, &ch,
>                                            module, chunk_size);
> -    if (err)
> +    if (err != GRUB_ERR_NONE)
>        goto out;
>
>      chunk_src = get_virtual_current_address (ch);
> @@ -252,7 +252,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct 
> grub_relocator *relocator,
>                       (grub_uint8_t *) chunk_src + module
>                       + s->sh_addr - *kern_end,
>                       s->sh_offset, s->sh_size);
> -         if (err)
> +         if (err != GRUB_ERR_NONE)
>             goto out;
>           break;
>         case SHT_NOBITS:
> @@ -271,12 +271,12 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct 
> grub_relocator *relocator,
>    load (file, argv[0], (grub_uint8_t *) chunk_src + curload - *kern_end, 
> e.e_shoff,
>         (grub_size_t) shnum * e.e_shentsize);
>    e.e_shoff = curload - module;
> -  curload +=  (grub_addr_t) 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_size_t) phnum * e.e_phentsize);
>    e.e_phoff = curload - module;
> -  curload +=  (grub_addr_t) phnum * e.e_phentsize;
> +  curload += (grub_addr_t) phnum * e.e_phentsize;
>
>    *kern_end = curload;
>
> @@ -285,7 +285,7 @@ SUFFIX (grub_freebsd_load_elfmodule) (struct 
> grub_relocator *relocator,
>                                 curload - module);
>  out:
>    grub_free (shdr);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      return err;
>    return SUFFIX (grub_freebsd_load_elf_meta) (relocator, file, argv[0], 
> kern_end);
>  }
> @@ -313,7 +313,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct 
> grub_relocator *relocator,
>    grub_size_t chunk_size;
>
>    err = read_headers (file, filename, &e, &shdr);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      goto out;
>
>    err = grub_elf_get_shnum (&e, &shnum);
> @@ -323,7 +323,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct 
> grub_relocator *relocator,
>    err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA |
>                            FREEBSD_MODINFOMD_ELFHDR, &e,
>                            sizeof (e));
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      goto out;
>
>    for (s = shdr; s < (Elf_Shdr *) ((grub_uint8_t *) shdr + shnum * 
> e.e_shentsize);
> @@ -351,7 +351,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct 
> grub_relocator *relocator,
>      grub_relocator_chunk_t ch;
>      err = grub_relocator_alloc_chunk_addr (relocator, &ch,
>                                            symtarget, chunk_size);
> -    if (err)
> +    if (err != GRUB_ERR_NONE)
>        goto out;
>      sym_chunk = get_virtual_current_address (ch);
>    }
> @@ -414,14 +414,14 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct 
> grub_relocator *relocator,
>        err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA |
>                                FREEBSD_MODINFOMD_DYNAMIC, &dynamic,
>                                sizeof (dynamic));
> -      if (err)
> +      if (err != GRUB_ERR_NONE)
>         goto out;
>      }
>
>    err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA |
>                            FREEBSD_MODINFOMD_SSYM, &symstart,
>                            sizeof (symstart));
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      goto out;
>
>    err = grub_bsd_add_meta (FREEBSD_MODINFO_METADATA |
> @@ -429,7 +429,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (struct 
> grub_relocator *relocator,
>                            sizeof (symend));
>  out:
>    grub_free (shdr);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      return err;
>
>    *kern_end = ALIGN_PAGE (symend);
> @@ -455,7 +455,7 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator 
> *relocator,
>    grub_addr_t symtarget;
>
>    err = read_headers (file, filename, &e, &shdr);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      {
>        grub_free (shdr);
>        return grub_errno;
> @@ -492,7 +492,7 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator 
> *relocator,
>      grub_relocator_chunk_t ch;
>      err = grub_relocator_alloc_chunk_addr (relocator, &ch,
>                                            symtarget, chunk_size);
> -    if (err)
> +    if (err != GRUB_ERR_NONE)
>        goto out;
>      sym_chunk = get_virtual_current_address (ch);
>    }
> @@ -566,7 +566,7 @@ SUFFIX (grub_netbsd_load_elf_meta) (struct grub_relocator 
> *relocator,
>                            sizeof (symtab));
>  out:
>    grub_free (shdr);
> -  if (err)
> +  if (err != GRUB_ERR_NONE)
>      return err;
>
>    *kern_end = ALIGN_PAGE (symtarget + chunk_size);
> @@ -590,7 +590,7 @@ SUFFIX(grub_openbsd_find_ramdisk) (grub_file_t file,
>      Elf_Shnum shnum;
>
>      err = read_headers (file, filename, &e, &shdr);
> -    if (err)
> +    if (err != GRUB_ERR_NONE)
>        {
>         grub_free (shdr);
>         return err;
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 3e72c6caa..b76451f73 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -25,9 +25,9 @@
>  # define Elf_Shdr              Elf32_Shdr
>  # define Elf_Word              Elf32_Word
>  # define Elf_Shnum             Elf32_Shnum
> -# 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
> +# define grub_multiboot_elf_get_shnum          grub_elf32_get_shnum
> +# define grub_multiboot_elf_get_shstrndx       grub_elf32_get_shstrndx
> +# define grub_multiboot_elf_get_phnum          grub_elf32_get_phnum
>  #elif defined(MULTIBOOT_LOAD_ELF64)
>  # define XX                    64
>  # define E_MACHINE             MULTIBOOT_ELF64_MACHINE
> @@ -37,9 +37,9 @@
>  # define Elf_Shdr              Elf64_Shdr
>  # define Elf_Word              Elf64_Word
>  # define Elf_Shnum             Elf64_Shnum
> -# 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
> +# define grub_multiboot_elf_get_shnum          grub_elf64_get_shnum
> +# define grub_multiboot_elf_get_shstrndx       grub_elf64_get_shstrndx
> +# define grub_multiboot_elf_get_phnum          grub_elf64_get_phnum
>  #else
>  #error "I'm confused"
>  #endif
> @@ -87,15 +87,15 @@ 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);
> +  err = grub_multiboot_elf_get_shnum (ehdr, &shnum);
>    if (err != GRUB_ERR_NONE)
>      return err;
>
> -  err = grub_elf_get_shstrndx (ehdr, &shstrndx);
> +  err = grub_multiboot_elf_get_shstrndx (ehdr, &shstrndx);
>    if (err != GRUB_ERR_NONE)
>      return err;
>
> -  err = grub_elf_get_phnum (ehdr, &phnum);
> +  err = grub_multiboot_elf_get_phnum (ehdr, &phnum);
>    if (err != GRUB_ERR_NONE)
>      return err;
>
> @@ -138,7 +138,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>                                                    load_size, mld->align ? 
> mld->align : 1,
>
> -      if (err)
> +      if (err != GRUB_ERR_NONE)
>          {
>            grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
>            return err;
> @@ -173,7 +173,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>               err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT 
> (relocator), &ch,
>                                                      phdr(i)->p_paddr, 
> phdr(i)->p_memsz);
>
> -             if (err)
> +             if (err != GRUB_ERR_NONE)
>                 {
>                   grub_dprintf ("multiboot_loader", "Cannot allocate memory 
> for OS image\n");
>                   return err;
> @@ -284,7 +284,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>                                                   sh->sh_size, 
> sh->sh_addralign,
>                                                   
> GRUB_RELOCATOR_PREFERENCE_NONE,
>                                                   
> mld->avoid_efi_boot_services);
> -         if (err)
> +         if (err != GRUB_ERR_NONE)
>             {
>               grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);
>               return err;
> @@ -322,6 +322,6 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  #undef Elf_Shdr
>  #undef Elf_Word
>  #undef Elf_Shnum
> -#undef grub_elf_get_shnum
> -#undef grub_elf_get_shstrndx
> -#undef grub_elf_get_phnum
> +#undef grub_multiboot_elf_get_shnum
> +#undef grub_multiboot_elf_get_shstrndx
> +#undef grub_multiboot_elf_get_phnum