ShellPkg: Additional CodeQL fixes (#268)

## 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 ShellPkg and boot changes on QemuQ35Pkg to EFI shell. ## Integration Instructions N/A
microsoft · May 3, 2023 · 1cd65ab · 1cd65ab
1 parent d6d3c39
commit 1cd65ab
Show file tree

Hide file tree

Showing 41 changed files with 661 additions and 120 deletions.
diff --git a/CodeQlFilters.yml b/CodeQlFilters.yml
@@ -15,6 +15,9 @@
     "-MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c:SM02311",
     "-MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c:SM02311",
     "-MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c:SM02311",
+    "-ShellPkg/Library/UefiShellLevel3CommandsLib/Alias.c:SM02311",
+    "-ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c:SM02311",
+    "-ShellPkg/Library/UefiShellLevel2CommandsLib/Map.c:SM02311",
     "-MdeModulePkg/Universal/Disk/UdfDxe/FileName.c:cpp/uselesstest",
     "-MdeModulePkg/Universal/EbcDxe/EbcDebugger/EdbSupportString.c:cpp/uselesstest",
     "-MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c:cpp/uselesstest",

diff --git a/MdePkg/Include/IndustryStandard/PciExpress21.h b/MdePkg/Include/IndustryStandard/PciExpress21.h
@@ -684,9 +684,9 @@ typedef struct {
   UINT16                                      DpaControl;
   UINT8                                       DpaPowerAllocationArray[1];
 } PCI_EXPRESS_EXTENDED_CAPABILITIES_DYNAMIC_POWER_ALLOCATION;
-
-#define PCI_EXPRESS_EXTENDED_CAPABILITY_DYNAMIC_POWER_ALLOCATION_GET_SUBSTATE_MAX(POWER)  (UINT16)(((POWER->DpaCapability)&0x0000000F))
-
+// MU_CHANGE [START] - CodeQL change
+#define PCI_EXPRESS_EXTENDED_CAPABILITY_DYNAMIC_POWER_ALLOCATION_GET_SUBSTATE_MAX(POWER)  (UINT32)(((POWER->DpaCapability)&0x0000000F))
+// MU_CHANGE [END] - CodeQL change
 #define PCI_EXPRESS_EXTENDED_CAPABILITY_LATENCE_TOLERANCE_REPORTING_ID    0x0018
 #define PCI_EXPRESS_EXTENDED_CAPABILITY_LATENCE_TOLERANCE_REPORTING_VER1  0x1
 

diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
@@ -1324,7 +1324,9 @@ DoStartupScript (
     }
 
     Status = RunShellCommand (FileStringPath, &CalleeStatus);
-    if (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE) {
+    // MU_CHANGE [START] - CodeQL change
+    if ((!EFI_ERROR (Status)) && (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE)) {
+      // MU_CHANGE [END] - CodeQL change
       ShellCommandRegisterExit (gEfiShellProtocol->BatchIsActive (), (UINT64)CalleeStatus);
     }
 
@@ -2607,11 +2609,19 @@ RunCommandOrFile (
         CommandWithPath = ShellFindFilePathEx (FirstParameter, mExecutableExtensions);
       }
 
-      //
-      // This should be impossible now.
-      //
-      ASSERT (CommandWithPath != NULL);
+      // MU_CHANGE [START] - CodeQL change
+      if (CommandWithPath == NULL) {
+        //
+        // This should be impossible now.
+        //
+        ASSERT (CommandWithPath != NULL);
+        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_SHELL_NOT_FOUND), ShellInfoObject.HiiHandle, FirstParameter);
+        SetLastError (SHELL_NOT_FOUND);
+        SHELL_FREE_NON_NULL (CommandWithPath);
+        return EFI_NOT_FOUND;
+      }
 
+      // MU_CHANGE [END] - CodeQL change
       //
       // Make sure that path is not just a directory (or not found)
       //
@@ -3328,8 +3338,11 @@ FindFirstCharacter (
   IN CONST CHAR16  EscapeCharacter
   )
 {
-  UINT32  WalkChar;
-  UINT32  WalkStr;
+  // MU_CHANGE [START] - CodeQL change
+  UINTN  WalkChar;
+  UINTN  WalkStr;
+
+  // MU_CHANGE [END] - CodeQL change
 
   for (WalkStr = 0; WalkStr < StrLen (String); WalkStr++) {
     if (String[WalkStr] == EscapeCharacter) {

diff --git a/ShellPkg/Application/Shell/ShellManParser.c b/ShellPkg/Application/Shell/ShellManParser.c
@@ -600,7 +600,14 @@ ProcessManFile (
   TempString = ShellCommandGetCommandHelp (Command);
   if (TempString != NULL) {
     FileHandle = ConvertEfiFileProtocolToShellHandle (CreateFileInterfaceMem (TRUE), NULL);
-    HelpSize   = StrLen (TempString) * sizeof (CHAR16);
+    // MU_CHANGE [START] - CodeQL change
+    if (FileHandle == NULL) {
+      Status = EFI_OUT_OF_RESOURCES;
+      goto Done;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
+    HelpSize = StrLen (TempString) * sizeof (CHAR16);
     ShellWriteFile (FileHandle, &HelpSize, TempString);
     ShellSetFilePosition (FileHandle, 0);
     HelpSize  = 0;
@@ -624,8 +631,15 @@ ProcessManFile (
     Status = SearchPathForFile (TempString, &FileHandle);
     if (EFI_ERROR (Status)) {
       FileDevPath = FileDevicePath (NULL, TempString);
-      DevPath     = AppendDevicePath (ShellInfoObject.ImageDevPath, FileDevPath);
-      Status      = InternalOpenFileDevicePath (DevPath, &FileHandle, EFI_FILE_MODE_READ, 0);
+      // MU_CHANGE [START] - CodeQL change
+      if (FileDevPath == NULL) {
+        Status = EFI_INVALID_PARAMETER;
+        goto Done;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
+      DevPath = AppendDevicePath (ShellInfoObject.ImageDevPath, FileDevPath);
+      Status  = InternalOpenFileDevicePath (DevPath, &FileHandle, EFI_FILE_MODE_READ, 0);
       SHELL_FREE_NON_NULL (FileDevPath);
       SHELL_FREE_NON_NULL (DevPath);
     }
@@ -733,7 +747,14 @@ ProcessManFile (
       }
 
       FileHandle = ConvertEfiFileProtocolToShellHandle (CreateFileInterfaceMem (TRUE), NULL);
-      HelpSize   = StrLen (TempString) * sizeof (CHAR16);
+      // MU_CHANGE [START] - CodeQL change
+      if (FileHandle == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Done;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
+      HelpSize = StrLen (TempString) * sizeof (CHAR16);
       ShellWriteFile (FileHandle, &HelpSize, TempString);
       ShellSetFilePosition (FileHandle, 0);
       HelpSize  = 0;

diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c b/ShellPkg/Application/Shell/ShellParametersProtocol.c
@@ -1307,6 +1307,12 @@ UpdateStdInStdOutStdErr (
             TempHandle = CreateFileInterfaceFile (TempHandle, FALSE);
           }
 
+          // MU_CHANGE [START] - CodeQL change
+          if (TempHandle == NULL) {
+            return EFI_OUT_OF_RESOURCES;
+          }
+
+          // MU_CHANGE [END] - CodeQL change
           ShellParameters->StdIn = TempHandle;
           gST->ConIn             = CreateSimpleTextInOnFile (TempHandle, &gST->ConsoleInHandle);
         }

diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -436,7 +436,6 @@ EfiShellGetFilePathFromDevicePath (
         if ((DevicePathType (&FilePath->Header) != MEDIA_DEVICE_PATH) ||
             (DevicePathSubType (&FilePath->Header) != MEDIA_FILEPATH_DP))
         {
-          FreePool (PathForReturn);
           return NULL;
         }
 
@@ -447,7 +446,12 @@ EfiShellGetFilePathFromDevicePath (
 
         AlignedNode = AllocateCopyPool (DevicePathNodeLength (FilePath), FilePath);
         if (AlignedNode == NULL) {
-          FreePool (PathForReturn);
+          // MU_CHANGE [START] - CodeQL change
+          if (PathForReturn != NULL) {
+            FreePool (PathForReturn);
+          }
+
+          // MU_CHANGE [END] - CodeQL change
           return NULL;
         }
 
@@ -719,7 +723,13 @@ EfiShellGetDeviceName (
         continue;
       }
 
-      Lang   = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE);
+      Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE);
+      // MU_CHANGE [START] - CodeQL change
+      if (Lang == NULL) {
+        continue;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       Status = CompName2->GetControllerName (CompName2, DeviceHandle, NULL, Lang, &DeviceNameToReturn);
       FreePool (Lang);
       Lang = NULL;
@@ -736,7 +746,13 @@ EfiShellGetDeviceName (
     // Now check the parent controller using this as the child.
     //
     if (DeviceNameToReturn == NULL) {
-      PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, &ParentControllerCount, &ParentControllerBuffer);
+      // MU_CHANGE [START] - CodeQL change
+      Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, &ParentControllerCount, &ParentControllerBuffer);
+      if (EFI_ERROR (Status)) {
+        ParentControllerCount = 0;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {
         PARSE_HANDLE_DATABASE_UEFI_DRIVERS (ParentControllerBuffer[LoopVar], &ParentDriverCount, &ParentDriverBuffer);
         for (HandleCount = 0; HandleCount < ParentDriverCount; HandleCount++) {
@@ -766,7 +782,13 @@ EfiShellGetDeviceName (
             continue;
           }
 
-          Lang   = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE);
+          Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE);
+          // MU_CHANGE [START] - CodeQL change
+          if (Lang == NULL) {
+            continue;
+          }
+
+          // MU_CHANGE [END] - CodeQL change
           Status = CompName2->GetControllerName (CompName2, ParentControllerBuffer[LoopVar], DeviceHandle, Lang, &DeviceNameToReturn);
           FreePool (Lang);
           Lang = NULL;
@@ -1806,8 +1828,10 @@ EfiShellExecute (
   OUT EFI_STATUS  *StatusCode OPTIONAL
   )
 {
-  EFI_STATUS                Status;
-  CHAR16                    *Temp;
+  EFI_STATUS  Status;
+  // MU_CHANGE [START] - CodeQL change
+  CHAR16  *Temp = NULL;
+  // MU_CHANGE [END] - CodeQL change
   EFI_DEVICE_PATH_PROTOCOL  *DevPath;
   UINTN                     Size;
 
@@ -1817,14 +1841,31 @@ EfiShellExecute (
 
   if (NestingEnabled ()) {
     DevPath = AppendDevicePath (ShellInfoObject.ImageDevPath, ShellInfoObject.FileDevPath);
+    // MU_CHANGE [START] - CodeQL change
+    if (DevPath == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
 
     DEBUG_CODE_BEGIN ();
     Temp = ConvertDevicePathToText (ShellInfoObject.FileDevPath, TRUE, TRUE);
-    FreePool (Temp);
+    if (Temp != NULL) {
+      FreePool (Temp);
+    }
+
     Temp = ConvertDevicePathToText (ShellInfoObject.ImageDevPath, TRUE, TRUE);
-    FreePool (Temp);
-    Temp = ConvertDevicePathToText (DevPath, TRUE, TRUE);
-    FreePool (Temp);
+    if (Temp != NULL) {
+      FreePool (Temp);
+    }
+
+    if (DevPath != NULL) {
+      Temp = ConvertDevicePathToText (DevPath, TRUE, TRUE);
+    }
+
+    if (Temp != NULL) {
+      FreePool (Temp);
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     DEBUG_CODE_END ();
 
     Temp = NULL;
@@ -2387,11 +2428,13 @@ ShellSearchHandle (
   CHAR16               *CurrentFilePattern;
   EFI_SHELL_FILE_INFO  *ShellInfo;
   EFI_SHELL_FILE_INFO  *ShellInfoNode;
-  EFI_SHELL_FILE_INFO  *NewShellNode;
-  EFI_FILE_INFO        *FileInfo;
-  BOOLEAN              Directory;
-  CHAR16               *NewFullName;
-  UINTN                Size;
+  // MU_CHANGE [START] - CodeQL change
+  EFI_SHELL_FILE_INFO  *NewShellNode = NULL;
+  EFI_FILE_INFO        *FileInfo     = NULL;
+  // MU_CHANGE [END] - CodeQL change
+  BOOLEAN  Directory;
+  CHAR16   *NewFullName;
+  UINTN    Size;
 
   if (  (FilePattern      == NULL)
      || (UnicodeCollation == NULL)
@@ -2432,14 +2475,19 @@ ShellSearchHandle (
       //
       // We want the root node.  create the node.
       //
-      FileInfo     = FileHandleGetInfo (FileHandle);
-      NewShellNode = CreateAndPopulateShellFileInfo (
-                       MapName,
-                       EFI_SUCCESS,
-                       L"\\",
-                       FileHandle,
-                       FileInfo
-                       );
+      FileInfo = FileHandleGetInfo (FileHandle);
+      // MU_CHANGE [START] - CodeQL change
+      if (FileInfo != NULL) {
+        NewShellNode = CreateAndPopulateShellFileInfo (
+                         MapName,
+                         EFI_SUCCESS,
+                         L"\\",
+                         FileHandle,
+                         FileInfo
+                         );
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       SHELL_FREE_NON_NULL (FileInfo);
     } else {
       //
@@ -2629,7 +2677,12 @@ EfiShellFindFiles (
   }
 
   PatternCopy = PathCleanUpDirectories (PatternCopy);
+  // MU_CHANGE [START] - CodeQL change
+  if (PatternCopy == NULL) {
+    return (EFI_OUT_OF_RESOURCES);
+  }
 
+  // MU_CHANGE [END] - CodeQL change
   Count = StrStr (PatternCopy, L":") - PatternCopy + 1;
   ASSERT (Count <= StrLen (PatternCopy));
 
@@ -2713,6 +2766,12 @@ EfiShellOpenFileList (
   //
   if (StrStr (Path, L":") == NULL) {
     CurDir = EfiShellGetCurDir (NULL);
+    // MU_CHANGE [START] - CodeQL change
+    if (CurDir == NULL) {
+      return EFI_NOT_FOUND;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     ASSERT ((Path2 == NULL && Path2Size == 0) || (Path2 != NULL));
     StrnCatGrow (&Path2, &Path2Size, CurDir, 0);
     StrnCatGrow (&Path2, &Path2Size, L"\\", 0);
@@ -3120,8 +3179,13 @@ EfiShellSetCurDir (
   }
 
   DirectoryName = StrnCatGrow (&DirectoryName, NULL, Dir, 0);
-  ASSERT (DirectoryName != NULL);
+  // MU_CHANGE [START] - CodeQL change
+  if (DirectoryName == NULL) {
+    ASSERT (DirectoryName != NULL);
+    return (EFI_OUT_OF_RESOURCES);
+  }
 
+  // MU_CHANGE [END] - CodeQL change
   PathCleanUpDirectories (DirectoryName);
 
   if (FileSystem == NULL) {

diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
@@ -1815,6 +1815,13 @@ DownloadFile (
                     Context->Uri,
                     StrLen (Context->Uri)
                     );
+    // MU_CHANGE [START] - CodeQL change
+    if (DownloadUrl == NULL) {
+      Status = EFI_OUT_OF_RESOURCES;
+      goto ON_EXIT;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
 
     PRINT_HII (STRING_TOKEN (STR_HTTP_DOWNLOADING), DownloadUrl);
 

diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
@@ -395,7 +395,13 @@ RunTftp (
   }
 
   RemoteFilePath = ShellCommandLineGetRawValue (CheckPackage, 2);
-  ASSERT (RemoteFilePath != NULL);
+  // MU_CHANGE [START] - CodeQL change
+  if (RemoteFilePath == NULL) {
+    ASSERT (RemoteFilePath != NULL);
+    goto Error;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   FilePathSize        = StrLen (RemoteFilePath) + 1;
   AsciiRemoteFilePath = AllocatePool (FilePathSize);
   if (AsciiRemoteFilePath == NULL) {