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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-