Skip to content

Commit

Permalink
UefiCpuPkg/PiSmmCpuDxeSmm: fix NULL deref when gSmmBaseHobGuid is mis…
Browse files Browse the repository at this point in the history
…sing

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4682
Fixes: 725acd0

Before commit 725acd0 ("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 725acd0, 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 725acd0 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>
Reported-by: Gerd Hoffmann <kraxel@redhat.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>
  • Loading branch information
lersek authored and mergify[bot] committed Feb 14, 2024
1 parent 72c441d commit edc6681
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
Original file line number Diff line number Diff line change
Expand Up @@ -1146,6 +1146,13 @@ PiCpuSmmEntry (
// 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 ();

This comment has been minimized.

Copy link
@makubacki

makubacki Jun 4, 2024

Member

Shouldn't be done in this PR, but we might want to replace this with PanicLib in our code.

}

//
// very old processors (i486 + pentium) need 32k not 4k alignment, exclude them.
//
Expand Down

0 comments on commit edc6681

Please sign in to comment.