From: Darren Kenny <darren.kenny@oracle.com>
To: Alec Brown <alec.r.brown@oracle.com>, grub-devel@gnu.org
Cc: daniel.kiper@oracle.com, alec.r.brown@oracle.com
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 [thread overview]
Message-ID: <m2fshx7roe.fsf@oracle.com> (raw)
In-Reply-To: <1660343149-8188-1-git-send-email-alec.r.brown@oracle.com>
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
next prev parent reply other threads:[~2022-08-15 10:00 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-12 22:25 [PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-core Alec Brown
2022-08-12 22:25 ` [PATCH v2 1/5] elf: Validate number of elf section header table entries Alec Brown
2022-08-12 22:25 ` [PATCH v2 2/5] elf: Validate elf section header table index for section name string table Alec Brown
2022-08-12 22:25 ` [PATCH v2 3/5] elf: Validate number of elf program header table entries Alec Brown
2022-08-12 22:25 ` [PATCH v2 4/5] util/grub-module-verifierXX.c: Changed get_shnum() return type Alec Brown
2022-08-12 22:25 ` [PATCH v2 5/5] loader: Update error conditionals to use enums Alec Brown
2022-08-15 10:00 ` Darren Kenny [this message]
2022-08-19 14:19 ` [PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-core 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=m2fshx7roe.fsf@oracle.com \
--to=darren.kenny@oracle.com \
--cc=alec.r.brown@oracle.com \
--cc=daniel.kiper@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.