From a32651360552559ee6a8978b5bcdc6e7dcc72b8c Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 27 Sep 2017 14:17:20 -0400 Subject: 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 --- MokManager.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-------------- 1 file 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: -- cgit v1.2.3