ShellPkg: More CodeQL fixes (#321)

## Description Various fixes - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Build and boot changes on QemuQ35Pkg to EFI shell. ## Integration Instructions N/A
microsoft · May 9, 2023 · a05d819 · a05d819
1 parent 5b2d935
commit a05d819
Show file tree

Hide file tree

Showing 27 changed files with 384 additions and 59 deletions.
diff --git a/ShellPkg/Application/Shell/FileHandleWrappers.c b/ShellPkg/Application/Shell/FileHandleWrappers.c
@@ -2144,8 +2144,14 @@ FileInterfaceFileWrite (
     // Ascii
     //
     AsciiBuffer = AllocateZeroPool (*BufferSize);
-    Size        = AsciiSPrint (AsciiBuffer, *BufferSize, "%S", Buffer); // MU_CHANGE BZ3232
-    Status      = (((EFI_FILE_PROTOCOL_FILE *)This)->Orig->Write (((EFI_FILE_PROTOCOL_FILE *)This)->Orig, &Size, AsciiBuffer));
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (AsciiBuffer == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
+    Size   = AsciiSPrint (AsciiBuffer, *BufferSize, "%S", Buffer);      // MU_CHANGE BZ3232
+    Status = (((EFI_FILE_PROTOCOL_FILE *)This)->Orig->Write (((EFI_FILE_PROTOCOL_FILE *)This)->Orig, &Size, AsciiBuffer));
     FreePool (AsciiBuffer);
     return (Status);
   }

diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
@@ -575,6 +575,13 @@ UefiMain (
 
     Size       = 100;
     TempString = AllocateZeroPool (Size);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (TempString == NULL) {
+      ASSERT (TempString != NULL);
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
 
     UnicodeSPrint (TempString, Size, L"%d", PcdGet8 (PcdShellSupportLevel));
     Status = InternalEfiShellSetEnv (L"uefishellsupport", TempString, TRUE);

diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c b/ShellPkg/Application/Shell/ShellParametersProtocol.c
@@ -354,7 +354,13 @@ CreatePopulateInstallShellParametersProtocol (
   Status = SHELL_GET_ENVIRONMENT_VARIABLE (L"ShellOpt", &Size, FullCommandLine);
   if (Status == EFI_BUFFER_TOO_SMALL) {
     FullCommandLine = AllocateZeroPool (Size + LoadedImage->LoadOptionsSize);
-    Status          = SHELL_GET_ENVIRONMENT_VARIABLE (L"ShellOpt", &Size, FullCommandLine);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (FullCommandLine == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
+    Status = SHELL_GET_ENVIRONMENT_VARIABLE (L"ShellOpt", &Size, FullCommandLine);
   }
 
   if (Status == EFI_NOT_FOUND) {

diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -2912,6 +2912,12 @@ EfiShellGetEnvEx (
         // Allocate the space and recall the get function
         //
         Buffer = AllocateZeroPool (Size);
+        // MU_CHANGE [BEGIN] - CodeQL change
+        if (Buffer == NULL) {
+          return NULL;
+        }
+
+        // MU_CHANGE [END] - CodeQL change
         Status = SHELL_GET_ENVIRONMENT_VARIABLE_AND_ATTRIBUTES (Name, Attributes, &Size, Buffer);
       }
 
@@ -3562,6 +3568,13 @@ EfiShellGetAlias (
     Status  = gRT->GetVariable (AliasLower, &gShellAliasGuid, &Attribs, &RetSize, RetVal);
     if (Status == EFI_BUFFER_TOO_SMALL) {
       RetVal = AllocateZeroPool (RetSize);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (RetVal == NULL) {
+        FreePool (AliasLower);
+        return NULL;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       Status = gRT->GetVariable (AliasLower, &gShellAliasGuid, &Attribs, &RetSize, RetVal);
     }
 

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -315,25 +315,28 @@ LoadedImageProtocolDumpInformation (
   PdbFileName = PeCoffLoaderGetPdbPointer (LoadedImage->ImageBase);
   DataType    = ConvertMemoryType (LoadedImage->ImageDataType);
   CodeType    = ConvertMemoryType (LoadedImage->ImageCodeType);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if ((PdbFileName != NULL) && (DataType != NULL) && (CodeType != NULL) && (FilePath != NULL)) {
+    RetVal = CatSPrint (
+               RetVal,
+               Temp,
+               LoadedImage->Revision,
+               LoadedImage->ParentHandle,
+               LoadedImage->SystemTable,
+               LoadedImage->DeviceHandle,
+               FilePath,
+               PdbFileName,
+               LoadedImage->LoadOptionsSize,
+               LoadedImage->LoadOptions,
+               LoadedImage->ImageBase,
+               LoadedImage->ImageSize,
+               CodeType,
+               DataType,
+               LoadedImage->Unload
+               );
+  }
 
-  RetVal = CatSPrint (
-             RetVal,
-             Temp,
-             LoadedImage->Revision,
-             LoadedImage->ParentHandle,
-             LoadedImage->SystemTable,
-             LoadedImage->DeviceHandle,
-             FilePath,
-             PdbFileName,
-             LoadedImage->LoadOptionsSize,
-             LoadedImage->LoadOptions,
-             LoadedImage->ImageBase,
-             LoadedImage->ImageSize,
-             CodeType,
-             DataType,
-             LoadedImage->Unload
-             );
-
+  // MU_CHANGE [END] - CodeQL change
   SHELL_FREE_NON_NULL (Temp);
   SHELL_FREE_NON_NULL (FilePath);
   SHELL_FREE_NON_NULL (CodeType);
@@ -395,7 +398,13 @@ GraphicsOutputProtocolDumpInformation (
   }
 
   Fmt = ConvertPixelFormat (GraphicsOutput->Mode->Info->PixelFormat);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Fmt == NULL) {
+    SHELL_FREE_NON_NULL (Temp);
+    return NULL;
+  }
 
+  // MU_CHANGE [END] - CodeQL change
   RetVal = CatSPrint (
              NULL,
              Temp,
@@ -813,7 +822,12 @@ TxtOutProtocolDumpInformation (
 
   Size   = (Dev->Mode->MaxMode + 1) * 80;
   RetVal = AllocateZeroPool (Size);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (RetVal == NULL) {
+    return NULL;
+  }
 
+  // MU_CHANGE END] - CodeQL change
   Temp = HiiGetString (mHandleParsingHiiHandle, STRING_TOKEN (STR_TXT_OUT_DUMP_HEADER), NULL);
   if (Temp != NULL) {
     UnicodeSPrint (RetVal, Size, Temp, Dev, Dev->Mode->Attribute);
@@ -824,6 +838,13 @@ TxtOutProtocolDumpInformation (
   // Dump TextOut Info
   //
   Temp = HiiGetString (mHandleParsingHiiHandle, STRING_TOKEN (STR_TXT_OUT_DUMP_LINE), NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Temp == NULL) {
+    FreePool (RetVal);
+    return NULL;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   for (Index = 0; Index < Dev->Mode->MaxMode; Index++) {
     Status  = Dev->QueryMode (Dev, Index, &Col, &Row);
     NewSize = Size - StrSize (RetVal);

diff --git a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
@@ -202,6 +202,12 @@ GetBootOptionCrc (
                   );
   if (Status == EFI_BUFFER_TOO_SMALL) {
     Buffer = AllocateZeroPool (BufferSize);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (Buffer == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     Status = gRT->GetVariable (
                     VariableName,
                     (EFI_GUID *)&gEfiGlobalVariableGuid,
@@ -1388,6 +1394,14 @@ BcfgDisplayDump (
                     );
     if (Status == EFI_BUFFER_TOO_SMALL) {
       Buffer = AllocateZeroPool (BufferSize);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (Buffer == NULL) {
+        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM), gShellBcfgHiiHandle, L"bcfg");
+        ++Errors;
+        goto Cleanup;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       Status = gRT->GetVariable (
                       VariableName,
                       (EFI_GUID *)&gEfiGlobalVariableGuid,

diff --git a/ShellPkg/Library/UefiShellCommandLib/ConsistMapping.c b/ShellPkg/Library/UefiShellCommandLib/ConsistMapping.c
@@ -1404,6 +1404,12 @@ GetHIDevicePath (
   NonHIDevicePathNodeCount = 0;
 
   HIDevicePath = AllocateZeroPool (sizeof (EFI_DEVICE_PATH_PROTOCOL));
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (HIDevicePath == NULL) {
+    return NULL;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   SetDevicePathEndNode (HIDevicePath);
 
   Node.DevPath.Type      = END_DEVICE_PATH_TYPE;

diff --git a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
@@ -1263,6 +1263,12 @@ ShellCommandCreateNewMappingName (
   String = NULL;
 
   String = AllocateZeroPool (PcdGet8 (PcdShellMapNameLength) * sizeof (String[0]));
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (String == NULL) {
+    return (NULL);
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   UnicodeSPrint (
     String,
     PcdGet8 (PcdShellMapNameLength) * sizeof (String[0]),
@@ -1475,7 +1481,14 @@ ShellCommandCreateInitialMappingsAndPaths (
       // Get default name first
       //
       NewDefaultName = ShellCommandCreateNewMappingName (MappingTypeFileSystem);
-      ASSERT (NewDefaultName != NULL);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (NewDefaultName == NULL) {
+        ASSERT (NewDefaultName != NULL);
+        Status = EFI_OUT_OF_RESOURCES;
+        break;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName, DevicePathList[Count], 0, TRUE);
       ASSERT_EFI_ERROR (Status);
       FreePool (NewDefaultName);
@@ -1565,7 +1578,15 @@ ShellCommandCreateInitialMappingsAndPaths (
       // Get default name first
       //
       NewDefaultName = ShellCommandCreateNewMappingName (MappingTypeBlockIo);
-      ASSERT (NewDefaultName != NULL);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (NewDefaultName == NULL) {
+        ASSERT (NewDefaultName != NULL);
+        SHELL_FREE_NON_NULL (HandleList);
+        SHELL_FREE_NON_NULL (DevicePathList);
+        return EFI_OUT_OF_RESOURCES;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName, DevicePathList[Count], 0, FALSE);
       ASSERT_EFI_ERROR (Status);
       FreePool (NewDefaultName);

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
@@ -448,7 +448,13 @@ CascadeProcessVariables (
     StrnCatGrow (&FoundVarName, &NameSize, PrevName, 0);
   } else {
     FoundVarName = AllocateZeroPool (sizeof (CHAR16));
-    NameSize     = sizeof (CHAR16);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (FoundVarName == NULL) {
+      return (SHELL_OUT_OF_RESOURCES);
+    }
+
+    // MU_CHANGE [END] - CodeQL change
+    NameSize = sizeof (CHAR16);
   }
 
   Status = gRT->GetNextVariableName (&NameSize, FoundVarName, &FoundVarGuid);
@@ -817,6 +823,12 @@ ShellCommandRunDmpStore (
       // Get the Name of the variable to find
       //
       Name = ShellCommandLineGetRawValue (Package, 1);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (Name == NULL) {
+        return (SHELL_INVALID_PARAMETER);
+      }
+
+      // MU_CHANGE [END] - CodeQL change
 
       if (ShellStatus == SHELL_SUCCESS) {
         if (ShellCommandLineGetFlag (Package, L"-s")) {

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
@@ -107,6 +107,12 @@ MenuBarRefresh (
   //
   for (Item = MenuItems; Item != NULL && Item->Function != NULL; Item++) {
     NameString = HiiGetString (gShellDebug1HiiHandle, Item->NameToken, NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (NameString == NULL) {
+      return EFI_INVALID_PARAMETER;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
 
     Width = MAX ((StrLen (NameString) + 6), 20);
     if (((Col + Width) > LastCol)) {
@@ -115,7 +121,12 @@ MenuBarRefresh (
     }
 
     FunctionKeyString = HiiGetString (gShellDebug1HiiHandle, Item->FunctionKeyToken, NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (FunctionKeyString == NULL) {
+      return EFI_INVALID_PARAMETER;
+    }
 
+    // MU_CHANGE [END] - CodeQL change
     ShellPrintEx ((INT32)(Col) - 1, (INT32)(Row) - 1, L"%E%s%N  %H%s%N  ", FunctionKeyString, NameString);
 
     FreePool (NameString);

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
@@ -302,6 +302,13 @@ LoadEfiDriversFromRomImage (
                                       );
             if (!EFI_ERROR (Status)) {
               DecompressedImageBuffer = AllocateZeroPool (DestinationSize);
+              // MU_CHANGE [BEGIN] - CodeQL change
+              if (DecompressedImageBuffer == NULL) {
+                ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_OUT_MEM), gShellDebug1HiiHandle, L"loadpcirom");
+                return EFI_OUT_OF_RESOURCES;
+              }
+
+              // MU_CHANGE [END] - CodeQL change
               if (ImageBuffer != NULL) {
                 Scratch = AllocateZeroPool (ScratchSize);
                 if (Scratch != NULL) {

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/MemMap.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/MemMap.c
@@ -214,7 +214,14 @@ ShellCommandRunMemMap (
       if (Status == EFI_BUFFER_TOO_SMALL) {
         Size       += SIZE_1KB;
         Descriptors = AllocateZeroPool (Size);
-        Status      = gBS->GetMemoryMap (&Size, Descriptors, &MapKey, &ItemSize, &Version);
+        // MU_CHANGE [BEGIN] - CodeQL change
+        if (Descriptors == NULL) {
+          ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_OUT_MEM), gShellDebug1HiiHandle, L"memmap");
+          ShellStatus = SHELL_OUT_OF_RESOURCES;
+        }
+
+        // MU_CHANGE [END] - CodeQL change
+        Status = gBS->GetMemoryMap (&Size, Descriptors, &MapKey, &ItemSize, &Version);
       }
 
       if (EFI_ERROR (Status)) {

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SetVar.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/SetVar.c
@@ -421,6 +421,7 @@ ShellCommandRunSetVar (
         if (StringGuid != NULL) {
           RStatus = StrToGuid (StringGuid, &Guid);
         } else {
+          ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), gShellDebug1HiiHandle, L"setvar", StringGuid);
           return SHELL_INVALID_PARAMETER;
         }
 
@@ -438,6 +439,13 @@ ShellCommandRunSetVar (
         Status = gRT->GetVariable ((CHAR16 *)VariableName, &Guid, &Attributes, &Size, Buffer);
         if (Status == EFI_BUFFER_TOO_SMALL) {
           Buffer = AllocateZeroPool (Size);
+          // MU_CHANGE [START] - CodeQL change
+          if (Buffer == NULL) {
+            ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_OUT_MEM), gShellDebug1HiiHandle, L"setvar");
+            return SHELL_OUT_OF_RESOURCES;
+          }
+
+          // MU_CHANGE [END] - CodeQL change
           Status = gRT->GetVariable ((CHAR16 *)VariableName, &Guid, &Attributes, &Size, Buffer);
         }
 
@@ -459,6 +467,13 @@ ShellCommandRunSetVar (
         Status = gRT->GetVariable ((CHAR16 *)VariableName, &Guid, &Attributes, &Size, Buffer);
         if (Status == EFI_BUFFER_TOO_SMALL) {
           Buffer = AllocateZeroPool (Size);
+          // MU_CHANGE [START] - CodeQL change
+          if (Buffer == NULL) {
+            ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_OUT_MEM), gShellDebug1HiiHandle, L"setvar");
+            return SHELL_OUT_OF_RESOURCES;
+          }
+
+          // MU_CHANGE [END] - CodeQL change
           Status = gRT->GetVariable ((CHAR16 *)VariableName, &Guid, &Attributes, &Size, Buffer);
         }
 

diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/DevTree.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/DevTree.c
@@ -195,6 +195,14 @@ ShellCommandRunDevTree (
     Lang = ShellCommandLineGetValue (Package, L"-l");
     if (Lang != NULL) {
       Language = AllocateZeroPool (StrSize (Lang));
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (Language == NULL) {
+        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_OUT_MEM), gShellDriver1HiiHandle, L"devtree");
+        ShellCommandLineFreeVarList (Package);
+        return (SHELL_OUT_OF_RESOURCES);
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       AsciiSPrint (Language, StrSize (Lang), "%S", Lang);
     } else if (!ShellCommandLineGetFlag (Package, L"-l")) {
       ASSERT (Language == NULL);
@@ -212,6 +220,16 @@ ShellCommandRunDevTree (
     Lang      = ShellCommandLineGetRawValue (Package, 1);
     HiiString = HiiGetString (gShellDriver1HiiHandle, STRING_TOKEN (STR_DEV_TREE_OUTPUT), Language);
 
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (HiiString == NULL) {
+      ASSERT (HiiString != NULL);
+      SHELL_FREE_NON_NULL (Language);
+      ShellCommandLineFreeVarList (Package);
+      return (SHELL_INVALID_PARAMETER);
+    }
+
+    // MU_CHANGE [END] - CodeQL change
+
     if (Lang == NULL) {
       for (LoopVar = 1; ; LoopVar++) {
         TheHandle = ConvertHandleIndexToHandle (LoopVar);