[edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes

["Laszlo Ersek" <lersek@redhat.com>]

Commit 725acd0b9cc0 ("UefiCpuPkg: Avoid assuming only one smmbasehob",
2023-12-12) introduced a helper function called GetSmBase(), replacing the
lookup of the first and only "gSmmBaseHobGuid" GUID HOB, with iterated
lookups plus memory allocation.

This introduced a new failure mode for setting "mCpuHotPlugData.SmBase".
Namely, before commit 725acd0b9cc0, "mCpuHotPlugData.SmBase" would be set
to NULL if and only if the GUID HOB was absent. After the commit, a NULL
assignment would be possible if the GUID HOB was absent, *or* one of the
memory allocations inside GetSmBase() failed.

In relation to this conflation of distinct failure modes, commit
725acd0b9cc0 actually introduced a NULL pointer dereference. Namely, a
NULL "mCpuHotPlugData.SmBase" is not handled properly at all now. We're
going to fix that NULL pointer dereference in a subsequent patch; however,
as a pre-requisite for that we need to tell apart the failure modes of
GetSmBase().

For memory allocation failures, return EFI_OUT_OF_RESOURCES. Move the
"assertion" that SMRAM cannot be exhausted happen out to the caller
(PiCpuSmmEntry()). Strengthen the assertion by adding an explicit
CpuDeadLoop() call. (Note: GetSmBase() *already* calls CpuDeadLoop() if
(NumberOfProcessors != MaxNumberOfCpus).)

For the absence of the GUID HOB, return EFI_NOT_FOUND.

For good measure, make GetSmBase() STATIC (it should have been STATIC from
the start).

This is just a refactoring, no behavioral difference is intended (beyond
the explicit CpuDeadLoop() upon SMRAM exhaustion).

Cc: Dun Tan <dun.tan@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4682
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    context:-U4

 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 40 ++++++++++++++------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index cd394826ffcf..09382945ddb4 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -619,16 +619,23 @@ SmBaseHobCompare (
 
 /**
   Extract SmBase for all CPU from SmmBase HOB.
 
-  @param[in]  MaxNumberOfCpus   Max NumberOfCpus.
+  @param[in]  MaxNumberOfCpus        Max NumberOfCpus.
 
-  @retval SmBaseBuffer          Pointer to SmBase Buffer.
-  @retval NULL                  gSmmBaseHobGuid was not been created.
+  @param[out] AllocatedSmBaseBuffer  Pointer to SmBase Buffer allocated
+                                     by this function. Only set if the
+                                     function returns EFI_SUCCESS.
+
+  @retval EFI_SUCCESS           SmBase Buffer output successfully.
+  @retval EFI_OUT_OF_RESOURCES  Memory allocation failed.
+  @retval EFI_NOT_FOUND         gSmmBaseHobGuid was never created.
 **/
-UINTN *
+STATIC
+EFI_STATUS
 GetSmBase (
-  IN  UINTN  MaxNumberOfCpus
+  IN  UINTN  MaxNumberOfCpus,
+  OUT UINTN  **AllocatedSmBaseBuffer
   )
 {
   UINTN              HobCount;
   EFI_HOB_GUID_TYPE  *GuidHob;
@@ -649,9 +656,9 @@ GetSmBase (
   NumberOfProcessors = 0;
 
   FirstSmmBaseGuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
   if (FirstSmmBaseGuidHob == NULL) {
-    return NULL;
+    return EFI_NOT_FOUND;
   }
 
   GuidHob = FirstSmmBaseGuidHob;
   while (GuidHob != NULL) {
@@ -671,11 +678,10 @@ GetSmBase (
     CpuDeadLoop ();
   }
 
   SmBaseHobs = AllocatePool (sizeof (SMM_BASE_HOB_DATA *) * HobCount);
-  ASSERT (SmBaseHobs != NULL);
   if (SmBaseHobs == NULL) {
-    return NULL;
+    return EFI_OUT_OF_RESOURCES;
   }
 
   //
   // Record each SmmBaseHob pointer in the SmBaseHobs.
@@ -691,9 +697,9 @@ GetSmBase (
   SmBaseBuffer = (UINTN *)AllocatePool (sizeof (UINTN) * (MaxNumberOfCpus));
   ASSERT (SmBaseBuffer != NULL);
   if (SmBaseBuffer == NULL) {
     FreePool (SmBaseHobs);
-    return NULL;
+    return EFI_OUT_OF_RESOURCES;
   }
 
   QuickSort (SmBaseHobs, HobCount, sizeof (SMM_BASE_HOB_DATA *), (BASE_SORT_COMPARE)SmBaseHobCompare, &SortBuffer);
   PrevProcessorIndex = 0;
@@ -713,9 +719,10 @@ GetSmBase (
     PrevProcessorIndex += SmBaseHobs[HobIndex]->NumberOfProcessors;
   }
 
   FreePool (SmBaseHobs);
-  return SmBaseBuffer;
+  *AllocatedSmBaseBuffer = SmBaseBuffer;
+  return EFI_SUCCESS;
 }
 
 /**
   Function to compare 2 MP_INFORMATION2_HOB_DATA pointer based on ProcessorIndex.
@@ -1110,10 +1117,17 @@ PiCpuSmmEntry (
   //
   // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found,
   // means the SmBase relocation has been done.
   //
-  mCpuHotPlugData.SmBase = GetSmBase (mMaxNumberOfCpus);
-  if (mCpuHotPlugData.SmBase != NULL) {
+  mCpuHotPlugData.SmBase = NULL;
+  Status                 = GetSmBase (mMaxNumberOfCpus, &mCpuHotPlugData.SmBase);
+  if (Status == EFI_OUT_OF_RESOURCES) {
+    ASSERT (Status != EFI_OUT_OF_RESOURCES);
+    CpuDeadLoop ();
+  }
+
+  if (!EFI_ERROR (Status)) {
+    ASSERT (mCpuHotPlugData.SmBase != NULL);
     //
     // Check whether the Required TileSize is enough.
     //
     if (TileSize > SIZE_8KB) {
@@ -1125,8 +1139,10 @@ PiCpuSmmEntry (
     }
 
     mSmmRelocated = TRUE;
   } else {
+    ASSERT (Status == EFI_NOT_FOUND);
+    ASSERT (mCpuHotPlugData.SmBase == NULL);
     //
     // When the HOB doesn't exist, allocate new SMBASE itself.
     //
     DEBUG ((DEBUG_INFO, "PiCpuSmmEntry: gSmmBaseHobGuid not found!\n"));



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115417): https://edk2.groups.io/g/devel/message/115417
Mute This Topic: https://groups.io/mt/104341342/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-