* [PATCH] multiboot: Add support for program headers at "strange" locations
@ 2020-05-23 15:30 Jacob Paul
2020-05-23 15:37 ` John Paul Adrian Glaubitz
0 siblings, 1 reply; 2+ messages in thread
From: Jacob Paul @ 2020-05-23 15:30 UTC (permalink / raw)
To: grub-devel
Currently, grub_multiboot_load_elf() will throw an error if the program
header is at a too high offset (since it would be outside the allocated
buffer space of MULTIBOOT_SEARCH bytes). It is understandable why it
does this, since 99.99% of all cases e_phoff has a value of 0x34 or
0x40. So, to be able to support a (uncommon) circumstance like this, the
program header can be loaded into a separate buffer if needed, and then
freed at the end. The only downside to this is that
grub_multiboot_load_elf() must at every return statement check if
phdr_base is a separate buffer and if so de-allocate it.
Signed-off-by: Jacob Paul <jacob.paul@64epicks.com>
---
grub-core/loader/multiboot_elfxx.c | 86 +++++++++++++++++++++++++-----
1 file changed, 74 insertions(+), 12 deletions(-)
diff --git a/grub-core/loader/multiboot_elfxx.c
b/grub-core/loader/multiboot_elfxx.c
index 70cd1db51..a538d871a 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -75,12 +75,37 @@ 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"));
- /* FIXME: Should we support program headers at strange locations? */
+ /* Program header outside of the allocated buffer space. Weird, but
just work around it by allocating a new buffer for it */
if (ehdr->e_phoff + (grub_uint32_t) ehdr->e_phnum *
ehdr->e_phentsize > MULTIBOOT_SEARCH)
- return grub_error (GRUB_ERR_BAD_OS, "program header at a too high
offset");
+ {
+ phdr_base = grub_malloc ((grub_size_t) ehdr->e_phnum *
ehdr->e_phentsize);
+ if (!phdr_base)
+ return grub_errno;
+
+ if (grub_file_seek (mld->file, (grub_off_t) ehdr->e_phoff) ==
(grub_off_t) -1)
+ {
+ grub_free(phdr_base);
+ return grub_error (GRUB_ERR_BAD_OS, "invalid program header
offset");
+ }
- phdr_base = (char *) mld->buffer + ehdr->e_phoff;
+ if (grub_file_read (mld->file, phdr_base, (grub_size_t)
ehdr->e_phnum * ehdr->e_phentsize)
+ != (grub_ssize_t) ehdr->e_phnum * ehdr->e_phentsize)
+ {
+ grub_free (phdr_base);
+ if (!grub_errno)
+ grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of
file %s"), mld->filename);
+ return grub_errno;
+ }
+ }
+ else
+ {
+ phdr_base = (char *) mld->buffer + ehdr->e_phoff;
+ }
+
#define phdr(i) ((Elf_Phdr *) (phdr_base + (i) * ehdr->e_phentsize))
+/* FIXME: find a better way to de-allocate phdr_base if an error occurs.
+ Putting this in front of every exit is probably not the prettiest
solution */
+#define phdr_free() if ((grub_size_t) phdr_base != (grub_size_t)
mld->buffer + ehdr->e_phoff) grub_free(phdr_base);
mld->link_base_addr = ~0;
@@ -94,7 +119,10 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
*mld)
#ifdef MULTIBOOT_LOAD_ELF64
if (highest_load >= 0x100000000)
- return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
+ {
+ phdr_free();
+ return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
+ }
#endif
if (mld->relocatable)
@@ -107,7 +135,10 @@ CONCAT(grub_multiboot_load_elf, XX)
(mbi_load_data_t *mld)
mld->avoid_efi_boot_services);
if (load_size > mld->max_addr || mld->min_addr > mld->max_addr -
load_size)
- return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or
load size");
+ {
+ phdr_free();
+ return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address
and/or load size");
+ }
err = grub_relocator_alloc_chunk_align (GRUB_MULTIBOOT
(relocator), &ch,
mld->min_addr, mld->max_addr - load_size,
@@ -117,6 +148,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
*mld)
if (err)
{
grub_dprintf ("multiboot_loader", "Cannot allocate memory
for OS image\n");
+ phdr_free();
return err;
}
@@ -152,6 +184,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
*mld)
if (err)
{
grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS
image\n");
+ phdr_free();
return err;
}
@@ -162,7 +195,10 @@ CONCAT(grub_multiboot_load_elf, XX)
(mbi_load_data_t *mld)
{
if (grub_file_seek (mld->file, (grub_off_t) phdr(i)->p_offset)
== (grub_off_t) -1)
- return grub_errno;
+ {
+ phdr_free();
+ return grub_errno;
+ }
if (grub_file_read (mld->file, (grub_uint8_t *) source +
load_offset, phdr(i)->p_filesz)
!= (grub_ssize_t) phdr(i)->p_filesz)
@@ -170,6 +206,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
*mld)
if (!grub_errno)
grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file
%s"),
mld->filename);
+ phdr_free();
return grub_errno;
}
}
@@ -191,19 +228,28 @@ CONCAT(grub_multiboot_load_elf, XX)
(mbi_load_data_t *mld)
/* We still in 32-bit mode. */
if ((ehdr->e_entry - phdr(i)->p_vaddr)
+ phdr(i)->p_paddr < 0xffffffff80000000ULL)
- return grub_error (GRUB_ERR_BAD_OS, "invalid entry point for ELF64");
+ {
+ phdr_free();
+ return grub_error (GRUB_ERR_BAD_OS, "invalid entry point for ELF64");
+ }
# else
/* We still in 32-bit mode. */
if ((ehdr->e_entry - phdr(i)->p_vaddr)
+ phdr(i)->p_paddr > 0xffffffff)
- return grub_error (GRUB_ERR_BAD_OS, "invalid entry point for ELF64");
+ {
+ phdr_free();
+ return grub_error (GRUB_ERR_BAD_OS, "invalid entry point for ELF64");
+ }
# endif
#endif
break;
}
if (i == ehdr->e_phnum)
- return grub_error (GRUB_ERR_BAD_OS, "entry point isn't in a segment");
+ {
+ phdr_free();
+ return grub_error (GRUB_ERR_BAD_OS, "entry point isn't in a
segment");
+ }
#if defined (__i386__) || defined (__x86_64__)
@@ -219,11 +265,15 @@ CONCAT(grub_multiboot_load_elf, XX)
(mbi_load_data_t *mld)
shdr = grub_malloc ((grub_uint32_t) ehdr->e_shnum *
ehdr->e_shentsize);
if (!shdr)
- return grub_errno;
+ {
+ phdr_free();
+ return grub_errno;
+ }
if (grub_file_seek (mld->file, ehdr->e_shoff) == (grub_off_t) -1)
{
grub_free (shdr);
+ phdr_free();
return grub_errno;
}
@@ -233,6 +283,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
*mld)
if (!grub_errno)
grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file %s"),
mld->filename);
+ phdr_free();
return grub_errno;
}
@@ -244,7 +295,10 @@ CONCAT(grub_multiboot_load_elf, XX)
(mbi_load_data_t *mld)
grub_addr_t target;
if (mld->mbi_ver >= 2 && (sh->sh_type == SHT_REL || sh->sh_type ==
SHT_RELA))
- return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "ELF files with
relocs are not supported yet");
+ {
+ phdr_free();
+ return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "ELF files with
relocs are not supported yet");
+ }
/* This section is a loaded section,
so we don't care. */
@@ -263,13 +317,17 @@ CONCAT(grub_multiboot_load_elf, XX)
(mbi_load_data_t *mld)
if (err)
{
grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);
+ phdr_free();
return err;
}
src = get_virtual_current_address (ch);
target = get_physical_target_address (ch);
if (grub_file_seek (mld->file, sh->sh_offset) == (grub_off_t) -1)
- return grub_errno;
+ {
+ phdr_free();
+ return grub_errno;
+ }
if (grub_file_read (mld->file, src, sh->sh_size)
!= (grub_ssize_t) sh->sh_size)
@@ -277,6 +335,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
*mld)
if (!grub_errno)
grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file %s"),
mld->filename);
+ phdr_free();
return grub_errno;
}
sh->sh_addr = target;
@@ -285,7 +344,10 @@ CONCAT(grub_multiboot_load_elf, XX)
(mbi_load_data_t *mld)
ehdr->e_shstrndx, shdr);
}
+ phdr_free();
+
#undef phdr
+#undef phdr_free
return grub_errno;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] multiboot: Add support for program headers at "strange" locations
2020-05-23 15:30 [PATCH] multiboot: Add support for program headers at "strange" locations Jacob Paul
@ 2020-05-23 15:37 ` John Paul Adrian Glaubitz
0 siblings, 0 replies; 2+ messages in thread
From: John Paul Adrian Glaubitz @ 2020-05-23 15:37 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: Jacob Paul
On 5/23/20 5:30 PM, Jacob Paul via Grub-devel wrote:
> Currently, grub_multiboot_load_elf() will throw an error if the program header is at a too high offset (since it would be outside the allocated buffer space of MULTIBOOT_SEARCH bytes). It is understandable why it does this, since 99.99% of all cases e_phoff has a value of 0x34 or 0x40. So, to be able to support a (uncommon) circumstance like this, the program header can be loaded into a separate buffer if needed, and then freed at the end. The only downside to this is that grub_multiboot_load_elf() must at every return statement check if phdr_base is a separate buffer and if so de-allocate it.
I would suggest adding newlines in the description as otherwise the text might
end in one line in the actual commit message.
Although I'm not sure whether "git-am" adds automatic line breaks.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-23 15:37 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-23 15:30 [PATCH] multiboot: Add support for program headers at "strange" locations Jacob Paul
2020-05-23 15:37 ` John Paul Adrian Glaubitz
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.