[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-c
|
From: |
Alec Brown |
|
Subject: |
[PATCH v2 0/5] Fix coverity bugs and add checks for elf values in grub-core |
|
Date: |
Fri, 12 Aug 2022 18:25:44 -0400 |
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 <=
- [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, 2022/08/15