Hi Alec, These changes look good to me,
Reviewed-by: Darren Kenny <darren.ke...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel