File shim-bsc1088585-handle-mok-allocations-better.patch of Package shim.7637
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
