Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/18/21 12:17 PM, Laszlo Ersek wrote:
> On 05/17/21 17:03, Lendacky, Thomas wrote:
>> On 5/16/21 11:22 PM, Laszlo Ersek wrote:
>>> On 05/14/21 22:28, Lendacky, Thomas wrote:
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503778789%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=194iu8I6ucUBr6bNR0fIBa%2FgQhtUQQpJdN55gzOHlmY%3D&reserved=0
>>>>
>>>> The SEV-ES stacks currently share a page with the reset code and data.
>>>> Separate the SEV-ES stacks from the reset vector code and data to avoid
>>>> possible stack overflows from overwriting the code and/or data.
>>>>
>>>> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
>>>> to allocate a new area, below the reset vector and data.
>>>>
>>>> Both the PEI and DXE versions of GetWakeupBuffer() are changed so that
>>>> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
>>>> allocation in order to ensure that the new buffer allocation is below the
>>>> previous allocation. When PcdSevEsIsEnabled is false, the original logic
>>>> is followed.
>>>>
>>>> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
>>>
>>> Is this really a *bugfix*?
>>>
>>> I called what's being fixed "a gap in a generic protection mechanism",
>>> in <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324%23c9&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503778789%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=QFwYOCIEZb8Uc6zX60a7p6QWIkPdzTInggQQv3dRVlQ%3D&amp;reserved=0>.
>>>
>>> I don't know if that makes this patch a feature addition (or feature
>>> completion -- the feature being "more extensive page protections"), or a
>>> bugfix.
>>>
>>> The distinction matters because of the soft feature freeze:
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503788787%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=bZJ%2BK1Y%2Fs4CXXy%2Fz3IKVnWWCUSz3Noo3cPsWR%2Fby5H0%3D&amp;reserved=0
>>>
>>> We still have approximately 2 hours before the SFF starts. So if we can
>>> *finish* reviewing this in 2 hours, then it can be merged during the
>>> SFF, even if we call it a feature. But I'd like Marvin to take a look as
>>> well, plus I'd like at least one of Eric and Ray to check.
>>>
>>> ... I'm tempted not to call it a bugfix, because the lack of this patch
>>> does not break SEV-ES usage, as far as I understand.
>>>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
>>>>   libraries and to make it obvious they are SEV-ES specific.
>>>> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
>>>>   so that the new support is only use for SEV-ES guests.
>>>> ---
>>>>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
>>>>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 49 +++++++++++++-------
>>>>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
>>>>  3 files changed, 69 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> index 7839c249760e..93fc63bf93e3 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> @@ -29,6 +29,11 @@ VOID             *mReservedApLoopFunc = NULL;
>>>>  UINTN            mReservedTopOfApStack;
>>>>  volatile UINT32  mNumberToFinish = 0;
>>>>  
>>>> +//
>>>> +// Begin wakeup buffer allocation below 0x88000
>>>> +//
>>>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
>>>> +
>>>>  /**
>>>>    Enable Debug Agent to support source debugging on AP function.
>>>>  
>>>> @@ -102,7 +107,14 @@ GetWakeupBuffer (
>>>>    // LagacyBios driver depends on CPU Arch protocol which guarantees below
>>>>    // allocation runs earlier than LegacyBios driver.
>>>>    //
>>>> -  StartAddress = 0x88000;
>>>> +  if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> +    //
>>>> +    // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
>>>> +    //
>>>> +    StartAddress = mSevEsDxeWakeupBuffer;
>>>> +  } else {
>>>> +    StartAddress = 0x88000;
>>>> +  }
>>>>    Status = gBS->AllocatePages (
>>>>                    AllocateMaxAddress,
>>>>                    MemoryType,
>>>> @@ -112,6 +124,11 @@ GetWakeupBuffer (
>>>>    ASSERT_EFI_ERROR (Status);
>>>>    if (EFI_ERROR (Status)) {
>>>>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>>>> +  } else if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> +    //
>>>> +    // Next SEV-ES wakeup buffer allocation must be below this allocation
>>>> +    //
>>>> +    mSevEsDxeWakeupBuffer = StartAddress;
>>>>    }
>>>>  
>>>>    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> index dc2a54aa31e8..b9a06747edbf 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
>>>>             AddressMap->SwitchToRealSize +
>>>>             sizeof (MP_CPU_EXCHANGE_INFO);
>>>>  
>>>> -  //
>>>> -  // The AP reset stack is only used by SEV-ES guests. Do not add to the
>>>> -  // allocation if SEV-ES is not enabled.
>>>> -  //
>>>> -  if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> -    //
>>>> -    // Stack location is based on APIC ID, so use the total number of
>>>> -    // processors for calculating the total stack area.
>>>> -    //
>>>> -    Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>>>> -
>>>> -    Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
>>>> -  }
>>>> -
>>>>    return Size;
>>>>  }
>>>>  
>>>> @@ -1192,6 +1178,7 @@ AllocateResetVector (
>>>>    )
>>>>  {
>>>>    UINTN           ApResetVectorSize;
>>>> +  UINTN           ApResetStackSize;
>>>>  
>>>>    if (CpuMpData->WakeupBuffer == (UINTN) -1) {
>>>>      ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
>>>> @@ -1207,9 +1194,39 @@ AllocateResetVector (
>>>>                                      CpuMpData->AddressMap.ModeTransitionOffset
>>>>                                      );
>>>>      //
>>>> -    // The reset stack starts at the end of the buffer.
>>>> +    // The AP reset stack is only used by SEV-ES guests. Do not allocate it
>>>> +    // if SEV-ES is not enabled.
>>>>      //
>>>> -    CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
>>>> +    if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> +      //
>>>> +      // Stack location is based on ProcessorNumber, so use the total number
>>>
>>> sneaky documenation fix :) I vaguely remember discussing earlier that
>>> the APIC ID reference was incorrect. OK.
>>
>> Yeah, I should have made mention of that in the commit message.
>>
>>>
>>>> +      // of processors for calculating the total stack area.
>>>> +      //
>>>> +      ApResetStackSize = (AP_RESET_STACK_SIZE *
>>>> +                          PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>>>> +
>>>> +      //
>>>> +      // Invoke GetWakeupBuffer a second time to allocate the stack area
>>>> +      // below 1MB. The returned buffer will be page aligned and sized and
>>>> +      // below the previously allocated buffer.
>>>> +      //
>>>> +      CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
>>>> +
>>>> +      //
>>>> +      // Check to be sure that the "allocate below" behavior hasn't changed.
>>>> +      // This will also catch a failed allocation, as "-1" is returned on
>>>> +      // failure.
>>>> +      //
>>>> +      if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
>>>> +        DEBUG ((
>>>> +          DEBUG_ERROR,
>>>> +          "SEV-ES AP reset stack is not below wakeup buffer\n"
>>>> +          ));
>>>> +
>>>> +        ASSERT (FALSE);
>>>> +        CpuDeadLoop ();
>>>> +      }
>>>> +    }
>>>>    }
>>>>    BackupAndPrepareWakeupBuffer (CpuMpData);
>>>>  }
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> index 3989bd6a7a9f..90015c650c68 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> @@ -11,6 +11,8 @@
>>>>  #include <Guid/S3SmmInitDone.h>
>>>>  #include <Ppi/ShadowMicrocode.h>
>>>>  
>>>> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
>>>> +
>>>>  /**
>>>>    S3 SMM Init Done notification function.
>>>>  
>>>> @@ -220,7 +222,13 @@ GetWakeupBuffer (
>>>>          // Need memory under 1MB to be collected here
>>>>          //
>>>>          WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
>>>> -        if (WakeupBufferEnd > BASE_1MB) {
>>>> +        if (PcdGetBool (PcdSevEsIsEnabled) &&
>>>> +            WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
>>>> +          //
>>>> +          // SEV-ES Wakeup buffer should be under 1MB and under any previous one
>>>> +          //
>>>> +          WakeupBufferEnd = mSevEsPeiWakeupBuffer;
>>>> +        } else if (WakeupBufferEnd > BASE_1MB) {
>>>>            //
>>>>            // Wakeup buffer should be under 1MB
>>>>            //
>>>> @@ -244,6 +252,15 @@ GetWakeupBuffer (
>>>>            }
>>>>            DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>>                                 WakeupBufferStart, WakeupBufferSize));
>>>> +
>>>> +          if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> +            //
>>>> +            // Next SEV-ES wakeup buffer allocation must be below this
>>>> +            // allocation
>>>> +            //
>>>> +            mSevEsPeiWakeupBuffer = WakeupBufferStart;
>>>> +          }
>>>> +
>>>>            return (UINTN)WakeupBufferStart;
>>>>          }
>>>>        }
>>>>
>>>
>>> The code in the patch seems sound to me, but, now that I've managed to
>>> take a bit more careful look, I think the patch is incomplete.
>>>
>>> Note the BackupAndPrepareWakeupBuffer() function call -- visible in the
>>> context --, at the end of the AllocateResetVector() function! Before, we
>>> had a single area allocated for the reset vector, which was
>>> appropriately sized for SEV-ES stacks too, in case SEV-ES was enabled.
>>>
>>> That was because MpInitLibInitialize() called GetApResetVectorSize()
>>> too, and stored the result to the "BackupBufferSize" field. Thus, the
>>> BackupAndPrepareWakeupBuffer() function, and its counterpart
>>> RestoreWakeupBuffer(), would include the SEV-ES AP stacks area in the
>>> backup/restore operations.
>>
>> The restore is not performed for an SEV-ES guest (see FreeResetVector()),
>> because the memory is still needed.
> 
> I apologize for missing this. I'm not too familiar with the SEV-ES
> specifics in UefiCpuPkg.
> 
> One question: given that FreeResetVector() does not call
> RestoreWakeupBuffer() when SEV-ES is enabled, would it make sense for
> AllocateResetVector() to not call BackupAndPrepareWakeupBuffer() either,
> in case SEV-ES is enabled? Because, if we never restore, do we really
> need the backup? I wonder if such a patch could be prepended to this one
> (in order to form a 2-patch series).
> 
> (Well, BackupAndPrepareWakeupBuffer() performs two things, backup and
> preparation -- I guess we certainly need the preparation of the wake up
> buffer, but do we need to back up the original contents of the area?
> Including the backup buffer allocation.)

We don't need to, but it doesn't hurt. I wanted to avoid sprinkling too
many SEV-ES specific checks throughout the code.

> 
>>
>>>
>>> But now, with SEV-ES enabled, we'll have a separate, discontiguous area
>>> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart
>>> RestoreWakeupBuffer() take that into account.
>>>
>>> Therefore I think, while this patch is regression-free for the SEV-ES
>>> *disabled* case, it may corrupt memory (through not restoring the AP
>>> stack area's original contents) with SEV-ES enabled.
>>
>> This is the current behavior for SEV-ES. The wakeup buffer memory is
>> marked as reserved, at least in the DXE phase.
>>
>>>
>>> I think we need to turn "ApResetStackSize" into an explicit field. It
>>> should have a defined value only when SEV-ES is enabled. And for that
>>> case, we seem to need a *separate backup buffer* too.
>>>
>>> FWIW, I'd really like to re-classify this BZ as a Feature Request (see
>>> the Product field on BZ#3324), and I'd really like to delay the patch
>>> until after edk2-stable202105. The patch is not necessary for using
>>> SEV-ES, but it has a chance to break SEV-ES.
>>
>> I'm ok with delaying this if you want.
> 
> Here's what I'd like to do:
> 
> - Reach an agreement with Marvin about the ASSERT(). I'm fine if we drop
> it, and fine if we keep it.

I can drop it since there's already a debug message issued at that point.

> 
> - Eric or Ray to check the patch as well, because (as I said above) I
> didn't follow the SEV-ES patches for UefiCpuPkg (that series was just
> huge, apologies), and so now I don't have memories to reach back to.
> 
> - Figure out if we need to conditionalize the
> BackupAndPrepareWakeupBuffer() call (or a part of that function anyway).
> 
> We can and should continue discussing these things during the feature
> freeze; I'd like us to merge the patch after the tag.
> 
> Sorry again that I'm missing bits and pieces; I'm this close |<->| to
> email bankruptcy.

I know that feeling, stay solvent!

Thanks,
Tom

> 
> Thanks
> Laszlo
>