From ba13b5a7ccb4141e9918caf1c0272a5d72c4da61 Mon Sep 17 00:00:00 2001
From: Lenny Szubowicz <lszubowi@redhat.com>
Date: Tue, 6 Jan 2015 11:17:01 -0500
Subject: [PATCH 21/31] 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 <lszubowi@redhat.com>
---
 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.7.4