Additional CodeQL Fixes (microsoft#358)

## 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
TaylorBeebe · Apr 27, 2023 · 4b0f44f · 4b0f44f
1 parent 9118b65
commit 4b0f44f
Show file tree

Hide file tree

Showing 12 changed files with 126 additions and 39 deletions.
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -418,6 +418,12 @@ CoreAllocatePoolI (
     NoPages  = EFI_SIZE_TO_PAGES (Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
     Head     = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity, NeedGuard);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (Head == NULL) {
+      return NULL;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     if (NeedGuard) {
       Head = AdjustPoolHeadA ((EFI_PHYSICAL_ADDRESS)(UINTN)Head, NoPages, Size);
     }

diff --git a/NetworkPkg/Ip6Dxe/Ip6Input.c b/NetworkPkg/Ip6Dxe/Ip6Input.c
@@ -1314,24 +1314,25 @@ Ip6InstanceFrameAcceptable (
   // Check whether the protocol is acceptable.
   //
   ExtHdrs = NetbufGetByte (Packet, 0, NULL);
-
-  if (!Ip6IsExtsValid (
-         IpInstance->Service,
-         Packet,
-         &Head->NextHeader,
-         ExtHdrs,
-         (UINT32)Head->PayloadLength,
-         TRUE,
-         NULL,
-         &Proto,
-         NULL,
-         NULL,
-         NULL
-         ))
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if ((ExtHdrs == NULL) || !Ip6IsExtsValid (
+                              IpInstance->Service,
+                              Packet,
+                              &Head->NextHeader,
+                              ExtHdrs,
+                              (UINT32)Head->PayloadLength,
+                              TRUE,
+                              NULL,
+                              &Proto,
+                              NULL,
+                              NULL,
+                              NULL
+                              ))
   {
     return FALSE;
   }
 
+  // MU_CHANGE [END] - CodeQL change
   //
   // The upper layer driver may want to receive the ICMPv6 error packet
   // invoked by its packet, like UDP.
@@ -1349,23 +1350,25 @@ Ip6InstanceFrameAcceptable (
       //
       ErrMsgPayloadLen = NTOHS (Icmp.IpHead.PayloadLength);
       ErrMsgPayload    = NetbufGetByte (Packet, sizeof (Icmp), NULL);
-
-      if (!Ip6IsExtsValid (
-             NULL,
-             NULL,
-             &Icmp.IpHead.NextHeader,
-             ErrMsgPayload,
-             ErrMsgPayloadLen,
-             TRUE,
-             NULL,
-             &Proto,
-             NULL,
-             NULL,
-             NULL
-             ))
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if ((ErrMsgPayload == NULL) || !Ip6IsExtsValid (
+                                        NULL,
+                                        NULL,
+                                        &Icmp.IpHead.NextHeader,
+                                        ErrMsgPayload,
+                                        ErrMsgPayloadLen,
+                                        TRUE,
+                                        NULL,
+                                        &Proto,
+                                        NULL,
+                                        NULL,
+                                        NULL
+                                        ))
       {
         return FALSE;
       }
+
+      // MU_CHANGE [END] - CodeQL change
     }
   }
 

diff --git a/NetworkPkg/Ip6Dxe/Ip6Mld.c b/NetworkPkg/Ip6Dxe/Ip6Mld.c
@@ -181,7 +181,14 @@ Ip6SendMldReport (
   // Fill a IPv6 Router Alert option in a Hop-by-Hop Options Header
   //
   Options = NetbufAllocSpace (Packet, (UINT32)OptionLen, FALSE);
-  ASSERT (Options != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Options == NULL) {
+    ASSERT (Options != NULL);
+    NetbufFree (Packet);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   Status = Ip6FillHopByHop (Options, &OptionLen, IP6_ICMP);
   if (EFI_ERROR (Status)) {
     NetbufFree (Packet);

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -1554,7 +1554,13 @@ Ip6ProcessNeighborSolicit (
     OptionLen = (UINT16)(Head->PayloadLength - IP6_ND_LENGTH);
     if (OptionLen != 0) {
       Option = NetbufGetByte (Packet, IP6_ND_LENGTH, NULL);
-      ASSERT (Option != NULL);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (Option == NULL) {
+        ASSERT (Option != NULL);
+        goto Exit;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
 
       //
       // All included options should have a length that is greater than zero.
@@ -2043,8 +2049,13 @@ Ip6ProcessRouterAdvertise (
   OptionLen = (UINT16)(Head->PayloadLength - IP6_RA_LENGTH);
   if (OptionLen != 0) {
     Option = NetbufGetByte (Packet, IP6_RA_LENGTH, NULL);
-    ASSERT (Option != NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (Option == NULL) {
+      ASSERT (Option != NULL);
+      goto Exit;
+    }
 
+    // MU_CHANGE [END] - CodeQL change
     if (!Ip6IsNDOptionValid (Option, OptionLen)) {
       goto Exit;
     }

diff --git a/ShellPkg/Application/Shell/ShellManParser.c b/ShellPkg/Application/Shell/ShellManParser.c
@@ -549,6 +549,7 @@ ManFileFindTitleSection (
                                 returned help text.
   @retval EFI_INVALID_PARAMETER HelpText is NULL.
   @retval EFI_INVALID_PARAMETER ManFileName is invalid.
+  @retval EFI_INVALID_PARAMETER Command is invalid. // MU_CHANGE: CodeQL change
   @retval EFI_NOT_FOUND         There is no help text available for Command.
 **/
 EFI_STATUS
@@ -633,13 +634,19 @@ ProcessManFile (
       FileDevPath = FileDevicePath (NULL, TempString);
       // MU_CHANGE [START] - CodeQL change
       if (FileDevPath == NULL) {
-        Status = EFI_INVALID_PARAMETER;
+        Status = EFI_OUT_OF_RESOURCES;
         goto Done;
       }
 
-      // MU_CHANGE [END] - CodeQL change
       DevPath = AppendDevicePath (ShellInfoObject.ImageDevPath, FileDevPath);
-      Status  = InternalOpenFileDevicePath (DevPath, &FileHandle, EFI_FILE_MODE_READ, 0);
+
+      if (DevPath == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Done;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
+      Status = InternalOpenFileDevicePath (DevPath, &FileHandle, EFI_FILE_MODE_READ, 0);
       SHELL_FREE_NON_NULL (FileDevPath);
       SHELL_FREE_NON_NULL (DevPath);
     }

diff --git a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
@@ -1442,6 +1442,14 @@ BcfgDisplayDump (
     if (LoadOption->FilePathListLength != 0) {
       FilePathList  = (UINT8 *)Description + DescriptionSize;
       DevPathString = ConvertDevicePathToText (FilePathList, TRUE, FALSE);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (DevPathString == NULL) {
+        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM), gShellBcfgHiiHandle, L"bcfg");
+        ++Errors;
+        goto Cleanup;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
     }
 
     OptionalDataOffset = sizeof *LoadOption + DescriptionSize +

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
@@ -1378,7 +1378,12 @@ MainCommandDisplayHelp (
   //
   for (CurrentLine = 0; 0 != MainMenuHelpInfo[CurrentLine]; CurrentLine++) {
     InfoString = HiiGetString (gShellDebug1HiiHandle, MainMenuHelpInfo[CurrentLine], NULL);
-    ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (InfoString != NULL) {
+      ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
+    }
+
+    // MU_CHANGE [END] - CodeQL change
   }
 
   //

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
@@ -115,7 +115,12 @@ HMainCommandDisplayHelp (
                               ,
                    NULL
                    );
-    ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (InfoString != NULL) {
+      ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
+    }
+
+    // MU_CHANGE [END] - CodeQL change
   }
 
   //

diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvCfg.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvCfg.c
@@ -473,6 +473,20 @@ ConfigFromFile (
             // print out an error.
             //
             TempDevPathString = ConvertDevicePathToText ((EFI_DEVICE_PATH_PROTOCOL *)(((CHAR8 *)PackageHeader) + sizeof (EFI_HII_PACKAGE_HEADER)), TRUE, TRUE);
+            // MU_CHANGE [BEGIN] - CodeQL change
+            if (TempDevPathString == NULL) {
+              ShellPrintHiiEx (
+                -1,
+                -1,
+                NULL,
+                STRING_TOKEN (STR_GEN_OUT_MEM),
+                gShellDriver1HiiHandle,
+                L"drvcfg"
+                );
+              return (SHELL_OUT_OF_RESOURCES);
+            }
+
+            // MU_CHANGE [END] - CodeQL change
             ShellPrintHiiEx (
               -1,
               -1,

diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/OpenInfo.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/OpenInfo.c
@@ -25,6 +25,7 @@ STATIC CONST CHAR16  StringUnknown[]   = L"Unknown  ";
 
   @retval EFI_SUCCESS           The operation was successful.
   @retval EFI_INVALID_PARAMETER TheHandle was NULL.
+  @retval EFI_OUT_OF_RESOURCES  A memory allocation failed. // MU_CHANGE: CodeQL change
 **/
 EFI_STATUS
 TraverseHandleDatabase (
@@ -102,7 +103,14 @@ TraverseHandleDatabase (
               break;
           }
 
-          HandleIndex     = ConvertHandleToHandleIndex (OpenInfo[OpenInfoIndex].AgentHandle);
+          HandleIndex = ConvertHandleToHandleIndex (OpenInfo[OpenInfoIndex].AgentHandle);
+          // MU_CHANGE [BEGIN] - CodeQL change
+          if (HandleIndex == 0) {
+            FreePool (OpenInfo);
+            FreePool (ProtocolGuidArray);
+            return EFI_OUT_OF_RESOURCES;
+          }
+
           Name            = GetStringNameFromHandle (OpenInfo[OpenInfoIndex].AgentHandle, NULL);
           ControllerIndex = ConvertHandleToHandleIndex (OpenInfo[OpenInfoIndex].ControllerHandle);
           if (ControllerIndex != 0) {
@@ -118,7 +126,7 @@ TraverseHandleDatabase (
               OpenTypeString,
               Name
               );
-          } else {
+          } else if (Name != NULL) {
             ShellPrintHiiEx (
               -1,
               -1,
@@ -133,6 +141,7 @@ TraverseHandleDatabase (
           }
         }
 
+        // MU_CHANGE [END] - CodeQL change
         FreePool (OpenInfo);
       }
     }

diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Unload.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/Unload.c
@@ -25,6 +25,12 @@ DumpLoadedImageProtocolInfo (
   CHAR16  *TheString;
 
   TheString = GetProtocolInformationDump (TheHandle, &gEfiLoadedImageProtocolGuid, TRUE);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (TheString == NULL) {
+    return (EFI_INVALID_PARAMETER);
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   ShellPrintEx (-1, -1, L"%s", TheString);
 

diff --git a/...g/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c b/...g/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
@@ -187,7 +187,7 @@ DoesCacheExist (
   IN UNIT_TEST_FRAMEWORK_HANDLE  FrameworkHandle
   )
 {
-  CHAR16             *FileName; // MU_CHANGE: Use file name and path instead of device path
+  CHAR16             *FileName = NULL; // MU_CHANGE: Use file name and path instead of device path
   EFI_STATUS         Status;
   SHELL_FILE_HANDLE  FileHandle;
 
@@ -196,7 +196,13 @@ DoesCacheExist (
   //
   // MU_CHANGE: Use file name and path instead of device path
   FileName = GetCacheFileName (FrameworkHandle);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (FileName == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a - Failed to get cache file name.\n", __FUNCTION__));
+    return FALSE;
+  }
 
+  // MU_CHANGE [END] - CodeQL change
   //
   // Check to see whether the file exists.  If the file can be opened for
   // reading, it exists.  Otherwise, probably not.