From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.32969.1599203336279677561 for ; Fri, 04 Sep 2020 00:08:56 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) 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-180-fcTXeThrPz6JSeJnqftHvQ-1; Fri, 04 Sep 2020 03:08:54 -0400 X-MC-Unique: fcTXeThrPz6JSeJnqftHvQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EC54885C707; Fri, 4 Sep 2020 07:08:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-161.ams2.redhat.com [10.36.112.161]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5E58078B34; Fri, 4 Sep 2020 07:08:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT. To: "Yao, Jiewen" , "devel@edk2.groups.io" , "Dong, Eric" Cc: "Ni, Ray" References: <20200904015156.1273-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 4 Sep 2020 09:08:50 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0.003 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Hello Jiewen, On 09/04/20 05:48, Yao, Jiewen wrote: > I provided the feedback in https://bugzilla.tianocore.org/show_bug.cgi?id=2954. > > I suggest we justify the requirement at first. in comment , you write, > I don't treat it as a valid test case, to move page table, GDT, IDT in > UEFI environment. > > The CPU context should be managed by CPU driver, and only CPU driver. > Other driver or application should only change the system state via > UEFI/PI spec defined API. Otherwise, the system may run into an > unknown state. > > IMHO, I dont think it is a valid case. and I couldn't agree more with you -- I'm also convinced that what the particular UEFI application does is invalid. However, it seems like Eric really needs this change. I don't know his business case (maybe he's not at liberty to share it), but when he implied it was really important, I trusted him. So I figured we can accommodate this use case (even if we don't exactly know all the details, and even if we know for a fact that what the UEFI application does is *invalid*, per spec) -- but then the impact, on both performance, and on code complexity, should be minimal, for platforms that do not care. (And those platforms have the *right* not to care.) So I'm going to review this patch now *assuming* that the edk2 community is OK to accept the use case in the first place. I'm not *encouraging* the use case or whatever -- the use case is clearly wrong. But Eric seems to need it: maybe some of the circumstances are out of his control, and he can't just say "don't run this application", or "change the application". Perhaps it's a temporary fix in edk2 (Eric mentioned release time pressure, IIRC). I can fully appreciate that, and I'm OK to tolerate temporary fixes -- it's just that temporary fixes sometimes become permanent (increasing technical debt), and for that reason, I'd prefer this hack to be as clean and non-intrusive as possible. Thanks, Laszlo >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Dong, Eric >> Sent: Friday, September 4, 2020 9:52 AM >> To: devel@edk2.groups.io >> Cc: Ni, Ray ; Laszlo Ersek >> Subject: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for >> Cr3/GDT/IDT. >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954 >> >> AP needs to run from real mode to 32bit mode to LONG mode. Page table >> (pointed by Cr3) and GDT are necessary to set up to correct value when >> CPU execution mode is switched to LONG mode. >> AP uses the same location page table (Cr3) and GDT as what BSP uses. >> But when the page table or GDT is above 4GB, it's impossible for CPU >> to use because GDTR.base and Cr3 are 32bits before switching to LONG >> mode. >> >> This patch adds check for the Cr3, GDT and IDT to not above 4G limitation. >> >> The check is avoided -- assumed successful -- if the new >> PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason >> is that the 32-bit requirement is always ensured by edk2 itself; the >> requirement is only possibly invalidated by a particular UEFI shell >> application that manually moves the GDT/IDT/CR3 above 4GB. Platforms >> that don't intend to be compatible with such UEFI applications need not >> set the PCD to TRUE. >> >> If the PCD is TRUE and the check fails, then the StartupAllAPs(), >> StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are >> rejected at once. Reporting an error immediately is more graceful than >> hanging when the APs attempt to switch to long mode. >> >> Signed-off-by: Eric Dong >> Cc: Ray Ni >> Cc: Laszlo Ersek >> --- >> >> V3: >> >> only add check for the DxeMplib, no need for PeiMpLib. >> >> >> >> V2: >> >> Change the check point. Just in the different caller to make the logic >> >> clear. V1 patch add check just before the use of the code. It make the >> >> logic complicated. >> >> >> UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 + >> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 97 +++++++++++++++++++ >> UefiCpuPkg/UefiCpuPkg.dec | 4 + >> UefiCpuPkg/UefiCpuPkg.uni | 2 + >> 4 files changed, 104 insertions(+) >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> index 1771575c69..7792df516e 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> @@ -74,5 +74,6 @@ >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds >> ## CONSUMES >> >> gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled ## CONSUMES >> >> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase ## >> SOMETIMES_CONSUMES >> >> + gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck ## >> CONSUMES >> >> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## >> CONSUMES >> >> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase ## >> CONSUMES >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >> index 2c00d72dde..332b4447bb 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >> @@ -29,6 +29,66 @@ VOID *mReservedApLoopFunc = NULL; >> UINTN mReservedTopOfApStack; >> >> volatile UINT32 mNumberToFinish = 0; >> >> >> >> +/** >> >> + Check whether Cr3/GDT/IDT value valid for the APs. >> >> + >> >> + @retval TRUE Pass the check. >> >> + @retval FALSE Fail the check. >> >> + >> >> +**/ >> >> +BOOLEAN >> >> +ValidCr3GdtIdtCheck ( >> >> + VOID >> >> + ) >> >> +{ >> >> + IA32_DESCRIPTOR Gdtr; >> >> + IA32_DESCRIPTOR Idtr; >> >> + >> >> + if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) { >> >> + return TRUE; >> >> + } >> >> + >> >> + // >> >> + // AP needs to run from real mode to 32bit mode to LONG mode. Page table >> >> + // (pointed by Cr3) and GDT are necessary to set up to correct value when >> >> + // CPU execution mode is switched to LONG mode. IDT also necessary if the >> >> + // exception happened. >> >> + // AP uses the same location page table (Cr3) and GDT/IDT as what BSP uses. >> >> + // But when the page table or GDT is above 4GB, it's impossible for CPU >> >> + // to use because GDTR.base and Cr3 are 32bits before switching to LONG >> >> + // mode. >> >> + // Here add check for the Cr3, GDT.Base and range, IDT.Base and range are >> >> + // not above 32 bits limitation. >> >> + // >> >> + if (AsmReadCr3 () >= BASE_4GB) { >> >> + return FALSE; >> >> + } >> >> + >> >> + AsmReadGdtr (&Gdtr); >> >> + // >> >> + // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit >> >> + // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means >> >> + // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit >> >> + // check. >> >> + // >> >> + if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) { >> >> + return FALSE; >> >> + } >> >> + >> >> + AsmReadIdtr (&Idtr); >> >> + // >> >> + // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit >> >> + // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means >> >> + // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit >> >> + // check. >> >> + // >> >> + if (Idtr.Base + Idtr.Limit >= BASE_4GB) { >> >> + return FALSE; >> >> + } >> >> + >> >> + return TRUE; >> >> +} >> >> + >> >> /** >> >> Enable Debug Agent to support source debugging on AP function. >> >> >> >> @@ -394,6 +454,15 @@ MpInitChangeApLoopCallback ( >> { >> >> CPU_MP_DATA *CpuMpData; >> >> >> >> + // >> >> + // Check the Cr3/GDT/IDT before waking up AP. >> >> + // If the check return fail, it will block later >> >> + // OS boot, so halt the system here. >> >> + // >> >> + if (!ValidCr3GdtIdtCheck ()) { >> >> + CpuDeadLoop (); >> >> + } >> >> + >> >> CpuMpData = GetCpuMpData (); >> >> CpuMpData->PmCodeSegment = GetProtectedModeCS (); >> >> CpuMpData->Pm16CodeSegment = GetProtectedMode16CS (); >> >> @@ -676,6 +745,13 @@ MpInitLibStartupAllAPs ( >> { >> >> EFI_STATUS Status; >> >> >> >> + // >> >> + // Check whether Cr3/GDT/IDT valid for AP. >> >> + // >> >> + if (!ValidCr3GdtIdtCheck()) { >> >> + return EFI_UNSUPPORTED; >> >> + } >> >> + >> >> // >> >> // Temporarily stop checkAllApsStatus for avoid resource dead-lock. >> >> // >> >> @@ -783,6 +859,13 @@ MpInitLibStartupThisAP ( >> { >> >> EFI_STATUS Status; >> >> >> >> + // >> >> + // Check whether Cr3/GDT/IDT valid for AP. >> >> + // >> >> + if (!ValidCr3GdtIdtCheck()) { >> >> + return EFI_UNSUPPORTED; >> >> + } >> >> + >> >> // >> >> // temporarily stop checkAllApsStatus for avoid resource dead-lock. >> >> // >> >> @@ -839,6 +922,13 @@ MpInitLibSwitchBSP ( >> EFI_TIMER_ARCH_PROTOCOL *Timer; >> >> UINT64 TimerPeriod; >> >> >> >> + // >> >> + // Check whether Cr3/GDT/IDT valid for AP. >> >> + // >> >> + if (!ValidCr3GdtIdtCheck()) { >> >> + return EFI_UNSUPPORTED; >> >> + } >> >> + >> >> TimerPeriod = 0; >> >> // >> >> // Locate Timer Arch Protocol >> >> @@ -912,6 +1002,13 @@ MpInitLibEnableDisableAP ( >> EFI_STATUS Status; >> >> BOOLEAN TempStopCheckState; >> >> >> >> + // >> >> + // Check whether Cr3/GDT/IDT valid for AP. >> >> + // >> >> + if (!ValidCr3GdtIdtCheck()) { >> >> + return EFI_UNSUPPORTED; >> >> + } >> >> + >> >> TempStopCheckState = FALSE; >> >> // >> >> // temporarily stop checkAllAPsStatus for initialize parameters. >> >> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec >> index d83c084467..08ec36e76c 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.dec >> +++ b/UefiCpuPkg/UefiCpuPkg.dec >> @@ -148,6 +148,10 @@ >> # @Prompt Lock SMM Feature Control MSR. >> >> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOL >> EAN|0x3213210B >> >> >> >> + ## This value specifies whether need to check the Cr3/GDT/IDT value for the >> APs. >> >> + # @Prompt Enable Cr3/GDT/IDT value check for the APs. >> >> + >> gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEA >> N|0x30000044 >> >> + >> >> [PcdsFixedAtBuild] >> >> ## List of exception vectors which need switching stack. >> >> # This PCD will only take into effect if PcdCpuStackGuard is enabled. >> >> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni >> index 219c1963bf..3461a83003 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.uni >> +++ b/UefiCpuPkg/UefiCpuPkg.uni >> @@ -96,6 +96,8 @@ >> >> >> #string >> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_PROMP >> T #language en-US "Lock SMM Feature Control MSR" >> >> >> >> +#string >> STR_gUefiCpuPkgTokenSpaceGuid_PcdEnableCpuApCr3GdtIdtCheck_PROMPT >> #language en-US "Enable Cr3/GDT/IDT value check for the APs" >> >> + >> >> #string >> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP >> #language en-US "Lock SMM Feature Control MSR?

\n" >> >> "TRUE - locked.
\n" >> >> "FALSE - unlocked.
" >> >> -- >> 2.23.0.windows.1 >> >> >> -=-=-=-=-=-= >> Groups.io Links: You receive all messages sent to this group. >> >> View/Reply Online (#65019): https://edk2.groups.io/g/devel/message/65019 >> Mute This Topic: https://groups.io/mt/76621468/1772286 >> Group Owner: devel+owner@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen.yao@intel.com] >> -=-=-=-=-=-= >