[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 0/5] Fix coverity bugs and add checks for elf values in gr
From: |
Darren Kenny |
Subject: |
Re: [PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-core |
Date: |
Mon, 15 Aug 2022 11:00:01 +0100 |
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
- [PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-core, Alec Brown, 2022/08/12
- [PATCH v2 1/5] elf: Validate number of elf section header table entries, Alec Brown, 2022/08/12
- [PATCH v2 3/5] elf: Validate number of elf program header table entries, Alec Brown, 2022/08/12
- [PATCH v2 2/5] elf: Validate elf section header table index for section name string table, Alec Brown, 2022/08/12
- [PATCH v2 4/5] util/grub-module-verifierXX.c: Changed get_shnum() return type, Alec Brown, 2022/08/12
- [PATCH v2 5/5] loader: Update error conditionals to use enums, Alec Brown, 2022/08/12
- Re: [PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-core,
Darren Kenny <=