On 18.05.21 19:17, 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%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: 7b7508ad784d16a5208c8d12dff43aef6df0835bIs 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 numbersneaky 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.
As I said, it's a strong recommendation from me, but I am fine either way as well. If it ever causes any of the issues I outlined, it could be fixed later as well.
Best regards, Marvin
- 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
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75280): https://edk2.groups.io/g/devel/message/75280 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] -=-=-=-=-=-=-=-=-=-=-=-
