* [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT. @ 2020-09-04 1:51 Dong, Eric 2020-09-04 3:48 ` [edk2-devel] " Yao, Jiewen 2020-09-04 7:30 ` Laszlo Ersek 0 siblings, 2 replies; 4+ messages in thread From: Dong, Eric @ 2020-09-04 1:51 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Laszlo Ersek 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 <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> --- 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|BOOLEAN|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|BOOLEAN|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_PROMPT #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?<BR><BR>\n" "TRUE - locked.<BR>\n" "FALSE - unlocked.<BR>" -- 2.23.0.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT. 2020-09-04 1:51 [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT Dong, Eric @ 2020-09-04 3:48 ` Yao, Jiewen 2020-09-04 7:08 ` Laszlo Ersek 2020-09-04 7:30 ` Laszlo Ersek 1 sibling, 1 reply; 4+ messages in thread From: Yao, Jiewen @ 2020-09-04 3:48 UTC (permalink / raw) To: devel@edk2.groups.io, Dong, Eric; +Cc: Ni, Ray, Laszlo Ersek I provided the feedback in https://bugzilla.tianocore.org/show_bug.cgi?id=2954. I suggest we justify the requirement at first. Thank you Yao Jiewen > -----Original Message----- > From: devel@edk2.groups.io <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 <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > 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 <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > --- > > 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?<BR><BR>\n" > > "TRUE - locked.<BR>\n" > > "FALSE - unlocked.<BR>" > > -- > 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] > -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT. 2020-09-04 3:48 ` [edk2-devel] " Yao, Jiewen @ 2020-09-04 7:08 ` Laszlo Ersek 0 siblings, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2020-09-04 7:08 UTC (permalink / raw) To: Yao, Jiewen, devel@edk2.groups.io, Dong, Eric; +Cc: Ni, Ray 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 <https://bugzilla.tianocore.org/show_bug.cgi?id=2954#c2>, 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 <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 <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> >> 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 <eric.dong@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> --- >> >> 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?<BR><BR>\n" >> >> "TRUE - locked.<BR>\n" >> >> "FALSE - unlocked.<BR>" >> >> -- >> 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] >> -=-=-=-=-=-= > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT. 2020-09-04 1:51 [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT Dong, Eric 2020-09-04 3:48 ` [edk2-devel] " Yao, Jiewen @ 2020-09-04 7:30 ` Laszlo Ersek 1 sibling, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2020-09-04 7:30 UTC (permalink / raw) To: Eric Dong, devel; +Cc: Ray Ni On 09/04/20 03:51, Eric Dong wrote: > 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 <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > --- > 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()) { (1) missing space character > + 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()) { (2) missing space character > + 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()) { (3) missing space character > + 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()) { (4) missing space character > + 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|BOOLEAN|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|BOOLEAN|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_PROMPT #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?<BR><BR>\n" > "TRUE - locked.<BR>\n" > "FALSE - unlocked.<BR>" > (5) For consistency with the other entries in the UNI file, I think: (5a) "PcdEnableCpuApCr3GdtIdtCheck_PROMPT" should come after "PcdCpuSmmFeatureControlMsrLock_HELP", and (5b) we should add a HELP item as well, for the new entry. (Personally I don't care for UNI files *at all*, but I think it's best to preserve consistency with the existent contents of this file.) Thanks! Laszlo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-04 7:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-04 1:51 [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT Dong, Eric 2020-09-04 3:48 ` [edk2-devel] " Yao, Jiewen 2020-09-04 7:08 ` Laszlo Ersek 2020-09-04 7:30 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox