[PATCH 4/4] efi: Fix use-after-free in finish boot services
In grub-core/kern/efi/mm.c, grub_efi_finish_boot_services() has an instance
where the memory for the variable finish_mmap_buf is freed, but on the next
iteration of a while loop, grub_efi_get_memory_map() uses finish_mmap_buf. To
prevent this, we can set finish_mmap_buf to NULL after the free.
Signed-off-by: Alec Brown
---
grub-core/kern/efi/mm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 3705b8b1b..c74ccbb05 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -263,6 +263,7 @@ grub_efi_finish_boot_services (grub_efi_uintn_t
*outbuf_size, void *outbuf,
&finish_desc_size, &finish_desc_version) <=
0)
{
grub_free (finish_mmap_buf);
+ finish_mmap_buf = NULL;
return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
}
@@ -275,10 +276,12 @@ grub_efi_finish_boot_services (grub_efi_uintn_t
*outbuf_size, void *outbuf,
if (status != GRUB_EFI_INVALID_PARAMETER)
{
grub_free (finish_mmap_buf);
+ finish_mmap_buf = NULL;
return grub_error (GRUB_ERR_IO, "couldn't terminate EFI services");
}
grub_free (finish_mmap_buf);
+ finish_mmap_buf = NULL;
grub_printf ("Trying to terminate EFI services again\n");
}
grub_efi_is_finished = 1;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/4] elf: check program header offset doesn't exceed constraints
In grub-core/loader/multiboot_elfxx.c, we need to make sure that the program header offset is less than the file size along with the MULTIBOOT_SEARCH constant. We can do so by setting the variable phlimit to the minimum value of the two limits and check it each time we change program header index to insure that the program header offset isn't outside of the limits. Fixes: CID 314029 Fixes: CID 314038 Signed-off-by: Alec Brown --- grub-core/loader/multiboot_elfxx.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c index 52eaf9184..72b376cd5 100644 --- a/grub-core/loader/multiboot_elfxx.c +++ b/grub-core/loader/multiboot_elfxx.c @@ -70,6 +70,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld) grub_uint32_t load_offset = 0, load_size = 0; Elf_Shnum shnum; Elf_Word shstrndx, phnum; + grub_off_t phlimit; unsigned int i; void *source = NULL; @@ -100,7 +101,8 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld) return err; /* FIXME: Should we support program headers at strange locations? */ - if (ehdr->e_phoff + phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH) + phlimit = grub_min (MULTIBOOT_SEARCH, grub_file_size (mld->file)); + if ((grub_off_t) ehdr->e_phoff + phnum * ehdr->e_phentsize > phlimit) return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset"); phdr_base = (char *) mld->buffer + ehdr->e_phoff; -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/4] elf: Check program memory isn't larger than allocated memory size
In grub-core/loader/multiboot_elfxx.c, the code is filling an area of memory
with grub_memset() but doesn't check if there is space in the allocated memory
before doing so. To make sure we aren't zeroing memory past the allocated memory
region, we need to check that the offset into the allocated memory region plus
the memory size of the program is smaller than the allocated memory size.
Fixes: CID 314029
Fixes: CID 314038
Signed-off-by: Alec Brown
---
grub-core/loader/multiboot_elfxx.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/grub-core/loader/multiboot_elfxx.c
b/grub-core/loader/multiboot_elfxx.c
index e067d2d84..69f567588 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -67,7 +67,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
char *phdr_base;
grub_err_t err;
grub_relocator_chunk_t ch;
- grub_uint32_t load_offset = 0, load_size;
+ grub_uint32_t load_offset = 0, load_size = 0;
Elf_Shnum shnum;
Elf_Word shstrndx, phnum;
unsigned int i;
@@ -170,8 +170,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
}
else
{
+ load_size = phdr(i)->p_memsz;
err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT
(relocator), &ch,
-phdr(i)->p_paddr,
phdr(i)->p_memsz);
+phdr(i)->p_paddr,
load_size);
if (err != GRUB_ERR_NONE)
{
@@ -199,8 +200,14 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
}
if (phdr(i)->p_filesz < phdr(i)->p_memsz)
-grub_memset ((grub_uint8_t *) source + load_offset +
phdr(i)->p_filesz, 0,
-phdr(i)->p_memsz - phdr(i)->p_filesz);
+ {
+ /* Need to insure that the memory being set isn't larger than the
allocated memory*/
+ if (load_offset + phdr(i)->p_memsz - phdr(i)->p_filesz >
load_size)
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("memory being set
is larger than allocated memory"));
+
+ grub_memset ((grub_uint8_t *) source + load_offset +
phdr(i)->p_filesz, 0,
+ phdr(i)->p_memsz - phdr(i)->p_filesz);
+ }
}
}
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/4] elf: Check section header region before allocating memory
In grub-core/loader/multiboot_elfxx.c, space is being allocated for the section
header region, but isn't verifying if the region is within the file's size.
Before calling grub_calloc(), we can add a conditional to check if the section
header region is smaller than the file size.
Fixes: CID 314029
Fixes: CID 314038
Signed-off-by: Alec Brown
---
grub-core/loader/multiboot_elfxx.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/grub-core/loader/multiboot_elfxx.c
b/grub-core/loader/multiboot_elfxx.c
index 69f567588..52eaf9184 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -248,6 +248,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
{
grub_uint8_t *shdr, *shdrptr;
+ if ((grub_off_t) ehdr->e_shoff + shnum * ehdr->e_shentsize >
grub_file_size (mld->file))
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("ELF section header region
is larger than the file size"));
+
shdr = grub_calloc (shnum, ehdr->e_shentsize);
if (!shdr)
return grub_errno;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/4] Address coverity untrusted loop bound bugs in multiboot_elfxx.c
Coverity has listed two untrusted loop bound bugs in grub-core/loader/multiboot_elfxx.c. They are CID 314029 and CID 314038. After testing the first patch, the CID changed to an untrusted loop bound for line 244: shdr = grub_calloc (shnum, ehdr->e_shentsize);. I added a second patch to address this, but after making these changes, it reverted to the original bug of using tainted data in grub_memset(). The third patch addresses Coverity's issue with phdr() in grub_memset() and reduces the bug to only having an issue with using phnum as an untrusted loop bound. However, we can ignore this since phnum is already getting checked earlier in the function. I've also bundled a use-after-free patch with this patch set at the end. Alec Brown (4): elf: Check program memory isn't larger than allocated memory size elf: Check section header region before allocating memory elf: check program header offset doesn't exceed constraints efi: Fix use-after-free in finish boot services grub-core/kern/efi/mm.c| 3 +++ grub-core/loader/multiboot_elfxx.c | 22 +- 2 files changed, 20 insertions(+), 5 deletions(-) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/1] xfs: Fix issues found while fuzzing the XFS filesystem
From: Darren Kenny
While performing fuzz testing with XFS filesystem images with ASAN
enabled, several issues were found where the memory accesses are made
beyond the data that is allocated into the struct grub_xfs_data
structure's data field.
The existing stucture didn't store the size of the memory allocted into
the buffer in the data field and had no way to check it. To resolve
these issues, the data size is stored to enable accesses into the data
buffer.
With these checks in place, the fuzzing corpus no longer cause any
crashes.
Signed-off-by: Darren Kenny
Signed-off-by: Robbie Harwood
Signed-off-by: Marta Lewandowska
Signed-off-by: Lidong Chen
---
grub-core/fs/xfs.c | 32
1 file changed, 32 insertions(+)
diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index d6de7f1a2..a0aaa3aa8 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -239,6 +239,7 @@ struct grub_fshelp_node
struct grub_xfs_data
{
+ grub_size_t datasize;
struct grub_xfs_sblock sblock;
grub_disk_t disk;
int pos;
@@ -608,8 +609,20 @@ grub_xfs_read_block (grub_fshelp_node_t node,
grub_disk_addr_t fileblock)
}
else if (node->inode.format == XFS_INODE_FORMAT_EXT)
{
+ grub_addr_t exts_end = 0;
+ grub_addr_t data_end = 0;
+
nrec = grub_be_to_cpu32 (node->inode.nextents);
exts = (struct grub_xfs_extent *) grub_xfs_inode_data(&node->inode);
+
+ if (grub_mul(sizeof(struct grub_xfs_extent), nrec, &exts_end) ||
+ grub_add((grub_addr_t)node->data, exts_end, &exts_end) ||
+ grub_add((grub_addr_t)node->data, node->data->datasize, &data_end) ||
+ exts_end > data_end)
+{
+ grub_error (GRUB_ERR_BAD_FS, "Invalid number of XFS exts");
+ return 0;
+}
}
else
{
@@ -799,6 +812,12 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
grub_uint8_t *inopos = grub_xfs_inline_de_inopos(dir->data, de);
grub_uint8_t c;
+ if ((inopos + (smallino?4:8)) >
+ (grub_uint8_t*)dir + grub_xfs_fshelp_size(dir->data))
+ {
+ return grub_error (GRUB_ERR_BAD_FS, "not a correct XFS inode");
+ }
+
/* inopos might be unaligned. */
if (smallino)
ino = (((grub_uint32_t) inopos[0]) << 24)
@@ -825,6 +844,12 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
de->name[de->len] = c;
de = grub_xfs_inline_next_de(dir->data, head, de);
+
+ if ((grub_uint8_t*)de >= (grub_uint8_t*)dir +
grub_xfs_fshelp_size(dir->data))
+ {
+ return grub_error (GRUB_ERR_BAD_FS, "invalid XFS directory
entry");
+ }
+
}
break;
}
@@ -890,6 +915,10 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
}
filename = (char *)(direntry + 1);
+ if (filename + direntry->len - 1 > (char *) tail)
+ {
+ return grub_error (GRUB_ERR_BAD_FS, "invalid XFS directory
entry");
+ }
/* The byte after the filename is for the filetype, padding, or
tag, which is not used by GRUB. So it can be overwritten. */
filename[direntry->len] = '\0';
@@ -934,6 +963,8 @@ grub_xfs_mount (grub_disk_t disk)
if (!data)
return 0;
+ data->datasize = sizeof (struct grub_xfs_data);
+
grub_dprintf("xfs", "Reading sb\n");
/* Read the superblock. */
if (grub_disk_read (disk, 0, 0,
@@ -955,6 +986,7 @@ grub_xfs_mount (grub_disk_t disk)
if (! data)
goto fail;
+ data->datasize = sz;
data->diropen.data = data;
data->diropen.ino = grub_be_to_cpu64(data->sblock.rootino);
data->diropen.inode_read = 1;
--
2.39.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
