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

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

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%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N9MNXaBazq2tiVRHWSPVXRdlcZ97JOf24mc7p0m5Tqw%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%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=BiE2cBZRVRT3anwklHEKBHdhg8v2KjV%2FiiPwtx%2Fpon4%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%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns9s%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.)

> 
>>
>> 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.

- 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.

Thanks
Laszlo