File shim-bsc1088585-handle-mok-allocations-better.p... of Package shim

Overview Repositories Revisions Requests Users Attributes Meta

File shim-bsc1088585-handle-mok-allocations-better.patch of Package shim (Revision 340c2ff90118e3ebb7f48d0194ade7ba)

Currently displaying revision 340c2ff90118e3ebb7f48d0194ade7ba , Show latest

From c232e8577b0608664fd4ce7a6b24b8ed7d2fc7a4 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Wed, 27 Sep 2017 14:17:20 -0400
Subject: [PATCH] MokManager: handle mok parameter allocations better.

Covscan daftly claims:

288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330                if (MokSB) {
2331                        menu_strings[i] = L"Change Secure Boot state";
2332                        menu_item[i] = MOK_CHANGE_SB;
2333                        i++;
2334                }
2335
...
2358                choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359                if (choice < 0)
2360                        goto out;
...
2362                switch (menu_item[choice]) {
...
2395                case MOK_CHANGE_SB:
    CID 182841 (#1 of 1): Dereference after null check
    (FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
    mok_sb_prompt, which dereferences it. [show details]
2396                        efi_status = mok_sb_prompt(MokSB, MokSBSize);

Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL.  And then:

252. Condition efi_status == 0, taking true branch.
2397                        if (efi_status == EFI_SUCCESS)
2398                                MokSB = NULL;

This guarantees it won't be in the list the next time through the loop.

This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.

Also do the same thing for all of:
	MOK_CHANGE_SB
	MOK_SET_PW
	MOK_CHANGE_DB
	MOK_ENROLL_MOKX
	MOK_DELETE_MOKX

I also Lindent-ed everything I had to touch.

Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
   through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
   FreePool() on their respective variables (MokSB, etc), and
   check_mok_request() also calls FreePool() on these.  This sounds
   horrible, but it turns out it's not an issue, because they only free
   them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
   system if any of the mok_XX_prompt() calls actually returned
   EFI_SUCCESS, so we never get back to check_mok_request() for it to do
   its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
   the call to free_menu(), but we also can't hit this bug, because all
   the exit paths from the loop are "goto out" (or return error) rather
   than actually exiting on the loop conditional.

Signed-off-by: Peter Jones <pjones@redhat.com>
(cherry picked from commit a32651360552559ee6a8978b5bcdc6e7dcc72b8c)
Gary Lin: Fixed the conflict against shim 14.
---
 MokManager.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/MokManager.c b/MokManager.c
index 55af321..42bf72d 100644
--- a/MokManager.c
+++ b/MokManager.c
@@ -1060,9 +1060,6 @@ static EFI_STATUS mok_enrollment_prompt (void *MokNew, UINTN MokNewSize, int aut
 		}
 	}
 
-	if (MokNew)
-		FreePool (MokNew);
-
 	return EFI_SUCCESS;
 }
 
@@ -1609,9 +1606,6 @@ static EFI_STATUS mok_sb_prompt (void *MokSB, UINTN MokSBSize) {
 		}
 	}
 
-	if (MokSB)
-		FreePool(MokSB);
-
 	return EFI_SUCCESS;
 }
 
@@ -1729,9 +1723,6 @@ static EFI_STATUS mok_db_prompt (void *MokDB, UINTN MokDBSize) {
 		}
 	}
 
-	if (MokDB)
-		FreePool(MokDB);
-
 	return EFI_SUCCESS;
 }
 
@@ -1800,9 +1791,6 @@ static EFI_STATUS mok_pw_prompt (void *MokPW, UINTN MokPWSize) {
 mokpw_done:
 	LibDeleteVariable(L"MokPW", &shim_lock_guid);
 
-	if (MokPW)
-		FreePool(MokPW);
-
 	return EFI_SUCCESS;
 }
 
@@ -2184,8 +2172,8 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle,
 				 void *MokXNew, UINTN MokXNewSize,
 				 void *MokXDel, UINTN MokXDelSize)
 {
-	CHAR16 **menu_strings;
-	mok_menu_item *menu_item;
+	CHAR16 **menu_strings = NULL;
+	mok_menu_item *menu_item = NULL;
 	int choice = 0;
 	int mok_changed = 0;
 	EFI_STATUS efi_status;
@@ -2357,11 +2345,23 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle,
 			efi_status = mok_reset_prompt(FALSE);
 			break;
 		case MOK_ENROLL_MOK:
+			if (!MokNew) {
+				Print(L"MokManager: internal error: %s",
+				      L"MokNew was !NULL but is now NULL\n");
+				ret = EFI_ABORTED;
+				goto out;
+			}
 			efi_status = mok_enrollment_prompt(MokNew, MokNewSize, TRUE, FALSE);
 			if (efi_status == EFI_SUCCESS)
 				MokNew = NULL;
 			break;
 		case MOK_DELETE_MOK:
+			if (!MokDel) {
+				Print(L"MokManager: internal error: %s",
+				      L"MokDel was !NULL but is now NULL\n");
+				ret = EFI_ABORTED;
+				goto out;
+			}
 			efi_status = mok_deletion_prompt(MokDel, MokDelSize, FALSE);
 			if (efi_status == EFI_SUCCESS)
 				MokDel = NULL;
@@ -2370,26 +2370,56 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle,
 			efi_status = mok_reset_prompt(TRUE);
 			break;
 		case MOK_ENROLL_MOKX:
+			if (!MokXNew) {
+				Print(L"MokManager: internal error: %s",
+				      L"MokXNew was !NULL but is now NULL\n");
+				ret = EFI_ABORTED;
+				goto out;
+			}
 			efi_status = mok_enrollment_prompt(MokXNew, MokXNewSize, TRUE, TRUE);
 			if (efi_status == EFI_SUCCESS)
 				MokXNew = NULL;
 			break;
 		case MOK_DELETE_MOKX:
+			if (!MokXDel) {
+				Print(L"MokManager: internal error: %s",
+				      L"MokXDel was !NULL but is now NULL\n");
+				ret = EFI_ABORTED;
+				goto out;
+			}
 			efi_status = mok_deletion_prompt(MokXDel, MokXDelSize, TRUE);
 			if (efi_status == EFI_SUCCESS)
 				MokXDel = NULL;
 			break;
 		case MOK_CHANGE_SB:
+			if (!MokSB) {
+				Print(L"MokManager: internal error: %s",
+				      L"MokSB was !NULL but is now NULL\n");
+				ret = EFI_ABORTED;
+				goto out;
+			}
 			efi_status = mok_sb_prompt(MokSB, MokSBSize);
 			if (efi_status == EFI_SUCCESS)
 				MokSB = NULL;
 			break;
 		case MOK_SET_PW:
+			if (!MokPW) {
+				Print(L"MokManager: internal error: %s",
+				      L"MokPW was !NULL but is now NULL\n");
+				ret = EFI_ABORTED;
+				goto out;
+			}
 			efi_status = mok_pw_prompt(MokPW, MokPWSize);
 			if (efi_status == EFI_SUCCESS)
 				MokPW = NULL;
 			break;
 		case MOK_CHANGE_DB:
+			if (!MokDB) {
+				Print(L"MokManager: internal error: %s",
+				      L"MokDB was !NULL but is now NULL\n");
+				ret = EFI_ABORTED;
+				goto out;
+			}
 			efi_status = mok_db_prompt(MokDB, MokDBSize);
 			if (efi_status == EFI_SUCCESS)
 				MokDB = NULL;
@@ -2406,6 +2436,8 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle,
 			mok_changed = 1;
 
 		free_menu(menu_item, menu_strings);
+		menu_item = NULL;
+		menu_strings = NULL;
 	}
 
 out:
-- 
2.16.2

Places

File shim-bsc1088585-handle-mok-allocations-better.patch of Package shim (Revision 340c2ff90118e3ebb7f48d0194ade7ba)

Places