Fix CodeQL errors (#490)

Fixed some CodeQL failures we're seeing in a variety of packages. This takes the changes from the PR found [here](#488) and builds upon them. - [ ] 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, ... Tested through CodeQL checks. N/A
microsoft · Dec 18, 2023 · 2c4db39 · 2c4db39
1 parent 3b51a4a
commit 2c4db39
Show file tree

Hide file tree

Showing 17 changed files with 254 additions and 49 deletions.
diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/UnitTestMain.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/UnitTestMain.c
@@ -26,13 +26,23 @@ UefiTestMain (
   UNIT_TEST_FRAMEWORK_HANDLE  Framework;
 
   DEBUG ((DEBUG_INFO, "%a v%a\n", UNIT_TEST_NAME, UNIT_TEST_VERSION));
-  CreateUnitTest (UNIT_TEST_NAME, UNIT_TEST_VERSION, &Framework);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  Status = CreateUnitTest (UNIT_TEST_NAME, UNIT_TEST_VERSION, &Framework);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestsfor BaseCryptLib Tests!  Status = %r\n", Status));
+    goto Done;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   //
   // Execute the tests.
   //
   Status = RunAllTestSuites (Framework);
 
+  // MU_CHANGE [BEGIN] - CodeQL change
+Done:
+  // MU_CHANGE [END] - CodeQL change
   if (Framework) {
     FreeUnitTestFramework (Framework);
   }

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/X509Tests.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/X509Tests.c
@@ -573,7 +573,14 @@ TestVerifyX509 (
   Status      = X509GetIssuerName (mTestCert, sizeof (mTestCert), NULL, &SubjectSize);
   UT_ASSERT_TRUE (!Status);
   Subject = AllocatePool (SubjectSize);
-  Status  = X509GetIssuerName (mTestCert, sizeof (mTestCert), Subject, &SubjectSize);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Subject == NULL) {
+    ASSERT (Subject != NULL);
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+  Status = X509GetIssuerName (mTestCert, sizeof (mTestCert), Subject, &SubjectSize);
   UT_ASSERT_TRUE (Status);
   FreePool (Subject);
 

diff --git a/MdeModulePkg/Core/Pei/Ppi/Ppi.c b/MdeModulePkg/Core/Pei/Ppi/Ppi.c
@@ -324,16 +324,9 @@ ConvertPpiPointersFv (
 
     Guid = PrivateData->PpiData.PpiList.PpiPtrs[Index].Ppi->Guid;
     for (GuidIndex = 0; GuidIndex < ARRAY_SIZE (GuidCheckList); ++GuidIndex) {
-      //
-      // Don't use CompareGuid function here for performance reasons.
-      // Instead we compare the GUID as INT32 at a time and branch
-      // on the first failed comparison.
-      //
-      if ((((INT32 *)Guid)[0] == ((INT32 *)GuidCheckList[GuidIndex])[0]) &&
-          (((INT32 *)Guid)[1] == ((INT32 *)GuidCheckList[GuidIndex])[1]) &&
-          (((INT32 *)Guid)[2] == ((INT32 *)GuidCheckList[GuidIndex])[2]) &&
-          (((INT32 *)Guid)[3] == ((INT32 *)GuidCheckList[GuidIndex])[3]))
-      {
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (CompareGuid (Guid, GuidCheckList[GuidIndex]) == 0) {
+        // MU_CHANGE [END] - CodeQL change
         FvInfoPpi = PrivateData->PpiData.PpiList.PpiPtrs[Index].Ppi->Ppi;
         DEBUG ((DEBUG_VERBOSE, "      FvInfo: %p -> ", FvInfoPpi->FvInfo));
         if ((UINTN)FvInfoPpi->FvInfo == OrgFvHandle) {

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -1457,6 +1457,11 @@ GetFullSmramRanges (
   EFI_SMM_RESERVED_SMRAM_REGION   *SmramReservedRanges;
   UINTN                           MaxCount;
   BOOLEAN                         Rescan;
+  // MU_CHANGE [BEGIN] - CodeQL change
+  BOOLEAN  Failed;
+
+  Failed = FALSE;
+  // MU_CHANGE [END] - CodeQL change
 
   //
   // Get SMM Configuration Protocol if it is present.
@@ -1501,7 +1506,14 @@ GetFullSmramRanges (
     *FullSmramRangeCount = SmramRangeCount + AdditionSmramRangeCount;
     Size                 = (*FullSmramRangeCount) * sizeof (EFI_SMRAM_DESCRIPTOR);
     FullSmramRanges      = (EFI_SMRAM_DESCRIPTOR *)AllocateZeroPool (Size);
-    ASSERT (FullSmramRanges != NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (FullSmramRanges == NULL) {
+      ASSERT (FullSmramRanges != NULL);
+      Failed = TRUE;
+      goto Done;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
 
     Status = mSmmAccess->GetCapabilities (mSmmAccess, &Size, FullSmramRanges);
     ASSERT_EFI_ERROR (Status);
@@ -1548,18 +1560,41 @@ GetFullSmramRanges (
 
   Size                = MaxCount * sizeof (EFI_SMM_RESERVED_SMRAM_REGION);
   SmramReservedRanges = (EFI_SMM_RESERVED_SMRAM_REGION *)AllocatePool (Size);
-  ASSERT (SmramReservedRanges != NULL);
+
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (SmramReservedRanges == NULL) {
+    ASSERT (SmramReservedRanges != NULL);
+    Failed = TRUE;
+    goto Done;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+
   for (Index = 0; Index < SmramReservedCount; Index++) {
     CopyMem (&SmramReservedRanges[Index], &SmmConfiguration->SmramReservedRegions[Index], sizeof (EFI_SMM_RESERVED_SMRAM_REGION));
   }
 
   Size            = MaxCount * sizeof (EFI_SMRAM_DESCRIPTOR);
   TempSmramRanges = (EFI_SMRAM_DESCRIPTOR *)AllocatePool (Size);
-  ASSERT (TempSmramRanges != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (TempSmramRanges == NULL) {
+    ASSERT (TempSmramRanges != NULL);
+    Failed = TRUE;
+    goto Done;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   TempSmramRangeCount = 0;
 
   SmramRanges = (EFI_SMRAM_DESCRIPTOR *)AllocatePool (Size);
-  ASSERT (SmramRanges != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (SmramRanges == NULL) {
+    ASSERT (SmramRanges != NULL);
+    Failed = TRUE;
+    goto Done;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   Status = mSmmAccess->GetCapabilities (mSmmAccess, &Size, SmramRanges);
   ASSERT_EFI_ERROR (Status);
 
@@ -1616,7 +1651,14 @@ GetFullSmramRanges (
   // Sort the entries
   //
   FullSmramRanges = AllocateZeroPool ((TempSmramRangeCount + AdditionSmramRangeCount) * sizeof (EFI_SMRAM_DESCRIPTOR));
-  ASSERT (FullSmramRanges != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (FullSmramRanges == NULL) {
+    ASSERT (FullSmramRanges != NULL);
+    Failed = TRUE;
+    goto Done;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   *FullSmramRangeCount = 0;
   do {
     for (Index = 0; Index < TempSmramRangeCount; Index++) {
@@ -1640,9 +1682,25 @@ GetFullSmramRanges (
   ASSERT (*FullSmramRangeCount == TempSmramRangeCount);
   *FullSmramRangeCount += AdditionSmramRangeCount;
 
-  FreePool (SmramRanges);
-  FreePool (SmramReservedRanges);
-  FreePool (TempSmramRanges);
+  // MU_CHANGE [BEGIN] - CodeQL change
+Done:
+  if (SmramRanges != NULL) {
+    FreePool (SmramRanges);
+  }
+
+  if (SmramReservedRanges != NULL) {
+    FreePool (SmramReservedRanges);
+  }
+
+  if (TempSmramRanges != NULL) {
+    FreePool (TempSmramRanges);
+  }
+
+  if (Failed) {
+    return NULL;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   return FullSmramRanges;
 }

diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c b/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
@@ -1173,6 +1173,14 @@ IfrToString (
       } else {
         SrcBuf = GetBufferForValue (&Value);
         SrcLen = GetLengthForValue (&Value);
+        // MU_CHANGE [BEGIN] - CodeQL change
+        if ((SrcBuf == NULL) || (SrcLen == 0)) {
+          ASSERT (SrcBuf != NULL);
+          ASSERT (SrcLen != 0);
+          return EFI_NOT_FOUND;
+        }
+
+        // MU_CHANGE [END] - CodeQL change
       }
 
       TmpBuf = AllocateZeroPool (SrcLen + 3);
@@ -1183,6 +1191,7 @@ IfrToString (
       }
 
       // MU_CHANGE [END] - CodeQL change
+
       if (Format == EFI_IFR_STRING_ASCII) {
         CopyMem (TmpBuf, SrcBuf, SrcLen);
         PrintFormat = L"%a";
@@ -1292,7 +1301,8 @@ IfrToUint (
   Evaluate opcode EFI_IFR_CATENATE.
 
   @param  FormSet                Formset which contains this opcode.
-  @param  Result                 Evaluation result for this opcode.
+  @param  Result                 Evaluation result for this opcode.  Result
+                                 will be NULL on a failure.
 
   @retval EFI_SUCCESS            Opcode evaluation success.
   @retval Other                  Opcode evaluation failed.
@@ -1377,10 +1387,24 @@ IfrCatenate (
     ASSERT (Result->Buffer != NULL);
 
     TmpBuf = GetBufferForValue (&Value[0]);
-    ASSERT (TmpBuf != NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (TmpBuf == NULL) {
+      ASSERT (TmpBuf != NULL);
+      Status = EFI_OUT_OF_RESOURCES;
+      goto Done;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     CopyMem (Result->Buffer, TmpBuf, Length0);
     TmpBuf = GetBufferForValue (&Value[1]);
-    ASSERT (TmpBuf != NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (TmpBuf == NULL) {
+      ASSERT (TmpBuf != NULL);
+      Status = EFI_OUT_OF_RESOURCES;
+      goto Done;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     CopyMem (&Result->Buffer[Length0], TmpBuf, Length1);
   }
 
@@ -1405,6 +1429,13 @@ IfrCatenate (
     FreePool (StringPtr);
   }
 
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (EFI_ERROR (Status) && (Result != NULL)) {
+    FreePool (Result);
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+
   return Status;
 }
 

diff --git a/NetworkPkg/Ip4Dxe/Ip4Input.c b/NetworkPkg/Ip4Dxe/Ip4Input.c
@@ -1318,7 +1318,13 @@ Ip4InstanceDeliverPacket (
         // may be not continuous before the data.
         //
         Head = NetbufAllocSpace (Dup, IP4_MAX_HEADLEN, NET_BUF_HEAD);
-        ASSERT (Head != NULL);
+        // MU_CHANGE [BEGIN] - CodeQL change
+        if (Head == NULL) {
+          ASSERT (Head != NULL);
+          return EFI_OUT_OF_RESOURCES;
+        }
+
+        // MU_CHANGE [END] - CodeQL change
 
         Dup->Ip.Ip4 = (IP4_HEAD *)Head;
 

diff --git a/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c b/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
@@ -864,19 +864,26 @@ Ip6ManualAddrDadCallback (
         // data with those passed.
         //
         PassedAddr = (EFI_IP6_CONFIG_MANUAL_ADDRESS *)AllocatePool (Item->DataSize);
-        ASSERT (PassedAddr != NULL);
-
-        Item->Data.Ptr = PassedAddr;
-        Item->Status   = EFI_SUCCESS;
-
-        while (!NetMapIsEmpty (&Instance->DadPassedMap)) {
-          ManualAddr = (EFI_IP6_CONFIG_MANUAL_ADDRESS *)NetMapRemoveHead (&Instance->DadPassedMap, NULL);
-          CopyMem (PassedAddr, ManualAddr, sizeof (EFI_IP6_CONFIG_MANUAL_ADDRESS));
+        // MU_CHANGE [BEGIN] - CodeQL change
+        if (PassedAddr == NULL) {
+          ASSERT (PassedAddr != NULL);
+          Item->Data.Ptr = NULL;
+          Item->Status   = EFI_OUT_OF_RESOURCES;
+        } else {
+          Item->Data.Ptr = PassedAddr;
+          Item->Status   = EFI_SUCCESS;
+
+          while (!NetMapIsEmpty (&Instance->DadPassedMap)) {
+            ManualAddr = (EFI_IP6_CONFIG_MANUAL_ADDRESS *)NetMapRemoveHead (&Instance->DadPassedMap, NULL);
+            CopyMem (PassedAddr, ManualAddr, sizeof (EFI_IP6_CONFIG_MANUAL_ADDRESS));
+
+            PassedAddr++;
+          }
 
-          PassedAddr++;
+          ASSERT ((UINTN)PassedAddr - (UINTN)Item->Data.Ptr == Item->DataSize);
         }
 
-        ASSERT ((UINTN)PassedAddr - (UINTN)Item->Data.Ptr == Item->DataSize);
+        // MU_CHANGE [END] - CodeQL change
       }
     } else {
       //

diff --git a/NetworkPkg/Ip6Dxe/Ip6Input.c b/NetworkPkg/Ip6Dxe/Ip6Input.c
@@ -1522,7 +1522,13 @@ Ip6InstanceDeliverPacket (
       // may be not continuous before the data.
       //
       Head = NetbufAllocSpace (Dup, sizeof (EFI_IP6_HEADER), NET_BUF_HEAD);
-      ASSERT (Head != NULL);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (Head == NULL) {
+        ASSERT (Head != NULL);
+        return EFI_OUT_OF_RESOURCES;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       Dup->Ip.Ip6 = (EFI_IP6_HEADER *)Head;
 
       CopyMem (Head, Packet->Ip.Ip6, sizeof (EFI_IP6_HEADER));

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -1445,7 +1445,13 @@ Ip6SendNeighborSolicit (
   IcmpHead->Head.Code = 0;
 
   Target = (EFI_IPv6_ADDRESS *)NetbufAllocSpace (Packet, sizeof (EFI_IPv6_ADDRESS), FALSE);
-  ASSERT (Target != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Target == NULL) {
+    ASSERT (Target != NULL);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   IP6_COPY_ADDRESS (Target, TargetIp6Address);
 
   LinkLayerOption = NULL;

diff --git a/NetworkPkg/Ip6Dxe/Ip6Output.c b/NetworkPkg/Ip6Dxe/Ip6Output.c
@@ -866,7 +866,14 @@ Ip6Output (
       // Allocate the space to contain the fragmentable hdrs and copy the data.
       //
       Buf = NetbufAllocSpace (TmpPacket, FragmentHdrsLen, TRUE);
-      ASSERT (Buf != NULL);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (Buf == NULL) {
+        ASSERT (Buf != NULL);
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Error;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       CopyMem (Buf, ExtHdrs + UnFragmentHdrsLen, FragmentHdrsLen);
 
       //

diff --git a/NetworkPkg/Library/DxeNetLib/NetBuffer.c b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
@@ -303,6 +303,13 @@ NetbufDuplicate (
   NetbufReserve (Duplicate, HeadSpace);
 
   Dst = NetbufAllocSpace (Duplicate, Nbuf->TotalSize, NET_BUF_TAIL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Dst == NULL) {
+    ASSERT (Dst != NULL);
+    return NULL;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   NetbufCopy (Nbuf, 0, Nbuf->TotalSize, Dst);
 
   return Duplicate;