From 50ae3bd11b51f8e6cd86d4b4f2c8322a7036d095 Mon Sep 17 00:00:00 2001 From: Lenny Szubowicz Date: Tue, 6 Jan 2015 11:17:01 -0500 Subject: [PATCH] Fix buffer overflow when remove_from_boot_order removes nothing Deleting a boot entry via "-b xxxx -B" also attempts to remove that entry from boot order via a call to remove_from_boot_order. Although unusual, it's possible that the entry being deleted is not in boot order. Correct the handling of this case in remove_from_boot_order, which malloc's space for the new boot order list wrongly assuming that at least one entry will be removed. However, if no entry is removed, then 2 bytes are overwritten beyond the malloc'ed space. This can result in heap corruption and possible termination via a SIGABRT if the corruption is detected by the heap allocation routines. While there, simplify the routine to do the removal of boot entries in place in the original data buffer, skip the unnecessary BootOrder variable update if nothing got removed, and free the malloc'ed boot_order struct on the way out. Resolves: RH BZ 1168019 Signed-off-by: Lenny Szubowicz --- src/efibootmgr/efibootmgr.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/efibootmgr/efibootmgr.c b/src/efibootmgr/efibootmgr.c index eb13942..1b55125 100644 --- a/src/efibootmgr/efibootmgr.c +++ b/src/efibootmgr/efibootmgr.c @@ -429,8 +429,7 @@ static int remove_from_boot_order(uint16_t num) { efi_variable_t *boot_order = NULL; - uint64_t new_data_size; - uint16_t *new_data, *old_data; + uint16_t *data; unsigned int old_i,new_i; int rc; @@ -442,34 +441,32 @@ remove_from_boot_order(uint16_t num) } /* We've now got an array (in boot_order->data) of the - boot order. Simply copy the array, skipping the - entry we're deleting. + boot order. Squeeze out any instance of the entry we're + deleting by shifting the remainder down. */ - old_data = (uint16_t *)(boot_order->data); - /* Start with the same size */ - new_data_size = boot_order->data_size - sizeof (*new_data); - new_data = malloc(new_data_size); - if (!new_data) - return -1; + data = (uint16_t *)(boot_order->data); for (old_i=0,new_i=0; - old_i < boot_order->data_size / sizeof(*new_data); + old_i < boot_order->data_size / sizeof(data[0]); old_i++) { - if (old_data[old_i] != num) { - /* Copy this value */ - new_data[new_i] = old_data[old_i]; + if (data[old_i] != num) { + if (new_i != old_i) + data[new_i] = data[old_i]; new_i++; } } - /* Now new_data has what we need */ - free(boot_order->data); - boot_order->data = (uint8_t *)new_data; - boot_order->data_size = new_data_size; + /* If nothing removed, no need to update the BootOrder variable */ + if (new_i == old_i) + goto all_done; + + /* BootOrder variable needs to be updated */ efi_del_variable(EFI_GLOBAL_GUID, "BootOrder"); rc = efi_set_variable(EFI_GLOBAL_GUID, "BootOrder", boot_order->data, boot_order->data_size, boot_order->attributes); +all_done: free(boot_order->data); + free(boot_order); return rc; } -- 2.1.0