sravan/system76-edk2
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes

Browse Source 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4682

Commit 725acd0b9c ("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 and
unconditional "mCpuHotPlugData.SmBase" allocation, with iterated lookups
plus conditional memory allocation.

This introduced a new failure mode for setting "mCpuHotPlugData.SmBase".
Namely, before commit 725acd0b9c, "mCpuHotPlugData.SmBase" would be
allocated regardless of the GUID HOB being absent. After the commit,
"mCpuHotPlugData.SmBase" could remain NULL if the GUID HOB was absent,
*or* one of the memory allocations inside GetSmBase() failed; and in the
former case, we'd even proceed to the rest of PiCpuSmmEntry().

In relation to this conflation of distinct failure modes, commit
725acd0b9c 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>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
Reviewed-by: Rahul Kumar <rahul1.kumar@intel.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
This commit is contained in:
Laszlo Ersek
2024-02-13 13:09:17 -08:00
committed by mergify[bot]
parent 5fd3078a2e
commit 72c441df36
1 changed files with 28 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
View File

@@ -620,14 +620,21 @@ SmBaseHobCompare (
/** /**
Extract SmBase for all CPU from SmmBase HOB. Extract SmBase for all CPU from SmmBase HOB.
@param[in] MaxNumberOfCpus Max NumberOfCpus. @param[in] MaxNumberOfCpus Max NumberOfCpus.
@retval SmBaseBuffer Pointer to SmBase Buffer. @param[out] AllocatedSmBaseBuffer Pointer to SmBase Buffer allocated
@retval NULL gSmmBaseHobGuid was not been created. 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 ( GetSmBase (
IN UINTN MaxNumberOfCpus IN UINTN MaxNumberOfCpus,
OUT UINTN **AllocatedSmBaseBuffer
) )
{ {
UINTN HobCount; UINTN HobCount;
@@ -650,7 +657,7 @@ GetSmBase (
FirstSmmBaseGuidHob = GetFirstGuidHob (&gSmmBaseHobGuid); FirstSmmBaseGuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
if (FirstSmmBaseGuidHob == NULL) { if (FirstSmmBaseGuidHob == NULL) {
return NULL; return EFI_NOT_FOUND;
} }
GuidHob = FirstSmmBaseGuidHob; GuidHob = FirstSmmBaseGuidHob;
@@ -672,9 +679,8 @@ GetSmBase (
} }
SmBaseHobs = AllocatePool (sizeof (SMM_BASE_HOB_DATA *) * HobCount); SmBaseHobs = AllocatePool (sizeof (SMM_BASE_HOB_DATA *) * HobCount);
ASSERT (SmBaseHobs != NULL);
if (SmBaseHobs == NULL) { if (SmBaseHobs == NULL) {
return NULL; return EFI_OUT_OF_RESOURCES;
} }
// //
@@ -692,7 +698,7 @@ GetSmBase (
ASSERT (SmBaseBuffer != NULL); ASSERT (SmBaseBuffer != NULL);
if (SmBaseBuffer == NULL) { if (SmBaseBuffer == NULL) {
FreePool (SmBaseHobs); FreePool (SmBaseHobs);
return NULL; return EFI_OUT_OF_RESOURCES;
} }
QuickSort (SmBaseHobs, HobCount, sizeof (SMM_BASE_HOB_DATA *), (BASE_SORT_COMPARE)SmBaseHobCompare, &SortBuffer); QuickSort (SmBaseHobs, HobCount, sizeof (SMM_BASE_HOB_DATA *), (BASE_SORT_COMPARE)SmBaseHobCompare, &SortBuffer);
@@ -714,7 +720,8 @@ GetSmBase (
} }
FreePool (SmBaseHobs); FreePool (SmBaseHobs);
return SmBaseBuffer; *AllocatedSmBaseBuffer = SmBaseBuffer;
return EFI_SUCCESS;
} }
/** /**
@@ -1111,8 +1118,15 @@ PiCpuSmmEntry (
// Retrive the allocated SmmBase from gSmmBaseHobGuid. If found, // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found,
// means the SmBase relocation has been done. // means the SmBase relocation has been done.
// //
mCpuHotPlugData.SmBase = GetSmBase (mMaxNumberOfCpus); mCpuHotPlugData.SmBase = NULL;
if (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. // Check whether the Required TileSize is enough.
// //
@@ -1126,6 +1140,8 @@ PiCpuSmmEntry (
mSmmRelocated = TRUE; mSmmRelocated = TRUE;
} else { } else {
ASSERT (Status == EFI_NOT_FOUND);
ASSERT (mCpuHotPlugData.SmBase == NULL);
// //
// When the HOB doesn't exist, allocate new SMBASE itself. // When the HOB doesn't exist, allocate new SMBASE itself.
// //