[Cherry-pick] CodeQL Fixes - Second Pass from 202202

Makes changes to comply with alerts raised by CodeQL. A prior pass with a subset of queries used here was already done so this is mostly addressing issues discovered by enabling a few new queries. Most of the issues here fall into the following two categories: 1. Potential use of uninitialized pointer 2. Inconsistent integer width used in loop comparison - [ ] Breaking change? - Will this change break pre-existing builds or functionality without action being taken? **No** - Should just be refactoring to address alerts 1. Verified CI build 2. Verified boot to EFI shell on QemuQ35Pkg N/A Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> (cherry picked from commit 43e657c)
microsoft · May 9, 2023 · 881e889 · 881e889
1 parent 3c8294b
commit 881e889
Show file tree

Hide file tree

Showing 25 changed files with 121 additions and 59 deletions.
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1859,7 +1859,7 @@ PciProgramResizableBar (
                                     );
   ASSERT_EFI_ERROR (Status);
 
-  for (Index = 0; Index < ResizableBarNumber; Index++) {
+  for (Index = 0; (UINTN)Index < ResizableBarNumber; Index++) {
     //
     // When the bit of Capabilities Set, indicates that the Function supports
     // operating with the BAR sized to (2^Bit) MB.

diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
@@ -2615,7 +2615,7 @@ ScsiDiskInquiryDevice (
         //
         // Locate the code for the Block Limits VPD page
         //
-        for (Index = 0; Index < PageLength; Index++) {
+        for (Index = 0; (UINTN)Index < PageLength; Index++) {
           //
           // Sanity check
           //

diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -405,7 +405,7 @@ UfsInitUtpPrdt (
   Remaining    = Buffer;
   PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize + UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
 
-  for (PrdtIndex = 0; PrdtIndex < PrdtNumber; PrdtIndex++) {
+  for (PrdtIndex = 0; (UINTN)PrdtIndex < PrdtNumber; PrdtIndex++) {
     if (RemainingLen < UFS_MAX_DATA_LEN_PER_PRD) {
       Prdt[PrdtIndex].DbCount = (UINT32)RemainingLen - 1;
     } else {

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -1166,16 +1166,7 @@ CoreOpenProtocol (
     // Keep Interface unmodified in case of any Error
     // except EFI_ALREADY_STARTED and EFI_UNSUPPORTED.
     //
-    if (!EFI_ERROR (Status) || (Status == EFI_ALREADY_STARTED)) {
-      //
-      // According to above logic, if 'Prot' is NULL, then the 'Status' must be
-      // EFI_UNSUPPORTED. Here the 'Status' is not EFI_UNSUPPORTED, so 'Prot'
-      // must be not NULL.
-      //
-      // The ASSERT here is for addressing a false positive NULL pointer
-      // dereference issue raised from static analysis.
-      //
-      ASSERT (Prot != NULL);
+    if ((!EFI_ERROR (Status) || (Status == EFI_ALREADY_STARTED)) && (Prot != NULL)) {
       //
       // EFI_ALREADY_STARTED is not an error for bus driver.
       // Return the corresponding protocol interface.

diff --git a/MdeModulePkg/Core/Pei/FwVol/FwVol.c b/MdeModulePkg/Core/Pei/FwVol/FwVol.c
@@ -338,7 +338,7 @@ FindFileEx (
   FileOffset = (UINT32)((UINT8 *)FfsFileHeader - (UINT8 *)FwVolHeader);
   ASSERT (FileOffset <= 0xFFFFFFFF);
 
-  while (FileOffset < (FvLength - sizeof (EFI_FFS_FILE_HEADER))) {
+  while ((UINTN)FileOffset < (UINTN)(FvLength - sizeof (EFI_FFS_FILE_HEADER))) {
     //
     // Get FileState which is the highest bit of the State
     //

diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxMmLib.c b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxMmLib.c
@@ -506,7 +506,11 @@ SaveLockBox (
     ));
 
   LockBoxQueue = InternalGetLockBoxQueue ();
-  ASSERT (LockBoxQueue != NULL);
+  if (LockBoxQueue == NULL) {
+    ASSERT (LockBoxQueue != NULL);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
   InsertTailList (LockBoxQueue, &LockBox->Link);
 
   //
@@ -840,6 +844,7 @@ RestoreLockBox (
   @retval RETURN_SUCCESS            the information is restored successfully.
   @retval RETURN_NOT_STARTED        it is too early to invoke this interface
   @retval RETURN_UNSUPPORTED        the service is not supported by implementaion.
+  @retval RETURN_OUT_OF_RESOURCES   noT enough resourceS to save the information.
 **/
 RETURN_STATUS
 EFIAPI
@@ -854,7 +859,10 @@ RestoreAllLockBoxInPlace (
   DEBUG ((DEBUG_INFO, "SmmLockBoxSmmLib RestoreAllLockBoxInPlace - Enter\n"));
 
   LockBoxQueue = InternalGetLockBoxQueue ();
-  ASSERT (LockBoxQueue != NULL);
+  if (LockBoxQueue == NULL) {
+    ASSERT (LockBoxQueue != NULL);
+    return EFI_OUT_OF_RESOURCES;
+  }
 
   //
   // Restore all, Buffer and Length MUST be NULL

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -2304,12 +2304,20 @@ BmEnumerateBootOptions (
       }
 
       Description = BmGetBootDescription (Handles[Index]);
+      if (Description == NULL) {
+        continue;
+      }
+
       BootOptions = ReallocatePool (
                       sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount),
                       sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount + 1),
                       BootOptions
                       );
-      ASSERT (BootOptions != NULL);
+      if (BootOptions == NULL) {
+        ASSERT (BootOptions != NULL);
+        FreePool (Description);
+        continue;
+      }
 
       Status = EfiBootManagerInitializeLoadOption (
                  &BootOptions[(*BootOptionCount)++],
@@ -2355,12 +2363,20 @@ BmEnumerateBootOptions (
     }
 
     Description = BmGetBootDescription (Handles[Index]);
+    if (Description == NULL) {
+      continue;
+    }
+
     BootOptions = ReallocatePool (
                     sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount),
                     sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount + 1),
                     BootOptions
                     );
-    ASSERT (BootOptions != NULL);
+    if (BootOptions == NULL) {
+      ASSERT (BootOptions != NULL);
+      FreePool (Description);
+      continue;
+    }
 
     Status = EfiBootManagerInitializeLoadOption (
                &BootOptions[(*BootOptionCount)++],
@@ -2399,12 +2415,20 @@ BmEnumerateBootOptions (
     }
 
     Description = BmGetBootDescription (Handles[Index]);
+    if (Description == NULL) {
+      continue;
+    }
+
     BootOptions = ReallocatePool (
                     sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount),
                     sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount + 1),
                     BootOptions
                     );
-    ASSERT (BootOptions != NULL);
+    if (BootOptions == NULL) {
+      ASSERT (BootOptions != NULL);
+      FreePool (Description);
+      continue;
+    }
 
     Status = EfiBootManagerInitializeLoadOption (
                &BootOptions[(*BootOptionCount)++],

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c b/MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c
@@ -484,17 +484,19 @@ EfiBootManagerUpdateConsoleVariable (
     }
   }
 
-  //
-  // Finally, Update the variable of the default console by NewDevicePath
-  //
-  Status = gRT->SetVariable (
-                  mConVarName[ConsoleType],
-                  &gEfiGlobalVariableGuid,
-                  EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS
-                  | ((ConsoleType < ConInDev) ? EFI_VARIABLE_NON_VOLATILE : 0),
-                  GetDevicePathSize (NewDevicePath),
-                  NewDevicePath
-                  );
+  if (NewDevicePath != NULL) {
+    //
+    // Finally, Update the variable of the default console by NewDevicePath
+    //
+    Status = gRT->SetVariable (
+                    mConVarName[ConsoleType],
+                    &gEfiGlobalVariableGuid,
+                    EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS
+                    | ((ConsoleType < ConInDev) ? EFI_VARIABLE_NON_VOLATILE : 0),
+                    GetDevicePathSize (NewDevicePath),
+                    NewDevicePath
+                    );
+  }
 
   if (VarConsole == NewDevicePath) {
     if (VarConsole != NULL) {

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
@@ -454,6 +454,9 @@ EfiBootManagerSortLoadOptionVariable (
   UINT16                        *OptionOrder;
 
   LoadOption = EfiBootManagerGetLoadOptions (&LoadOptionCount, OptionType);
+  if (LoadOption == NULL) {
+    return;
+  }
 
   //
   // Insertion sort algorithm

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
@@ -57,7 +57,10 @@ BmDelPartMatchInstance (
       }
     }
 
-    FreePool (Instance);
+    if (Instance != NULL) {
+      FreePool (Instance);
+    }
+
     Instance      = GetNextDevicePathInstance (&Multi, &InstanceSize);
     InstanceSize -= END_DEVICE_PATH_LENGTH;
   }

diff --git a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
@@ -1216,7 +1216,7 @@ ValidateQuestionFromVfr (
   // Check IFR value is in block data, then Validate Value
   //
   PackageOffset = sizeof (EFI_HII_PACKAGE_LIST_HEADER);
-  while (PackageOffset < PackageListLength) {
+  while ((UINTN)PackageOffset < PackageListLength) {
     CopyMem (&PackageHeader, (UINT8 *)HiiPackageList + PackageOffset, sizeof (PackageHeader));
 
     //

diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AmlNamespace.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AmlNamespace.c
@@ -32,7 +32,7 @@ AmlConstructNodeList (
   @param[in]    Parent               AML parent node list.
   @param[in]    AmlByteEncoding      AML Byte Encoding.
 
-  @return       AML Node.
+  @return       AML Node or NULL if insufficient resources to allocate a buffer
 **/
 EFI_AML_NODE_LIST *
 AmlCreateNode (
@@ -44,7 +44,10 @@ AmlCreateNode (
   EFI_AML_NODE_LIST  *AmlNodeList;
 
   AmlNodeList = AllocatePool (sizeof (*AmlNodeList));
-  ASSERT (AmlNodeList != NULL);
+  if (AmlNodeList == NULL) {
+    ASSERT (AmlNodeList != NULL);
+    return NULL;
+  }
 
   AmlNodeList->Signature = EFI_AML_NODE_LIST_SIGNATURE;
   CopyMem (AmlNodeList->Name, NameSeg, AML_NAME_SEG_SIZE);

diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AmlString.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AmlString.c
@@ -379,7 +379,7 @@ AmlUpperCaseCopyMem (
 
   @param[in]    AslPath     ASL name.
 
-  @return AmlName
+  @return AmlName or NULL if insufficient resources to allocate a buffer
 **/
 UINT8 *
 AmlNameFromAslName (
@@ -401,7 +401,10 @@ AmlNameFromAslName (
   }
 
   AmlPath = AllocatePool (TotalLength);
-  ASSERT (AmlPath != NULL);
+  if (AmlPath == NULL) {
+    ASSERT (AmlPath != NULL);
+    return NULL;
+  }
 
   AmlBuffer = AmlPath;
   Buffer    = AslPath;

diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
@@ -466,7 +466,7 @@ GraphicsConsoleControllerDriverStart (
       //
       MaxMode = Private->GraphicsOutput->Mode->MaxMode;
 
-      for (ModeIndex = 0; ModeIndex < MaxMode; ModeIndex++) {
+      for (ModeIndex = 0; (UINTN)ModeIndex < MaxMode; ModeIndex++) {
         Status = Private->GraphicsOutput->QueryMode (
                                             Private->GraphicsOutput,
                                             ModeIndex,

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Mbr.c
@@ -359,7 +359,7 @@ PartitionInstallMbrChildHandles (
       if (ExtMbrStartingLba == 0) {
         break;
       }
-    } while (ExtMbrStartingLba  < ParentHdDev.PartitionSize);
+    } while ((UINT64)ExtMbrStartingLba  < ParentHdDev.PartitionSize);
   }
 
 Done:

diff --git a/MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.c b/MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.c
@@ -102,7 +102,7 @@ FtwGetLastWriteRecord (
   //
   // Try to find the last write record "that has not completed"
   //
-  for (Index = 0; Index < FtwWriteHeader->NumberOfWrites; Index += 1) {
+  for (Index = 0; (UINT64)Index < FtwWriteHeader->NumberOfWrites; Index += 1) {
     if (FtwRecord->DestinationComplete != FTW_VALID_STATE) {
       //
       // The last write record is found

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c
@@ -412,15 +412,15 @@ GlyphToBlt (
   // The glyph's upper left hand corner pixel is the most significant bit of the
   // first bitmap byte.
   //
-  for (Ypos = 0; Ypos < Cell->Height && (((UINT32)Ypos + YposOffset) < RowHeight); Ypos++) {
+  for (Ypos = 0; Ypos < Cell->Height && ((UINTN)((UINT32)Ypos + YposOffset) < RowHeight); Ypos++) {
     OffsetY = BITMAP_LEN_1_BIT (Cell->Width, Ypos);
 
     //
     // All bits in these bytes are meaningful.
     //
     for (Xpos = 0; Xpos < Cell->Width / 8; Xpos++) {
       Data = *(GlyphBuffer + OffsetY + Xpos);
-      for (Index = 0; Index < 8 && (((UINT32)Xpos * 8 + Index + Cell->OffsetX) < RowWidth); Index++) {
+      for (Index = 0; Index < 8 && ((UINTN)((UINT32)Xpos * 8 + Index + Cell->OffsetX) < RowWidth); Index++) {
         if ((Data & (1 << (8 - Index - 1))) != 0) {
           BltBuffer[Ypos * ImageWidth + Xpos * 8 + Index] = Foreground;
         } else {
@@ -436,7 +436,7 @@ GlyphToBlt (
       // There are some padding bits in this byte. Ignore them.
       //
       Data = *(GlyphBuffer + OffsetY + Xpos);
-      for (Index = 0; Index < Cell->Width % 8 && (((UINT32)Xpos * 8 + Index + Cell->OffsetX) < RowWidth); Index++) {
+      for (Index = 0; Index < (UINT16)(Cell->Width % 8) && ((UINTN)((UINT32)Xpos * 8 + Index + Cell->OffsetX) < RowWidth); Index++) {
         if ((Data & (1 << (8 - Index - 1))) != 0) {
           BltBuffer[Ypos * ImageWidth + Xpos * 8 + Index] = Foreground;
         } else {

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -300,7 +300,7 @@ Output1bitPixel (
       // Padding bits in this byte should be ignored.
       //
       Byte = *(Data + OffsetY + Xpos);
-      for (Index = 0; Index < Image->Width % 8; Index++) {
+      for (Index = 0; (UINT16)Index < (UINT16)(Image->Width % 8); Index++) {
         if ((Byte & (1 << (8 - Index - 1))) != 0) {
           CopyMem (&BitMapPtr[Ypos * Image->Width + Xpos * 8 + Index], &PaletteValue[1], sizeof (*BitMapPtr));
         } else {

diff --git a/MdeModulePkg/Universal/PCD/Pei/Pcd.c b/MdeModulePkg/Universal/PCD/Pei/Pcd.c
@@ -574,7 +574,7 @@ PeiPcdSetSku (
   }
 
   SkuIdTable = (SKU_ID *)((UINT8 *)PeiPcdDb + PeiPcdDb->SkuIdTableOffset);
-  for (Index = 0; Index < SkuIdTable[0]; Index++) {
+  for (Index = 0; (UINT64)Index < SkuIdTable[0]; Index++) {
     if (SkuId == SkuIdTable[Index + 1]) {
       DEBUG ((DEBUG_INFO, "PcdPei - SkuId is found in SkuId table.\n"));
       break;

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
@@ -478,6 +478,8 @@ InternalProtocolGetVariablePolicyInfo (
   UINTN                                  BufferSize;
   UINTN                                  VariableNameSize;
 
+  PolicyHeader = NULL;
+
   if ((VariableName == NULL) || (VendorGuid == NULL) || (VariablePolicy == NULL)) {
     return EFI_INVALID_PARAMETER;
   }
@@ -611,7 +613,11 @@ InternalProtocolGetVariablePolicyInfo (
 Done:
   ReleaseLockOnlyAtBootTime (&mMmCommunicationLock);
 
-  return (EFI_ERROR (Status)) ? Status : PolicyHeader->Result;
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  return (PolicyHeader != NULL) ? PolicyHeader->Result : EFI_SUCCESS;
 }
 
 /**

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -102,7 +102,7 @@ PeCoffLoaderGetPeHeader (
   UINTN                     Size;
   UINTN                     ReadSize;
   UINT32                    SectionHeaderOffset;
-  UINT32                    Index;
+  UINTN                     Index;
   UINT32                    HeaderWithoutDataDir;
   CHAR8                     BufferData;
   UINTN                     NumberOfSections;