On 05/14/21 22:28, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324 > > 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://bugzilla.tianocore.org/show_bug.cgi?id=3324#c9>. 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://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning 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. > + // 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. 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. 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. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75169): https://edk2.groups.io/g/devel/message/75169 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] -=-=-=-=-=-=-=-=-=-=-=-