[edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area
Laszlo Ersek
lersek at redhat.com
Mon May 17 04:22:23 UTC 2021
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.dong at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Rahul Kumar <rahul1.kumar at intel.com>
> Cc: Marvin Häuser <mhaeuser at posteo.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list