On 3/28/2023 12:24 PM, Michael Kubacki wrote:
Hi Oliver,Thanks for taking time to look through the series. I responded inline. Thanks, Michael On 3/28/2023 2:49 PM, Oliver Smith-Denny wrote:Yes, but that's outside the scope of this patch. If someone were to fix that, they should also address the other ~20 places in the file that use asserts in place of checking null properly.A couple comments below, thanks! On 3/24/2023 3:30 PM, Michael Kubacki wrote:From: Michael Kubacki <michael.kuba...@microsoft.com> Fixes CodeQL alerts for CWE-457: https://cwe.mitre.org/data/definitions/457.html Cc: Erich McMillan <emcmil...@microsoft.com> Cc: Michael D Kinney <michael.d.kin...@intel.com> Cc: Michael Kubacki <mikub...@linux.microsoft.com> Cc: Ray Ni <ray...@intel.com> Cc: Zhichao Gao <zhichao....@intel.com> Co-authored-by: Erich McMillan <emcmil...@microsoft.com> Signed-off-by: Michael Kubacki <michael.kuba...@microsoft.com> Reviewed-by: Zhichao Gao <zhichao....@intel.com> --- ShellPkg/Application/Shell/Shell.c | 1 +ShellPkg/Application/Shell/ShellProtocol.c | 60 ++++++++++---------- ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c | 56 +++++++++--------- ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c | 18 +++---ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c | 9 ++- ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 14 +++--ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c | 17 ++++-- ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c | 21 +++----8 files changed, 107 insertions(+), 89 deletions(-)diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.cindex 0ae6e14a34bf..f95c799bb2a4 100644 --- a/ShellPkg/Application/Shell/Shell.c +++ b/ShellPkg/Application/Shell/Shell.c @@ -1300,6 +1300,7 @@ DoStartupScript ( CHAR16 *FullFileStringPath; UINTN NewSize; + CalleeStatus = EFI_SUCCESS; Key.UnicodeChar = CHAR_NULL; Key.ScanCode = 0;diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.cindex e6d20ab16479..da8c31cb038a 100644 --- a/ShellPkg/Application/Shell/ShellProtocol.c +++ b/ShellPkg/Application/Shell/ShellProtocol.c @@ -735,50 +735,52 @@ EfiShellGetDeviceName ( // // Now check the parent controller using this as the child. // - if (DeviceNameToReturn == NULL) {- PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, &ParentControllerCount, &ParentControllerBuffer); + Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, &ParentControllerCount, &ParentControllerBuffer);+ if ((DeviceNameToReturn == NULL) && !EFI_ERROR (Status)) { for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {- PARSE_HANDLE_DATABASE_UEFI_DRIVERS (ParentControllerBuffer[LoopVar], &ParentDriverCount, &ParentDriverBuffer); - for (HandleCount = 0; HandleCount < ParentDriverCount; HandleCount++) {- //- // try using that driver's component name with controller and our driver as the child.- // - Status = gBS->OpenProtocol ( - ParentDriverBuffer[HandleCount], - &gEfiComponentName2ProtocolGuid, - (VOID **)&CompName2, - gImageHandle, - NULL, - EFI_OPEN_PROTOCOL_GET_PROTOCOL - ); - if (EFI_ERROR (Status)) {+ Status = PARSE_HANDLE_DATABASE_UEFI_DRIVERS (ParentControllerBuffer[LoopVar], &ParentDriverCount, &ParentDriverBuffer);+ if (!EFI_ERROR (Status)) {+ for (HandleCount = 0; HandleCount < ParentDriverCount; HandleCount++) {+ //+ // try using that driver's component name with controller and our driver as the child.+ // Status = gBS->OpenProtocol ( ParentDriverBuffer[HandleCount], - &gEfiComponentNameProtocolGuid, + &gEfiComponentName2ProtocolGuid, (VOID **)&CompName2, gImageHandle, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL ); - } + if (EFI_ERROR (Status)) { + Status = gBS->OpenProtocol ( + ParentDriverBuffer[HandleCount], + &gEfiComponentNameProtocolGuid, + (VOID **)&CompName2, + gImageHandle, + NULL, + EFI_OPEN_PROTOCOL_GET_PROTOCOL + ); + } + + if (EFI_ERROR (Status)) { + continue; + } - if (EFI_ERROR (Status)) { - continue;+ Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE); + Status = CompName2->GetControllerName (CompName2, ParentControllerBuffer[LoopVar], DeviceHandle, Lang, &DeviceNameToReturn);+ FreePool (Lang); + Lang = NULL; + if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) { + break; + } }- Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE); - Status = CompName2->GetControllerName (CompName2, ParentControllerBuffer[LoopVar], DeviceHandle, Lang, &DeviceNameToReturn);- FreePool (Lang); - Lang = NULL; + SHELL_FREE_NON_NULL (ParentDriverBuffer); if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) { break; } } - - SHELL_FREE_NON_NULL (ParentDriverBuffer); - if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) { - break; - } } SHELL_FREE_NON_NULL (ParentControllerBuffer);diff --git a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.cindex 36cf46fb2c38..4549cbde9b9a 100644 --- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c +++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c @@ -1399,10 +1399,11 @@ ShellCommandCreateInitialMappingsAndPaths ( CHAR16 *MapName; SHELL_MAP_LIST *MapListItem; - SplitCurDir = NULL; - MapName = NULL; - MapListItem = NULL; - HandleList = NULL; + ConsistMappingTable = NULL; + SplitCurDir = NULL; + MapName = NULL; + MapListItem = NULL; + HandleList = NULL; // // Reset the static members back to zero @@ -1458,32 +1459,35 @@ ShellCommandCreateInitialMappingsAndPaths ( //PerformQuickSort (DevicePathList, Count, sizeof (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);- ShellCommandConsistMappingInitialize (&ConsistMappingTable); - // - // Assign new Mappings to all... - // - for (Count = 0; HandleList[Count] != NULL; Count++) {+ if (!EFI_ERROR (ShellCommandConsistMappingInitialize (&ConsistMappingTable))) {// - // Get default name first + // Assign new Mappings to all... //- NewDefaultName = ShellCommandCreateNewMappingName (MappingTypeFileSystem);- ASSERT (NewDefaultName != NULL);Should we be checking for NewDefaultName to not be NULL before doing the below function call? Looks like ShellCommandAddMapItemAndUpdatePath does not do NULL checking and directly uses the Name (where it would fail in StrSize()). Similarly we then call FreePool on it, even if it is NULL.
Fair enough :)
- Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName, DevicePathList[Count], 0, TRUE);- ASSERT_EFI_ERROR (Status); - FreePool (NewDefaultName); - - // - // Now do consistent name - //- NewConsistName = ShellCommandConsistMappingGenMappingName (DevicePathList[Count], ConsistMappingTable);- if (NewConsistName != NULL) {- Status = ShellCommandAddMapItemAndUpdatePath (NewConsistName, DevicePathList[Count], 0, FALSE);+ for (Count = 0; HandleList[Count] != NULL; Count++) { + // + // Get default name first + //+ NewDefaultName = ShellCommandCreateNewMappingName (MappingTypeFileSystem);+ ASSERT (NewDefaultName != NULL);+ Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName, DevicePathList[Count], 0, TRUE);ASSERT_EFI_ERROR (Status); - FreePool (NewConsistName); + FreePool (NewDefaultName); + + // + // Now do consistent name + //+ NewConsistName = ShellCommandConsistMappingGenMappingName (DevicePathList[Count], ConsistMappingTable);+ if (NewConsistName != NULL) {+ Status = ShellCommandAddMapItemAndUpdatePath (NewConsistName, DevicePathList[Count], 0, FALSE);+ ASSERT_EFI_ERROR (Status); + FreePool (NewConsistName); + } } } - ShellCommandConsistMappingUnInitialize (ConsistMappingTable); + if (ConsistMappingTable != NULL) { + ShellCommandConsistMappingUnInitialize (ConsistMappingTable); + } SHELL_FREE_NON_NULL (HandleList); SHELL_FREE_NON_NULL (DevicePathList); @@ -1626,12 +1630,12 @@ ShellCommandUpdateMapping ( //PerformQuickSort (DevicePathList, Count, sizeof (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);- ShellCommandConsistMappingInitialize (&ConsistMappingTable);+ Status = ShellCommandConsistMappingInitialize (&ConsistMappingTable);// // Assign new Mappings to remainders //- for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL && !EFI_ERROR (Status); Count++) { + for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL; Count++) {// // Skip ones that already have //diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.cindex 97a4b57a932f..5329b559ba46 100644 --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c @@ -158,7 +158,10 @@ ShellCommandRunDblk ( ShellStatus = SHELL_INVALID_PARAMETER; } - ShellConvertStringToUint64 (LbaString, &Lba, TRUE, FALSE);+ if (EFI_ERROR (ShellConvertStringToUint64 (LbaString, &Lba, TRUE, FALSE))) { + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), gShellDebug1HiiHandle, L"dblk", LbaString);+ ShellStatus = SHELL_INVALID_PARAMETER; + } } if (BlockCountString == NULL) { @@ -169,12 +172,13 @@ ShellCommandRunDblk ( ShellStatus = SHELL_INVALID_PARAMETER; }- ShellConvertStringToUint64 (BlockCountString, &BlockCount, TRUE, FALSE);- if (BlockCount > 0x10) { - BlockCount = 0x10; - } else if (BlockCount == 0) {- ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), gShellDebug1HiiHandle, L"dblk", BlockCountString);- ShellStatus = SHELL_INVALID_PARAMETER;+ if (!EFI_ERROR (ShellConvertStringToUint64 (BlockCountString, &BlockCount, TRUE, FALSE))) {+ if (BlockCount > 0x10) { + BlockCount = 0x10; + } else if (BlockCount == 0) {+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), gShellDebug1HiiHandle, L"dblk", BlockCountString);+ ShellStatus = SHELL_INVALID_PARAMETER; + } } }diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.cindex 8bf23a2076a1..72f8c087cb69 100644 --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c @@ -112,10 +112,13 @@ ShellCommandRunEfiDecompress ( if (ShellStatus == SHELL_SUCCESS) { Status = FileHandleGetSize (InFileHandle, &Temp64Bit); - ASSERT (Temp64Bit <= (UINT32)(-1)); - InSize = (UINTN)Temp64Bit; ASSERT_EFI_ERROR (Status); - InBuffer = AllocateZeroPool (InSize); + if (!EFI_ERROR (Status)) {nit: If we got an EFI_ERROR from FileHandleGetSize, we will return EFI_OUT_OF_RESOURCES when we hit InBuffer == NULL below, which is not the real reason we failed. Perhaps we don't care, though.True. The goal here was not to change that behavior.+ ASSERT (Temp64Bit <= (UINT32)(-1)); + InSize = (UINTN)Temp64Bit; + InBuffer = AllocateZeroPool (InSize); + } + if (InBuffer == NULL) { Status = EFI_OUT_OF_RESOURCES; } else {diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.cindex d7a133c0c5b4..870c5b0d1da7 100644 --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c @@ -508,9 +508,10 @@ ShellCommandRunConnect ( Count = ShellCommandLineGetCount (Package); if (Param1 != NULL) {- Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE, FALSE);- Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate); - if (EFI_ERROR (Status)) {+ Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE, FALSE);+ if (!EFI_ERROR (Status)) { + Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate); + } else {ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE), gShellDriver1HiiHandle, L"connect", Param1);ShellStatus = SHELL_INVALID_PARAMETER; } @@ -519,9 +520,10 @@ ShellCommandRunConnect ( } if (Param2 != NULL) {- Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE, FALSE);- Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate); - if (EFI_ERROR (Status)) {+ Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE, FALSE);+ if (!EFI_ERROR (Status)) { + Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate); + } else {ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE), gShellDriver1HiiHandle, L"connect", Param2);ShellStatus = SHELL_INVALID_PARAMETER; }diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.cindex 009ae5282b27..fd49d1f7ceb4 100644 --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c @@ -160,12 +160,17 @@ ShellCommandRunDisconnect ( Param1 = ShellCommandLineGetRawValue (Package, 1); Param2 = ShellCommandLineGetRawValue (Package, 2); Param3 = ShellCommandLineGetRawValue (Package, 3);- ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE); - Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate1) : NULL; - ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE); - Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate2) : NULL; - ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE); - Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate3) : NULL; + if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE))) { + Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate1) : NULL;+ } ++ if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE))) { + Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate2) : NULL;+ } ++ if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE))) { + Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate3) : NULL;+ } if ((Param1 != NULL) && (Handle1 == NULL)) {ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE), gShellDriver1HiiHandle, L"disconnect", Param1); diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.cindex c645c9fd6882..8f70d6b6af39 100644 --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c @@ -438,25 +438,22 @@ ShellCommandRunDrvDiag ( ControllerHandleStr = ShellCommandLineGetRawValue (Package, 2); ChildHandleStr = ShellCommandLineGetRawValue (Package, 3); - if (DriverHandleStr == NULL) { - Handle1 = NULL; - } else {- ShellConvertStringToUint64 (DriverHandleStr, &Intermediate, TRUE, FALSE); + if ((DriverHandleStr != NULL) && ShellConvertStringToUint64 (DriverHandleStr, &Intermediate, TRUE, FALSE)) {Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate); + } else { + Handle1 = NULL; } - if (ControllerHandleStr == NULL) { - Handle2 = NULL; - } else {- ShellConvertStringToUint64 (ControllerHandleStr, &Intermediate, TRUE, FALSE); + if ((ControllerHandleStr != NULL) && ShellConvertStringToUint64 (ControllerHandleStr, &Intermediate, TRUE, FALSE)) {Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate); + } else { + Handle2 = NULL; } - if (ChildHandleStr == NULL) { - Handle3 = NULL; - } else {- ShellConvertStringToUint64 (ChildHandleStr, &Intermediate, TRUE, FALSE); + if ((ChildHandleStr != NULL) && ShellConvertStringToUint64 (ChildHandleStr, &Intermediate, TRUE, FALSE)) {Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate); + } else { + Handle3 = NULL; } Status = DoDiagnostics (
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#102074): https://edk2.groups.io/g/devel/message/102074 Mute This Topic: https://groups.io/mt/97834589/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
