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

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

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.

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

Thanks,
Tom

> 
> Thanks
> Laszlo
>