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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BiE2cBZRVRT3anwklHEKBHdhg8v2KjV%2FiiPwtx%2Fpon4%3D&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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns9s%3D&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.d...@intel.com>
>> Cc: Ray Ni <ray...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Cc: Rahul Kumar <rahul1.ku...@intel.com>
>> Cc: Marvin Häuser <mhaeu...@posteo.de>
>> Signed-off-by: Tom Lendacky <thomas.lenda...@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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75211): https://edk2.groups.io/g/devel/message/75211
Mute This Topic: https://groups.io/mt/82833645/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
