From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout.perfora.net (mout.perfora.net [74.208.4.194]) by mx.groups.io with SMTP id smtpd.web10.4937.1680029384499920747 for ; Tue, 28 Mar 2023 11:49:44 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: smith-denny.com, ip: 74.208.4.194, mailfrom: osd@smith-denny.com) Received: from [10.137.194.171] ([131.107.8.107]) by mrelay.perfora.net (mreueus004 [74.208.5.2]) with ESMTPSA (Nemesis) id 1Ma1LU-1pvXjF2Qjs-00Vveo; Tue, 28 Mar 2023 20:49:25 +0200 Message-ID: <3f708733-a5dd-de89-acdc-d49b9d0eb6b0@smith-denny.com> Date: Tue, 28 Mar 2023 11:49:23 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [edk2-devel] [PATCH v7 09/12] ShellPkg: Fix conditionally uninitialized variables To: devel@edk2.groups.io, mikuback@linux.microsoft.com Cc: Erich McMillan , Michael D Kinney , Ray Ni , Zhichao Gao References: <20230324223034.1560-1-mikuback@linux.microsoft.com> <20230324223034.1560-10-mikuback@linux.microsoft.com> From: "Oliver Smith-Denny" In-Reply-To: <20230324223034.1560-10-mikuback@linux.microsoft.com> X-Provags-ID: V03:K1:u/D6bTsWnRZ93aJaTD5lT434IBj0IHj666Tkuak5NMenDFPOqx3 IB0mMPyeOELfqMdp1P3t7G84twX9dqZ6rJ7scBQXog/6rD+j6jap8bcixaDvlMIRZ+yzCQP c3SUcEAT47vSw/hfiaZBatSwy/Me9x8BENdNmhHt8SO1Vpe+owHSnkRY+d11bCjMGfys59x xfRPettrRvLD8I87sQI+A== X-Spam-Flag: NO UI-OutboundReport: notjunk:1;M01:P0:gVN9yRUGq3A=;bStyMSbyoHV8kwiM/DVDqfQ0x+J 6C5oRNlw0qyxDHEhT/9Dx1kB9qvbPwamPI6AdOrA9AhnV84BUDyr2k/WFyVJn9qUkt2e/WgZQ QOF/StDs0PfRs2YjJNSVKCxdtM6prWRULlQfv73ZIT8X/AE7vIVBGQaNm9vQgU+WtwBK236sa RD9DJitFtQ/5zeqvlr8paodSIkhJhfr+m6VD5o+/OYHnuDIN3owkhEy84RGN33IGGkmCo+cDN sOx9mvTHuYMzJMP2PxccS/WpOJx5fnNhY+3kWV1/4cOfPNgXQGyeSbATfv4sMv5ZpgbzcrbJl fybe+B/+Axb+k6egJ7xYLYEN4db3Aibs6KPXtaQUlR/Q/soK4o2qR+etSeOZURRo+PUbg0ap4 02y7xyLJgi56JNCZQLbMQx5HYF29asTaSiF7s0fu4PjVAKeXZaYZsqU/RA1MO5z5a4uYvJuwg CAZc+uKlTx5rstKomj2er6X2wTCUxVRDL/5gRJFKaGFudGBzuqpfLR2VKA1tUGCmBM46JJCzi +bfnK3B3sxl2WT1o+BiXq18A5xeO+y/zwguXnUnbw2cfMYdmxf5d4grUNvZaOEJaU9vf6AlF4 P0hMUc+vuCO0uyAgIGLytQmHSzBGjfSSaSKEyK9wudXE3FTyjGIwa4zWkSNt/s5+IsRWExLk3 mLnk4ElDTETdFpw0DTUr2QOYl06dg4CXxEQF3LjCpQ== Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit A couple comments below, thanks! On 3/24/2023 3:30 PM, Michael Kubacki wrote: > From: Michael Kubacki > > Fixes CodeQL alerts for CWE-457: > https://cwe.mitre.org/data/definitions/457.html > > Cc: Erich McMillan > Cc: Michael D Kinney > Cc: Michael Kubacki > Cc: Ray Ni > Cc: Zhichao Gao > Co-authored-by: Erich McMillan > Signed-off-by: Michael Kubacki > Reviewed-by: Zhichao Gao > --- > 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.c > index 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.c > index 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.c > index 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. > - 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.c > index 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.c > index 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. > + 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.c > index 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.c > index 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.c > index 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 (