From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web12.80.1621359288515622878 for ; Tue, 18 May 2021 10:34:49 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=b0in8RF9; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id E27D82400FF for ; Tue, 18 May 2021 19:34:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1621359285; bh=OTv02MnyHN/OrBWCV9HMw8FD+d3rwhXNID5IbdbW2x4=; h=Subject:To:Cc:From:Date:From; b=b0in8RF9IcAvr/iS7W+c+8YsQn1rUvHof4McgCS6+Q4SLtv6ih+QN7Qga5KLANG8U 8686BpfnOjr+VOw7w64JYHN4KqnDIaUVE2NDGuST0V9lSznrda7pzhVh4rkp23SlHK gy9O4chOB06qj5PTqNh2aQzGG8ApC8SB00IUBCY4TaWeG18rZLxbR6Vj1CjvptzC2k /7Ym5c0+ew5ztFEz8cvVHtH61AYR/tUDBs28tC476ibDClD/CXhwfpAiGsX0v9oF0e /KkqhS+mGDD59yBN78q/sCAWy+JKgzhuLaz/1T1WsX8RcVaqP7obddlCEcq1B1EXBy KM20zGKRkjv6g== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Fl36D3ScNz9rxH; Tue, 18 May 2021 19:34:44 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area To: devel@edk2.groups.io, lersek@redhat.com, thomas.lendacky@amd.com Cc: Brijesh Singh , Eric Dong , Ray Ni , Rahul Kumar References: <3cae2ac836884b131725866264e0a0e1897052de.1621024125.git.thomas.lendacky@amd.com> <5394e010-6088-18e8-9a90-65eb55bbfac2@redhat.com> <7d3d835a-4354-108d-17b7-8679eb8c67d1@amd.com> <61a02049-7280-1a25-c718-c663acfcc56e@redhat.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: Date: Tue, 18 May 2021 17:34:44 +0000 MIME-Version: 1.0 In-Reply-To: <61a02049-7280-1a25-c718-c663acfcc56e@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable 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=3Dhttps%3A%2F= %2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=3D04%7C01%7Ct= homas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884= e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8ey= JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&= ;sdata=3DN9MNXaBazq2tiVRHWSPVXRdlcZ97JOf24mc7p0m5Tqw%3D&reserved=3D0 >>>> >>>> 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 avo= id >>>> 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 tha= t >>>> when PcdSevEsIsEnabled is true, they will track the previous reset bu= ffer >>>> allocation in order to ensure that the new buffer allocation is below= the >>>> previous allocation. When PcdSevEsIsEnabled is false, the original lo= gic >>>> 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://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fgi= thub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning= &data=3D04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d91= 8eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CU= nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC= JXVCI6Mn0%3D%7C1000&sdata=3D1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns= 9s%3D&reserved=3D0 >>> >>> We still have approximately 2 hours before the SFF starts. So if we ca= n >>> *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 patc= h >>> 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=C3=A4user >>>> 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-f= ree >>>> 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/Lib= rary/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 =3D NULL; >>>> UINTN mReservedTopOfApStack; >>>> volatile UINT32 mNumberToFinish =3D 0; >>>> =20 >>>> +// >>>> +// Begin wakeup buffer allocation below 0x88000 >>>> +// >>>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer =3D 0x88000; >>>> + >>>> /** >>>> Enable Debug Agent to support source debugging on AP function. >>>> =20 >>>> @@ -102,7 +107,14 @@ GetWakeupBuffer ( >>>> // LagacyBios driver depends on CPU Arch protocol which guarantee= s below >>>> // allocation runs earlier than LegacyBios driver. >>>> // >>>> - StartAddress =3D 0x88000; >>>> + if (PcdGetBool (PcdSevEsIsEnabled)) { >>>> + // >>>> + // SEV-ES Wakeup buffer should be under 0x88000 and under any pr= evious one >>>> + // >>>> + StartAddress =3D mSevEsDxeWakeupBuffer; >>>> + } else { >>>> + StartAddress =3D 0x88000; >>>> + } >>>> Status =3D gBS->AllocatePages ( >>>> AllocateMaxAddress, >>>> MemoryType, >>>> @@ -112,6 +124,11 @@ GetWakeupBuffer ( >>>> ASSERT_EFI_ERROR (Status); >>>> if (EFI_ERROR (Status)) { >>>> StartAddress =3D (EFI_PHYSICAL_ADDRESS) -1; >>>> + } else if (PcdGetBool (PcdSevEsIsEnabled)) { >>>> + // >>>> + // Next SEV-ES wakeup buffer allocation must be below this alloc= ation >>>> + // >>>> + mSevEsDxeWakeupBuffer =3D StartAddress; >>>> } >>>> =20 >>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart =3D %x, WakeupBufferSize = =3D %x\n", >>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Librar= y/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); >>>> =20 >>>> - // >>>> - // 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 o= f >>>> - // processors for calculating the total stack area. >>>> - // >>>> - Size +=3D AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProces= sorNumber); >>>> - >>>> - Size =3D ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT); >>>> - } >>>> - >>>> return Size; >>>> } >>>> =20 >>>> @@ -1192,6 +1178,7 @@ AllocateResetVector ( >>>> ) >>>> { >>>> UINTN ApResetVectorSize; >>>> + UINTN ApResetStackSize; >>>> =20 >>>> if (CpuMpData->WakeupBuffer =3D=3D (UINTN) -1) { >>>> ApResetVectorSize =3D GetApResetVectorSize (&CpuMpData->Address= Map); >>>> @@ -1207,9 +1194,39 @@ AllocateResetVector ( >>>> CpuMpData->AddressMap.ModeTrans= itionOffset >>>> ); >>>> // >>>> - // The reset stack starts at the end of the buffer. >>>> + // The AP reset stack is only used by SEV-ES guests. Do not allo= cate it >>>> + // if SEV-ES is not enabled. >>>> // >>>> - CpuMpData->SevEsAPResetStackStart =3D CpuMpData->WakeupBuffer + = ApResetVectorSize; >>>> + if (PcdGetBool (PcdSevEsIsEnabled)) { >>>> + // >>>> + // Stack location is based on ProcessorNumber, so use the tota= l 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 =3D (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 siz= ed and >>>> + // below the previously allocated buffer. >>>> + // >>>> + CpuMpData->SevEsAPResetStackStart =3D GetWakeupBuffer (ApReset= StackSize); >>>> + >>>> + // >>>> + // Check to be sure that the "allocate below" behavior hasn't = changed. >>>> + // This will also catch a failed allocation, as "-1" is return= ed on >>>> + // failure. >>>> + // >>>> + if (CpuMpData->SevEsAPResetStackStart >=3D CpuMpData->WakeupBu= ffer) { >>>> + 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/Lib= rary/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 >>>> =20 >>>> +STATIC UINT64 mSevEsPeiWakeupBuffer =3D BASE_1MB; >>>> + >>>> /** >>>> S3 SMM Init Done notification function. >>>> =20 >>>> @@ -220,7 +222,13 @@ GetWakeupBuffer ( >>>> // Need memory under 1MB to be collected here >>>> // >>>> WakeupBufferEnd =3D 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 =3D mSevEsPeiWakeupBuffer; >>>> + } else if (WakeupBufferEnd > BASE_1MB) { >>>> // >>>> // Wakeup buffer should be under 1MB >>>> // >>>> @@ -244,6 +252,15 @@ GetWakeupBuffer ( >>>> } >>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart =3D %x, WakeupBuff= erSize =3D %x\n", >>>> WakeupBufferStart, WakeupBufferSize)= ); >>>> + >>>> + if (PcdGetBool (PcdSevEsIsEnabled)) { >>>> + // >>>> + // Next SEV-ES wakeup buffer allocation must be below th= is >>>> + // allocation >>>> + // >>>> + mSevEsPeiWakeupBuffer =3D 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 th= e >>> 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 are= a >>> -- 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=20 way as well. If it ever causes any of the issues I outlined, it could be= =20 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 > > > >=20 > >