From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web09.14695.1621225352527419444 for ; Sun, 16 May 2021 21:22:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Q7BOym7c; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621225351; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ven0d5TN/jgCLnPT5XLSR08FVkmIz0aEAvAZ6IO8Uo8=; b=Q7BOym7cozDGA3DyfKIha+/i5ysha9WUohxj4YtJ8O0loj7AtA9ISZG3qx7n1++MQykPRY /BI/KR0GXrNZ1D/xESZQ3nrBnqYCikBUSnm4YBHDtQYK+lcIK4day2iF5qqPWARCqnmyty BWzihMfPiA1z4deskRviHgCHn+Yk1HY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-290-Uk6_Gh71Nl-PmF_z4kikCQ-1; Mon, 17 May 2021 00:22:28 -0400 X-MC-Unique: Uk6_Gh71Nl-PmF_z4kikCQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 358041009460; Mon, 17 May 2021 04:22:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-22.ams2.redhat.com [10.36.112.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 80C855D9F0; Mon, 17 May 2021 04:22:24 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , Eric Dong , Ray Ni , Rahul Kumar , =?UTF-8?Q?Marvin_H=c3=a4user?= References: <3cae2ac836884b131725866264e0a0e1897052de.1621024125.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <5394e010-6088-18e8-9a90-65eb55bbfac2@redhat.com> Date: Mon, 17 May 2021 06:22:23 +0200 MIME-Version: 1.0 In-Reply-To: <3cae2ac836884b131725866264e0a0e1897052de.1621024125.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 . 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 > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Marvin Häuser > Signed-off-by: Tom Lendacky > > --- > > 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 > #include > > +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