* [edk2-devel] [edk2-stable202402 PATCH 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing
@ 2024-02-13 21:09 Laszlo Ersek
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Laszlo Ersek @ 2024-02-13 21:09 UTC (permalink / raw)
To: devel
Cc: Dun Tan, Gerd Hoffmann, Liming Gao, Michael D Kinney, Rahul Kumar,
Ray Ni
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4682
Personal CI run (in progress):
https://github.com/tianocore/edk2/pull/5370
Commit 725acd0b9cc0 ("UefiCpuPkg: Avoid assuming only one smmbasehob",
2023-12-12) introduced a NULL pointer dereference to PiSmmCpuDxeSmm on
such platforms that do not produce the "gSmmBaseHobGuid" GUID HOB at
all.
Please see the multi-step analysis in the following thread:
[edk2-devel] [PATCH 1/1] OvmfPkg/QemuVideoDxe: purge VbeShim
https://edk2.groups.io/g/devel/message/115377
message-id: <20240213085925.687848-1-kraxel@redhat.com>
This issue needs to be fixed for edk2-stable202402.
Cc: Dun Tan <dun.tan@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Best regards,
Laszlo
Laszlo Ersek (2):
UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is
missing
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 47 +++++++++++++++-----
1 file changed, 35 insertions(+), 12 deletions(-)
base-commit: 8801c75b4d77c2e6e06b3ddc8560e0db590f6342
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115416): https://edk2.groups.io/g/devel/message/115416
Mute This Topic: https://groups.io/mt/104341340/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-13 21:09 [edk2-devel] [edk2-stable202402 PATCH 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing Laszlo Ersek
@ 2024-02-13 21:09 ` Laszlo Ersek
2024-02-13 21:35 ` Laszlo Ersek
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing Laszlo Ersek
2024-02-14 9:01 ` [edk2-devel] [edk2-stable202402 PATCH 0/2] " Gerd Hoffmann
2 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2024-02-13 21:09 UTC (permalink / raw)
To: devel; +Cc: Dun Tan, Gerd Hoffmann, Rahul Kumar, Ray Ni
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [edk2-devel] [edk2-stable202402 PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing
2024-02-13 21:09 [edk2-devel] [edk2-stable202402 PATCH 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing Laszlo Ersek
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes Laszlo Ersek
@ 2024-02-13 21:09 ` Laszlo Ersek
2024-02-14 9:01 ` [edk2-devel] [edk2-stable202402 PATCH 0/2] " Gerd Hoffmann
2 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2024-02-13 21:09 UTC (permalink / raw)
To: devel; +Cc: Dun Tan, Gerd Hoffmann, Rahul Kumar, Ray Ni
Before commit 725acd0b9cc0 ("UefiCpuPkg: Avoid assuming only one
smmbasehob", 2023-12-12), PiCpuSmmEntry() used to look up
"gSmmBaseHobGuid", and allocate "mCpuHotPlugData.SmBase" regardless of the
GUID's presence:
> - mCpuHotPlugData.SmBase = (UINTN *)AllocatePool (sizeof (UINTN) * mMaxNumberOfCpus);
> - ASSERT (mCpuHotPlugData.SmBase != NULL);
After commit 725acd0b9cc0, PiCpuSmmEntry() -> GetSmBase() would allocate
"mCpuHotPlugData.SmBase" only on the success path, and no allocation would
be performed on *any* of the error paths.
This caused a problem: if "mCpuHotPlugData.SmBase" was left NULL because
the GUID HOB was missing, PiCpuSmmEntry() would still be supposed to
allocate "mCpuHotPlugData.SmBase", just like earlier. However, because
commit 725acd0b9cc0 conflated the two possible error modes (out of SMRAM,
and no GUID HOB), PiCpuSmmEntry() could not decide whether it should
allocate "mCpuHotPlugData.SmBase", or not. Currently, we never allocate if
GetSmBase() fails -- for any reason --, which means that on platforms that
don't produce the GUID HOB, "mCpuHotPlugData.SmBase" is left NULL, leading
to null pointer dereferences later, in PiCpuSmmEntry().
Now that a prior patch in the series distinguishes the two error modes
from each other, we can tell exactly when the GUID HOB is not found, and
reinstate the earlier "mCpuHotPlugData.SmBase" allocation for that case.
(With an actual error check thrown in, in addition to the original
"assertion".)
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
Fixes: 725acd0b9cc0
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
context:-U7
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 09382945ddb4..499f979d34e2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -1142,14 +1142,21 @@ PiCpuSmmEntry (
} 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"));
+
+ mCpuHotPlugData.SmBase = (UINTN *)AllocatePool (sizeof (UINTN) * mMaxNumberOfCpus);
+ if (mCpuHotPlugData.SmBase == NULL) {
+ ASSERT (mCpuHotPlugData.SmBase != NULL);
+ CpuDeadLoop ();
+ }
+
//
// very old processors (i486 + pentium) need 32k not 4k alignment, exclude them.
//
ASSERT (FamilyId >= 6);
//
// Allocate buffer for all of the tiles.
//
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115418): https://edk2.groups.io/g/devel/message/115418
Mute This Topic: https://groups.io/mt/104341426/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes Laszlo Ersek
@ 2024-02-13 21:35 ` Laszlo Ersek
2024-02-14 3:43 ` Michael D Kinney
0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2024-02-13 21:35 UTC (permalink / raw)
To: devel; +Cc: Dun Tan, Gerd Hoffmann, Rahul Kumar, Ray Ni
On 2/13/24 22:09, Laszlo Ersek wrote:
> 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.
Sorry, these two paragraphs are not precise. A better version:
----------
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 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 725acd0b9cc0, "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().
----------
Sorry, it's late.
If this patch set is accepted otherwise, then Mike or Liming, can you
please update the first two paragraphs of the commit message upon merge?
Thanks
Laszlo
>
> 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 (#115419): https://edk2.groups.io/g/devel/message/115419
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-13 21:35 ` Laszlo Ersek
@ 2024-02-14 3:43 ` Michael D Kinney
2024-02-14 11:22 ` Laszlo Ersek
2024-02-14 13:08 ` Leif Lindholm
0 siblings, 2 replies; 12+ messages in thread
From: Michael D Kinney @ 2024-02-14 3:43 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Leif Lindholm,
Andrew Fish (afish@apple.com), Gao, Liming
Cc: Tan, Dun, Gerd Hoffmann, Kumar, Rahul R, Ni, Ray,
Kinney, Michael D
Hi Laszlo,
Thank you for the quick fix.
I have reviewed the changes. I agree they fix the issue at hand.
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
I have adjusted the commit message with your suggested changes in
the PR I have prepared:
https://github.com/tianocore/edk2/pull/5373
There may be better ways to organize this code in general to make
it easier to understand and maintain in the future, but we can
let Ray review that when he returns. That will also likely be a
much bugger change that can be accepted just before a release.
I also approve this as a critical fix for edk2-stable202402
I will wait till tomorrow morning my time to see if Gerd and
Rahul and Leif can also provide their reviews/approvals and
to give me some time to run some tests.
I do not expect Ray Ni or Dun Tan to be available this week.
Best regards,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, February 13, 2024 1:36 PM
> To: devel@edk2.groups.io
> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>
> On 2/13/24 22:09, Laszlo Ersek wrote:
> > 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.
>
> Sorry, these two paragraphs are not precise. A better version:
>
> ----------
> 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 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 725acd0b9cc0, "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().
> ----------
>
> Sorry, it's late.
>
> If this patch set is accepted otherwise, then Mike or Liming, can you
> please update the first two paragraphs of the commit message upon
> merge?
>
> Thanks
> Laszlo
>
> >
> > 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 (#115427): https://edk2.groups.io/g/devel/message/115427
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing
2024-02-13 21:09 [edk2-devel] [edk2-stable202402 PATCH 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing Laszlo Ersek
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes Laszlo Ersek
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing Laszlo Ersek
@ 2024-02-14 9:01 ` Gerd Hoffmann
2024-02-14 9:40 ` rahul.r.kumar
2 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2024-02-14 9:01 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, Dun Tan, Liming Gao, Michael D Kinney, Rahul Kumar, Ray Ni
On Tue, Feb 13, 2024 at 10:09:16PM +0100, Laszlo Ersek wrote:
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4682
>
> Personal CI run (in progress):
> https://github.com/tianocore/edk2/pull/5370
>
> Commit 725acd0b9cc0 ("UefiCpuPkg: Avoid assuming only one smmbasehob",
> 2023-12-12) introduced a NULL pointer dereference to PiSmmCpuDxeSmm on
> such platforms that do not produce the "gSmmBaseHobGuid" GUID HOB at
> all.
>
> Please see the multi-step analysis in the following thread:
>
> [edk2-devel] [PATCH 1/1] OvmfPkg/QemuVideoDxe: purge VbeShim
> https://edk2.groups.io/g/devel/message/115377
> message-id: <20240213085925.687848-1-kraxel@redhat.com>
>
> This issue needs to be fixed for edk2-stable202402.
>
> Cc: Dun Tan <dun.tan@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
Series:
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115433): https://edk2.groups.io/g/devel/message/115433
Mute This Topic: https://groups.io/mt/104341340/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing
2024-02-14 9:01 ` [edk2-devel] [edk2-stable202402 PATCH 0/2] " Gerd Hoffmann
@ 2024-02-14 9:40 ` rahul.r.kumar
0 siblings, 0 replies; 12+ messages in thread
From: rahul.r.kumar @ 2024-02-14 9:40 UTC (permalink / raw)
To: Gerd Hoffmann, devel
[-- Attachment #1: Type: text/plain, Size: 376 bytes --]
Approved
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115435): https://edk2.groups.io/g/devel/message/115435
Mute This Topic: https://groups.io/mt/104341340/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 794 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-14 3:43 ` Michael D Kinney
@ 2024-02-14 11:22 ` Laszlo Ersek
2024-02-14 13:08 ` Leif Lindholm
1 sibling, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2024-02-14 11:22 UTC (permalink / raw)
To: devel, michael.d.kinney, Leif Lindholm,
Andrew Fish (afish@apple.com), Gao, Liming
Cc: Tan, Dun, Gerd Hoffmann, Kumar, Rahul R, Ni, Ray
On 2/14/24 04:43, Michael D Kinney wrote:
> Hi Laszlo,
>
> Thank you for the quick fix.
>
> I have reviewed the changes. I agree they fix the issue at hand.
>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>
> I have adjusted the commit message with your suggested changes in
> the PR I have prepared:
>
> https://github.com/tianocore/edk2/pull/5373
>
> There may be better ways to organize this code in general to make
> it easier to understand and maintain in the future, but we can
> let Ray review that when he returns. That will also likely be a
> much bugger change that can be accepted just before a release.
>
> I also approve this as a critical fix for edk2-stable202402
>
> I will wait till tomorrow morning my time to see if Gerd and
> Rahul and Leif can also provide their reviews/approvals and
> to give me some time to run some tests.
>
> I do not expect Ray Ni or Dun Tan to be available this week.
Thank you all very much for the feedback!
Laszlo
>
> Best regards,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>> Sent: Tuesday, February 13, 2024 1:36 PM
>> To: devel@edk2.groups.io
>> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
>> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
>> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>>
>> On 2/13/24 22:09, Laszlo Ersek wrote:
>>> 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.
>>
>> Sorry, these two paragraphs are not precise. A better version:
>>
>> ----------
>> 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 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 725acd0b9cc0, "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().
>> ----------
>>
>> Sorry, it's late.
>>
>> If this patch set is accepted otherwise, then Mike or Liming, can you
>> please update the first two paragraphs of the commit message upon
>> merge?
>>
>> Thanks
>> Laszlo
>>
>>>
>>> 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 (#115444): https://edk2.groups.io/g/devel/message/115444
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-14 3:43 ` Michael D Kinney
2024-02-14 11:22 ` Laszlo Ersek
@ 2024-02-14 13:08 ` Leif Lindholm
2024-02-14 17:26 ` Michael D Kinney
1 sibling, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2024-02-14 13:08 UTC (permalink / raw)
To: devel, michael.d.kinney, lersek@redhat.com, Leif Lindholm,
Andrew Fish (afish@apple.com), Gao, Liming
Cc: Tan, Dun, Gerd Hoffmann, Kumar, Rahul R, Ni, Ray
On 2024-02-14 03:43, Michael D Kinney wrote:
> Hi Laszlo,
>
> Thank you for the quick fix.
>
> I have reviewed the changes. I agree they fix the issue at hand.
>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>
> I have adjusted the commit message with your suggested changes in
> the PR I have prepared:
>
> https://github.com/tianocore/edk2/pull/5373
>
> There may be better ways to organize this code in general to make
> it easier to understand and maintain in the future, but we can
> let Ray review that when he returns. That will also likely be a
> much bugger change that can be accepted just before a release.
>
> I also approve this as a critical fix for edk2-stable202402
>
> I will wait till tomorrow morning my time to see if Gerd and
> Rahul and Leif can also provide their reviews/approvals and
> to give me some time to run some tests.
For the series:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
I'm happy for this to go into the stable tag.
/
Leif
> I do not expect Ray Ni or Dun Tan to be available this week.
>
> Best regards,
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>> Sent: Tuesday, February 13, 2024 1:36 PM
>> To: devel@edk2.groups.io
>> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
>> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
>> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>>
>> On 2/13/24 22:09, Laszlo Ersek wrote:
>>> 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.
>>
>> Sorry, these two paragraphs are not precise. A better version:
>>
>> ----------
>> 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 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 725acd0b9cc0, "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().
>> ----------
>>
>> Sorry, it's late.
>>
>> If this patch set is accepted otherwise, then Mike or Liming, can you
>> please update the first two paragraphs of the commit message upon
>> merge?
>>
>> Thanks
>> Laszlo
>>
>>>
>>> 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 (#115453): https://edk2.groups.io/g/devel/message/115453
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-14 13:08 ` Leif Lindholm
@ 2024-02-14 17:26 ` Michael D Kinney
2024-02-15 8:44 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Michael D Kinney @ 2024-02-14 17:26 UTC (permalink / raw)
To: Leif Lindholm, devel@edk2.groups.io, lersek@redhat.com,
Leif Lindholm, Andrew Fish (afish@apple.com), Gao, Liming
Cc: Tan, Dun, Gerd Hoffmann, Kumar, Rahul R, Ni, Ray,
Kinney, Michael D
Merged: https://github.com/tianocore/edk2/pull/5373
> -----Original Message-----
> From: Leif Lindholm <quic_llindhol@quicinc.com>
> Sent: Wednesday, February 14, 2024 5:08 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; lersek@redhat.com; Leif Lindholm
> <llindhol@qti.qualcomm.com>; Andrew Fish (afish@apple.com)
> <afish@apple.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>
> On 2024-02-14 03:43, Michael D Kinney wrote:
> > Hi Laszlo,
> >
> > Thank you for the quick fix.
> >
> > I have reviewed the changes. I agree they fix the issue at hand.
> >
> > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > I have adjusted the commit message with your suggested changes in
> > the PR I have prepared:
> >
> > https://github.com/tianocore/edk2/pull/5373
> >
> > There may be better ways to organize this code in general to make
> > it easier to understand and maintain in the future, but we can
> > let Ray review that when he returns. That will also likely be a
> > much bugger change that can be accepted just before a release.
> >
> > I also approve this as a critical fix for edk2-stable202402
> >
> > I will wait till tomorrow morning my time to see if Gerd and
> > Rahul and Leif can also provide their reviews/approvals and
> > to give me some time to run some tests.
>
> For the series:
> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
> I'm happy for this to go into the stable tag.
>
> /
> Leif
>
> > I do not expect Ray Ni or Dun Tan to be available this week.
> >
> > Best regards,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Laszlo
> >> Ersek
> >> Sent: Tuesday, February 13, 2024 1:36 PM
> >> To: devel@edk2.groups.io
> >> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> >> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
> >> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
> >> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
> >>
> >> On 2/13/24 22:09, Laszlo Ersek wrote:
> >>> 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.
> >>
> >> Sorry, these two paragraphs are not precise. A better version:
> >>
> >> ----------
> >> 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 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 725acd0b9cc0, "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().
> >> ----------
> >>
> >> Sorry, it's late.
> >>
> >> If this patch set is accepted otherwise, then Mike or Liming, can
> you
> >> please update the first two paragraphs of the commit message upon
> >> merge?
> >>
> >> Thanks
> >> Laszlo
> >>
> >>>
> >>> 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 (#115469): https://edk2.groups.io/g/devel/message/115469
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-14 17:26 ` Michael D Kinney
@ 2024-02-15 8:44 ` Laszlo Ersek
2024-02-19 9:12 ` duntan
0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2024-02-15 8:44 UTC (permalink / raw)
To: Kinney, Michael D, Leif Lindholm, devel@edk2.groups.io,
Leif Lindholm, Andrew Fish (afish@apple.com), Gao, Liming
Cc: Tan, Dun, Gerd Hoffmann, Kumar, Rahul R, Ni, Ray
On 2/14/24 18:26, Kinney, Michael D wrote:
> Merged: https://github.com/tianocore/edk2/pull/5373
Thanks!
Laszlo
>
>> -----Original Message-----
>> From: Leif Lindholm <quic_llindhol@quicinc.com>
>> Sent: Wednesday, February 14, 2024 5:08 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>; lersek@redhat.com; Leif Lindholm
>> <llindhol@qti.qualcomm.com>; Andrew Fish (afish@apple.com)
>> <afish@apple.com>; Gao, Liming <gaoliming@byosoft.com.cn>
>> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
>> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
>> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>>
>> On 2024-02-14 03:43, Michael D Kinney wrote:
>>> Hi Laszlo,
>>>
>>> Thank you for the quick fix.
>>>
>>> I have reviewed the changes. I agree they fix the issue at hand.
>>>
>>> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>>>
>>> I have adjusted the commit message with your suggested changes in
>>> the PR I have prepared:
>>>
>>> https://github.com/tianocore/edk2/pull/5373
>>>
>>> There may be better ways to organize this code in general to make
>>> it easier to understand and maintain in the future, but we can
>>> let Ray review that when he returns. That will also likely be a
>>> much bugger change that can be accepted just before a release.
>>>
>>> I also approve this as a critical fix for edk2-stable202402
>>>
>>> I will wait till tomorrow morning my time to see if Gerd and
>>> Rahul and Leif can also provide their reviews/approvals and
>>> to give me some time to run some tests.
>>
>> For the series:
>> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
>> I'm happy for this to go into the stable tag.
>>
>> /
>> Leif
>>
>>> I do not expect Ray Ni or Dun Tan to be available this week.
>>>
>>> Best regards,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>> Laszlo
>>>> Ersek
>>>> Sent: Tuesday, February 13, 2024 1:36 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
>>>> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
>>>> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
>>>> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>>>>
>>>> On 2/13/24 22:09, Laszlo Ersek wrote:
>>>>> 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.
>>>>
>>>> Sorry, these two paragraphs are not precise. A better version:
>>>>
>>>> ----------
>>>> 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 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 725acd0b9cc0, "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().
>>>> ----------
>>>>
>>>> Sorry, it's late.
>>>>
>>>> If this patch set is accepted otherwise, then Mike or Liming, can
>> you
>>>> please update the first two paragraphs of the commit message upon
>>>> merge?
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>>
>>>>> 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 (#115493): https://edk2.groups.io/g/devel/message/115493
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
2024-02-15 8:44 ` Laszlo Ersek
@ 2024-02-19 9:12 ` duntan
0 siblings, 0 replies; 12+ messages in thread
From: duntan @ 2024-02-19 9:12 UTC (permalink / raw)
To: Laszlo Ersek, Kinney, Michael D, Leif Lindholm,
devel@edk2.groups.io, Leif Lindholm,
Andrew Fish (afish@apple.com), Gao, Liming
Cc: Gerd Hoffmann, Kumar, Rahul R, Ni, Ray
Laszlo,
Sorry for the late reply. Thanks for your code refactoring patch and bugfix patch!
Thanks,
Dun
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Thursday, February 15, 2024 4:45 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; devel@edk2.groups.io; Leif Lindholm <llindhol@qti.qualcomm.com>; Andrew Fish (afish@apple.com) <afish@apple.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
On 2/14/24 18:26, Kinney, Michael D wrote:
> Merged: https://github.com/tianocore/edk2/pull/5373
Thanks!
Laszlo
>
>> -----Original Message-----
>> From: Leif Lindholm <quic_llindhol@quicinc.com>
>> Sent: Wednesday, February 14, 2024 5:08 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>; lersek@redhat.com; Leif Lindholm
>> <llindhol@qti.qualcomm.com>; Andrew Fish (afish@apple.com)
>> <afish@apple.com>; Gao, Liming <gaoliming@byosoft.com.cn>
>> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
>> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
>> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>>
>> On 2024-02-14 03:43, Michael D Kinney wrote:
>>> Hi Laszlo,
>>>
>>> Thank you for the quick fix.
>>>
>>> I have reviewed the changes. I agree they fix the issue at hand.
>>>
>>> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>>>
>>> I have adjusted the commit message with your suggested changes in
>>> the PR I have prepared:
>>>
>>> https://github.com/tianocore/edk2/pull/5373
>>>
>>> There may be better ways to organize this code in general to make it
>>> easier to understand and maintain in the future, but we can let Ray
>>> review that when he returns. That will also likely be a much bugger
>>> change that can be accepted just before a release.
>>>
>>> I also approve this as a critical fix for edk2-stable202402
>>>
>>> I will wait till tomorrow morning my time to see if Gerd and Rahul
>>> and Leif can also provide their reviews/approvals and to give me
>>> some time to run some tests.
>>
>> For the series:
>> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com> I'm happy for
>> this to go into the stable tag.
>>
>> /
>> Leif
>>
>>> I do not expect Ray Ni or Dun Tan to be available this week.
>>>
>>> Best regards,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>> Laszlo
>>>> Ersek
>>>> Sent: Tuesday, February 13, 2024 1:36 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Tan, Dun <dun.tan@intel.com>; Gerd Hoffmann
>>>> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni,
>>>> Ray <ray.ni@intel.com>
>>>> Subject: Re: [edk2-devel] [edk2-stable202402 PATCH 1/2]
>>>> UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes
>>>>
>>>> On 2/13/24 22:09, Laszlo Ersek wrote:
>>>>> 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.
>>>>
>>>> Sorry, these two paragraphs are not precise. A better version:
>>>>
>>>> ----------
>>>> 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 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 725acd0b9cc0, "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().
>>>> ----------
>>>>
>>>> Sorry, it's late.
>>>>
>>>> If this patch set is accepted otherwise, then Mike or Liming, can
>> you
>>>> please update the first two paragraphs of the commit message upon
>>>> merge?
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>>
>>>>> 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 (#115586): https://edk2.groups.io/g/devel/message/115586
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-02-19 9:12 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-13 21:09 [edk2-devel] [edk2-stable202402 PATCH 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing Laszlo Ersek
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: distinguish GetSmBase() failure modes Laszlo Ersek
2024-02-13 21:35 ` Laszlo Ersek
2024-02-14 3:43 ` Michael D Kinney
2024-02-14 11:22 ` Laszlo Ersek
2024-02-14 13:08 ` Leif Lindholm
2024-02-14 17:26 ` Michael D Kinney
2024-02-15 8:44 ` Laszlo Ersek
2024-02-19 9:12 ` duntan
2024-02-13 21:09 ` [edk2-devel] [edk2-stable202402 PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is missing Laszlo Ersek
2024-02-14 9:01 ` [edk2-devel] [edk2-stable202402 PATCH 0/2] " Gerd Hoffmann
2024-02-14 9:40 ` rahul.r.kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox