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>

1 files changed, 46 insertions, 14 deletions

diff --git a/MokManager.c b/MokManager.c
index af5777bd..16cf86fe 100644
--- a/MokManager.c
+++ b/MokManager.c
@@ -1038,9 +1038,6 @@ static EFI_STATUS mok_enrollment_prompt(void *MokNew, UINTN MokNewSize,
 		}
 	}
 
-	if (MokNew)
-		FreePool(MokNew);
-
 	return EFI_SUCCESS;
 }
 
@@ -1582,9 +1579,6 @@ static EFI_STATUS mok_sb_prompt(void *MokSB, UINTN MokSBSize)
 		}
 	}
 
-	if (MokSB)
-		FreePool(MokSB);
-
 	return EFI_SUCCESS;
 }
 
@@ -1704,9 +1698,6 @@ static EFI_STATUS mok_db_prompt(void *MokDB, UINTN MokDBSize)
 		}
 	}
 
-	if (MokDB)
-		FreePool(MokDB);
-
 	return EFI_SUCCESS;
 }
 
@@ -1776,9 +1767,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;
 }
 
@@ -2156,8 +2144,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;
@@ -2328,12 +2316,24 @@ 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_ERROR(efi_status))
 				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_ERROR(efi_status))
@@ -2343,28 +2343,58 @@ 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_ERROR(efi_status))
 				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_ERROR(efi_status))
 				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_ERROR(efi_status))
 				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_ERROR(efi_status))
 				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_ERROR(efi_status))
 				MokDB = NULL;
@@ -2381,6 +2411,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: