UefiCpuPkg: Additional CodeQL fixes (#263)

## Description Includes fixes for an additional set of CodeQL queries. - [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 UefiCpuPkg and boot changes on QemuQ35Pkg to EFI shell. ## Integration Instructions N/A Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
microsoft · May 4, 2023 · 7152456 · 7152456
1 parent 18b9dd8
commit 7152456
Show file tree

Hide file tree

Showing 9 changed files with 280 additions and 28 deletions.
diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathFromText.c b/MdePkg/Library/UefiDevicePathLib/DevicePathFromText.c
@@ -3823,6 +3823,7 @@ UefiDevicePathLibConvertTextToDevicePath (
   while ((DeviceNodeStr = GetNextDeviceNodeStr (&Str, &IsInstanceEnd)) != NULL) {
     DeviceNode = UefiDevicePathLibConvertTextToDeviceNode (DeviceNodeStr);
 
+    // MU_CHANGE - CodeQL Change: Note: DeviceNode may be NULL. That is an expected input in AppendDevicePathNode().
     NewDevicePath = AppendDevicePathNode (DevicePath, DeviceNode);
     if (DevicePath != NULL) {
       FreePool (DevicePath);
@@ -3836,7 +3837,13 @@ UefiDevicePathLibConvertTextToDevicePath (
 
     if (IsInstanceEnd) {
       DeviceNode = (EFI_DEVICE_PATH_PROTOCOL *)AllocatePool (END_DEVICE_PATH_LENGTH);
-      ASSERT (DeviceNode != NULL);
+      // MU_CHANGE [BEGIN] - CodeQL change
+      if (DeviceNode == NULL) {
+        ASSERT (DeviceNode != NULL);
+        return NULL;
+      }
+
+      // MU_CHANGE [END] - CodeQL change
       SetDevicePathEndNode (DeviceNode);
       DeviceNode->SubType = END_INSTANCE_DEVICE_PATH_SUBTYPE;
 

diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c
@@ -174,7 +174,8 @@ McaInitialize (
     }
 
     if (PcdGetBool (PcdIsPowerOnReset)) {
-      for (BankIndex = 0; BankIndex < (UINTN)McgCap.Bits.Count; BankIndex++) {
+      for (BankIndex = 0; BankIndex < (UINT32)McgCap.Bits.Count; BankIndex++) {
+        // MU_CHANGE - CodeQL change
         CPU_REGISTER_TABLE_WRITE64 (
           ProcessorNumber,
           Msr,

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
@@ -71,7 +71,13 @@ SetExceptionHandlerData (
   IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)IdtDescriptor.Base;
 
   Exception0StubHeader = AllocatePool (sizeof (*Exception0StubHeader));
-  ASSERT (Exception0StubHeader != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Exception0StubHeader == NULL) {
+    ASSERT (Exception0StubHeader != NULL);
+    return;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   CopyMem (
     Exception0StubHeader->ExceptionStubHeader,
     (VOID *)ArchGetIdtHandler (&IdtTable[0]),
@@ -165,10 +171,23 @@ InitializeCpuExceptionHandlers (
   RESERVED_VECTORS_DATA   *ReservedVectors;
 
   ReservedVectors = AllocatePool (sizeof (RESERVED_VECTORS_DATA) * CPU_EXCEPTION_NUM);
-  ASSERT (ReservedVectors != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (ReservedVectors == NULL) {
+    ASSERT (ReservedVectors != NULL);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   ExceptionHandlerData = AllocatePool (sizeof (EXCEPTION_HANDLER_DATA));
-  ASSERT (ExceptionHandlerData != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (ExceptionHandlerData == NULL) {
+    ASSERT (ExceptionHandlerData != NULL);
+    FreePool (ReservedVectors);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   ExceptionHandlerData->IdtEntryCount            = CPU_EXCEPTION_NUM;
   ExceptionHandlerData->ReservedVectors          = ReservedVectors;
   ExceptionHandlerData->ExternalInterruptHandler = AllocateZeroPool (sizeof (EFI_CPU_INTERRUPT_HANDLER) * ExceptionHandlerData->IdtEntryCount);

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -95,6 +95,7 @@ CpuInitDataInitialize (
   EFI_STATUS                 Status;
   UINTN                      ProcessorNumber;
   EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
+  CPU_STATUS_INFORMATION     CpuStatusBackupBuffer;  // MU_CHANGE - CodeQL change
   CPU_FEATURES_ENTRY         *CpuFeature;
   CPU_FEATURES_INIT_ORDER    *InitOrder;
   CPU_FEATURES_DATA          *CpuFeaturesData;
@@ -120,7 +121,24 @@ CpuInitDataInitialize (
   Package = 0;
   Thread  = 0;
 
+  // MU_CHANGE [BEGIN] - CodeQL change
+  CpuFeaturesData       = NULL;
+  CpuStatus             = NULL;
+  FirstCore             = NULL;
+  InitOrder             = NULL;
+  Location              = NULL;
+  ThreadCountPerCore    = NULL;
+  ThreadCountPerPackage = NULL;
+  // MU_CHANGE [END] - CodeQL change
+
   CpuFeaturesData = GetCpuFeaturesData ();
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (CpuFeaturesData == NULL) {
+    ASSERT (CpuFeaturesData != NULL);
+    return;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   //
   // Initialize CpuFeaturesData->MpService as early as possile, so later function can use it.
@@ -130,7 +148,13 @@ CpuInitDataInitialize (
   GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
 
   CpuFeaturesData->InitOrder = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (CPU_FEATURES_INIT_ORDER) * NumberOfCpus));
-  ASSERT (CpuFeaturesData->InitOrder != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (CpuFeaturesData->InitOrder == NULL) {
+    ASSERT (CpuFeaturesData->InitOrder != NULL);
+    return;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   ZeroMem (CpuFeaturesData->InitOrder, sizeof (CPU_FEATURES_INIT_ORDER) * NumberOfCpus);
 
   //
@@ -150,19 +174,38 @@ CpuInitDataInitialize (
   CpuFeaturesData->NumberOfCpus = (UINT32)NumberOfCpus;
 
   AcpiCpuData = GetAcpiCpuData ();
-  ASSERT (AcpiCpuData != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (AcpiCpuData == NULL) {
+    ASSERT (AcpiCpuData != NULL);
+    goto ExitOnError;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   CpuFeaturesData->AcpiCpuData = AcpiCpuData;
 
   CpuStatus = &AcpiCpuData->CpuFeatureInitData.CpuStatus;
-  Location  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (EFI_CPU_PHYSICAL_LOCATION) * NumberOfCpus));
-  ASSERT (Location != NULL);
+  CopyMem (&CpuStatusBackupBuffer, CpuStatus, sizeof (CpuStatusBackupBuffer)); // MU_CHANGE - CodeQL change
+  Location = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (EFI_CPU_PHYSICAL_LOCATION) * NumberOfCpus));
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Location == NULL) {
+    ASSERT (Location != NULL);
+    goto ExitOnError;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   ZeroMem (Location, sizeof (EFI_CPU_PHYSICAL_LOCATION) * NumberOfCpus);
   AcpiCpuData->CpuFeatureInitData.ApLocation = (EFI_PHYSICAL_ADDRESS)(UINTN)Location;
 
   for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus; ProcessorNumber++) {
     InitOrder                        = &CpuFeaturesData->InitOrder[ProcessorNumber];
     InitOrder->FeaturesSupportedMask = AllocateZeroPool (CpuFeaturesData->BitMaskSize);
-    ASSERT (InitOrder->FeaturesSupportedMask != NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (InitOrder->FeaturesSupportedMask == NULL) {
+      ASSERT (InitOrder->FeaturesSupportedMask != NULL);
+      goto ExitOnError;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     InitializeListHead (&InitOrder->OrderList);
     Status = GetProcessorInformation (ProcessorNumber, &ProcessorInfoBuffer);
     ASSERT_EFI_ERROR (Status);
@@ -214,12 +257,26 @@ CpuInitDataInitialize (
   // Collect valid core count in each package because not all cores are valid.
   //
   ThreadCountPerPackage = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (UINT32) * CpuStatus->PackageCount));
-  ASSERT (ThreadCountPerPackage != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (ThreadCountPerPackage == NULL) {
+    ASSERT (ThreadCountPerPackage != NULL);
+    goto ExitOnError;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+
   ZeroMem (ThreadCountPerPackage, sizeof (UINT32) * CpuStatus->PackageCount);
   CpuStatus->ThreadCountPerPackage = (EFI_PHYSICAL_ADDRESS)(UINTN)ThreadCountPerPackage;
 
   ThreadCountPerCore = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (UINT8) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount));
-  ASSERT (ThreadCountPerCore != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (ThreadCountPerCore == NULL) {
+    ASSERT (ThreadCountPerCore != NULL);
+    goto ExitOnError;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+
   ZeroMem (ThreadCountPerCore, sizeof (UINT8) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount);
   CpuStatus->ThreadCountPerCore = (EFI_PHYSICAL_ADDRESS)(UINTN)ThreadCountPerCore;
 
@@ -247,17 +304,34 @@ CpuInitDataInitialize (
   }
 
   CpuFeaturesData->CpuFlags.CoreSemaphoreCount = AllocateZeroPool (sizeof (UINT32) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount);
-  ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (CpuFeaturesData->CpuFlags.CoreSemaphoreCount == NULL) {
+    ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL);
+    goto ExitOnError;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+
   CpuFeaturesData->CpuFlags.PackageSemaphoreCount = AllocateZeroPool (sizeof (UINT32) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount);
-  ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (CpuFeaturesData->CpuFlags.PackageSemaphoreCount == NULL) {
+    ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL);
+    goto ExitOnError;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   //
   // Initialize CpuFeaturesData->InitOrder[].CpuInfo.First
   // Use AllocatePages () instead of AllocatePool () because pool cannot be freed in PEI phase but page can.
   //
   Pages     = EFI_SIZE_TO_PAGES (CpuStatus->PackageCount * sizeof (UINT32) + CpuStatus->PackageCount * CpuStatus->MaxCoreCount * sizeof (UINT32));
   FirstCore = AllocatePages (Pages);
-  ASSERT (FirstCore != NULL);
+  if (FirstCore == NULL) {
+    ASSERT (FirstCore != NULL);
+    goto ExitOnError;
+  }
+
   FirstThread = FirstCore + CpuStatus->PackageCount;
 
   //
@@ -317,6 +391,63 @@ CpuInitDataInitialize (
   }
 
   FreePages (FirstCore, Pages);
+
+  return;
+
+  // MU_CHANGE [BEGIN] - CodeQL change
+ExitOnError:
+  if (FirstCore != NULL) {
+    FreePages (FirstCore, Pages);
+  }
+
+  if ((CpuFeaturesData != NULL) && (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL)) {
+    FreePool ((VOID *)CpuFeaturesData->CpuFlags.PackageSemaphoreCount);
+    CpuFeaturesData->CpuFlags.PackageSemaphoreCount = NULL;
+  }
+
+  if ((CpuFeaturesData != NULL) && (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL)) {
+    FreePool ((VOID *)CpuFeaturesData->CpuFlags.CoreSemaphoreCount);
+    CpuFeaturesData->CpuFlags.CoreSemaphoreCount = NULL;
+  }
+
+  if (ThreadCountPerCore != NULL) {
+    FreePages (
+      ThreadCountPerCore,
+      EFI_SIZE_TO_PAGES (sizeof (UINT8) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount)
+      );
+  }
+
+  if (ThreadCountPerPackage != NULL) {
+    FreePages (
+      ThreadCountPerPackage,
+      EFI_SIZE_TO_PAGES (sizeof (UINT32) * CpuStatus->PackageCount)
+      );
+  }
+
+  if (InitOrder != NULL) {
+    for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus; ProcessorNumber++) {
+      InitOrder = &CpuFeaturesData->InitOrder[ProcessorNumber];
+      if (InitOrder->FeaturesSupportedMask != NULL) {
+        FreePool (InitOrder->FeaturesSupportedMask);
+        InitOrder->FeaturesSupportedMask = NULL;
+      }
+    }
+  }
+
+  if (Location != NULL) {
+    FreePages (Location, EFI_SIZE_TO_PAGES (sizeof (EFI_CPU_PHYSICAL_LOCATION) * NumberOfCpus));
+  }
+
+  if (CpuFeaturesData->InitOrder != NULL) {
+    FreePages (CpuFeaturesData->InitOrder, EFI_SIZE_TO_PAGES (sizeof (CPU_FEATURES_INIT_ORDER) * NumberOfCpus));
+    CpuFeaturesData->InitOrder = NULL;
+  }
+
+  if (CpuStatus != NULL) {
+    CopyMem (CpuStatus, &CpuStatusBackupBuffer, sizeof (*CpuStatus));
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 }
 
 /**

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -966,6 +966,8 @@ RegisterCpuFeature (
   Return ACPI_CPU_DATA data.
 
   @return  Pointer to ACPI_CPU_DATA data.
+           NULL if the ACPI CPU data structure cannot be allocated.   // MU_CHANGE - CodeQL change
+
 **/
 ACPI_CPU_DATA *
 GetAcpiCpuData (
@@ -984,7 +986,13 @@ GetAcpiCpuData (
   AcpiCpuData = (ACPI_CPU_DATA *)(UINTN)PcdGet64 (PcdCpuS3DataAddress);
   if (AcpiCpuData == NULL) {
     AcpiCpuData = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
-    ASSERT (AcpiCpuData != NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (AcpiCpuData == NULL) {
+      ASSERT (AcpiCpuData != NULL);
+      return NULL;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
     ZeroMem (AcpiCpuData, sizeof (ACPI_CPU_DATA));
 
     //
@@ -1006,7 +1014,15 @@ GetAcpiCpuData (
     NumberOfCpus  = AcpiCpuData->NumberOfCpus;
     TableSize     = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
     RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
-    ASSERT (RegisterTable != NULL);
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (RegisterTable == NULL) {
+      // Leave the AcpiCpuData data buffer allocated since it was assigned to a dynamic PCD
+      // which could have invoked PCD set callbacks that may have cached the buffer.
+      ASSERT (RegisterTable != NULL);
+      return NULL;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
 
     for (Index = 0; Index < NumberOfCpus; Index++) {
       Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
@@ -1111,7 +1127,14 @@ CpuRegisterTableWriteWorker (
   CpuFeaturesData = GetCpuFeaturesData ();
   if (CpuFeaturesData->RegisterTable == NULL) {
     AcpiCpuData = GetAcpiCpuData ();
-    ASSERT ((AcpiCpuData != NULL) && (AcpiCpuData->CpuFeatureInitData.RegisterTable != 0));
+    // MU_CHANGE [BEGIN] - CodeQL change
+    if (AcpiCpuData == NULL) {
+      ASSERT (AcpiCpuData != NULL);
+      return;
+    }
+
+    // MU_CHANGE [END] - CodeQL change
+    ASSERT (AcpiCpuData->CpuFeatureInitData.RegisterTable != 0);
     CpuFeaturesData->RegisterTable       = (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->CpuFeatureInitData.RegisterTable;
     CpuFeaturesData->PreSmmRegisterTable = (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->CpuFeatureInitData.PreSmmInitRegisterTable;
   }

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1206,10 +1206,23 @@ AllocateTokenBuffer (
   // Separate the Spin_lock and Proc_token because the alignment requires by Spin_Lock.
   //
   SpinLockBuffer = AllocatePool (SpinLockSize * TokenCountPerChunk);
-  ASSERT (SpinLockBuffer != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (SpinLockBuffer == NULL) {
+    ASSERT (SpinLockBuffer != NULL);
+    return NULL;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   ProcTokens = AllocatePool (sizeof (PROCEDURE_TOKEN) * TokenCountPerChunk);
-  ASSERT (ProcTokens != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (ProcTokens == NULL) {
+    ASSERT (ProcTokens != NULL);
+    FreePool (SpinLockBuffer);
+    return NULL;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   for (Index = 0; Index < TokenCountPerChunk; Index++) {
     SpinLock = (SPIN_LOCK *)(SpinLockBuffer + SpinLockSize * Index);

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -827,7 +827,13 @@ GetSmiCommandPort (
   Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)EfiLocateFirstAcpiTable (
                                                         EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE
                                                         );
-  ASSERT (Fadt != NULL);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Fadt == NULL) {
+    ASSERT (Fadt != NULL);
+    return;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   mSmiCommandPort = Fadt->SmiCmd;
   DEBUG ((DEBUG_INFO, "mSmiCommandPort = %x\n", mSmiCommandPort));