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

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

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