From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web12.29280.1599184321461015379 for ; Thu, 03 Sep 2020 18:52:01 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: eric.dong@intel.com) IronPort-SDR: IDaXnB4vaOWcVKuaeN0eLMHD7rqdmGwjoYfTDErq1qYuabGLM+wgtXWRLJ6TKdT3sLlZWqdPaT Bcaab0e5toAg== X-IronPort-AV: E=McAfee;i="6000,8403,9733"; a="145382152" X-IronPort-AV: E=Sophos;i="5.76,387,1592895600"; d="scan'208";a="145382152" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Sep 2020 18:52:00 -0700 IronPort-SDR: aYYEmgtUkdKi+VM/ppda6XgrABXzrA/AebOM77bLkdHAMv3Gh3oPvJ71sYwDPn9pbR0sqB4La1 CLspxhcz07vQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.76,387,1592895600"; d="scan'208";a="503282037" Received: from ydong10-desktop.ccr.corp.intel.com ([10.239.154.145]) by fmsmga005.fm.intel.com with ESMTP; 03 Sep 2020 18:51:58 -0700 From: "Dong, Eric" To: devel@edk2.groups.io Cc: Ray Ni , Laszlo Ersek Subject: [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT. Date: Fri, 4 Sep 2020 09:51:56 +0800 Message-Id: <20200904015156.1273-1-eric.dong@intel.com> X-Mailer: git-send-email 2.23.0.windows.1 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2954 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 ---=0D V3:=0D only add check for the DxeMplib, no need for PeiMpLib.=0D =0D V2:=0D Change the check point. Just in the different caller to make the logic=0D clear. V1 patch add check just before the use of the code. It make the=0D logic complicated.=0D 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/Lib= rary/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=0D gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled ## = CONSUMES=0D gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase ## = SOMETIMES_CONSUMES=0D + gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck ## = CONSUMES=0D gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## = CONSUMES=0D gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase ## = CONSUMES=0D diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/M= pInitLib/DxeMpLib.c index 2c00d72dde..332b4447bb 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c @@ -29,6 +29,66 @@ VOID *mReservedApLoopFunc =3D NULL; UINTN mReservedTopOfApStack;=0D volatile UINT32 mNumberToFinish =3D 0;=0D =0D +/**=0D + Check whether Cr3/GDT/IDT value valid for the APs.=0D +=0D + @retval TRUE Pass the check.=0D + @retval FALSE Fail the check.=0D +=0D +**/=0D +BOOLEAN=0D +ValidCr3GdtIdtCheck (=0D + VOID=0D + )=0D +{=0D + IA32_DESCRIPTOR Gdtr;=0D + IA32_DESCRIPTOR Idtr;=0D +=0D + if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) {=0D + return TRUE;=0D + }=0D +=0D + //=0D + // AP needs to run from real mode to 32bit mode to LONG mode. Page table= =0D + // (pointed by Cr3) and GDT are necessary to set up to correct value whe= n=0D + // CPU execution mode is switched to LONG mode. IDT also necessary if th= e=0D + // exception happened.=0D + // AP uses the same location page table (Cr3) and GDT/IDT as what BSP us= es.=0D + // But when the page table or GDT is above 4GB, it's impossible for CPU= =0D + // to use because GDTR.base and Cr3 are 32bits before switching to LONG= =0D + // mode.=0D + // Here add check for the Cr3, GDT.Base and range, IDT.Base and range ar= e=0D + // not above 32 bits limitation.=0D + //=0D + if (AsmReadCr3 () >=3D BASE_4GB) {=0D + return FALSE;=0D + }=0D +=0D + AsmReadGdtr (&Gdtr);=0D + //=0D + // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit=0D + // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means= =0D + // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit=0D + // check.=0D + //=0D + if (Gdtr.Base + Gdtr.Limit >=3D BASE_4GB) {=0D + return FALSE;=0D + }=0D +=0D + AsmReadIdtr (&Idtr);=0D + //=0D + // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit=0D + // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means= =0D + // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit=0D + // check.=0D + //=0D + if (Idtr.Base + Idtr.Limit >=3D BASE_4GB) {=0D + return FALSE;=0D + }=0D +=0D + return TRUE;=0D +}=0D +=0D /**=0D Enable Debug Agent to support source debugging on AP function.=0D =0D @@ -394,6 +454,15 @@ MpInitChangeApLoopCallback ( {=0D CPU_MP_DATA *CpuMpData;=0D =0D + //=0D + // Check the Cr3/GDT/IDT before waking up AP.=0D + // If the check return fail, it will block later=0D + // OS boot, so halt the system here.=0D + //=0D + if (!ValidCr3GdtIdtCheck ()) {=0D + CpuDeadLoop ();=0D + }=0D +=0D CpuMpData =3D GetCpuMpData ();=0D CpuMpData->PmCodeSegment =3D GetProtectedModeCS ();=0D CpuMpData->Pm16CodeSegment =3D GetProtectedMode16CS ();=0D @@ -676,6 +745,13 @@ MpInitLibStartupAllAPs ( {=0D EFI_STATUS Status;=0D =0D + //=0D + // Check whether Cr3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCr3GdtIdtCheck()) {=0D + return EFI_UNSUPPORTED;=0D + }=0D +=0D //=0D // Temporarily stop checkAllApsStatus for avoid resource dead-lock.=0D //=0D @@ -783,6 +859,13 @@ MpInitLibStartupThisAP ( {=0D EFI_STATUS Status;=0D =0D + //=0D + // Check whether Cr3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCr3GdtIdtCheck()) {=0D + return EFI_UNSUPPORTED;=0D + }=0D +=0D //=0D // temporarily stop checkAllApsStatus for avoid resource dead-lock.=0D //=0D @@ -839,6 +922,13 @@ MpInitLibSwitchBSP ( EFI_TIMER_ARCH_PROTOCOL *Timer;=0D UINT64 TimerPeriod;=0D =0D + //=0D + // Check whether Cr3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCr3GdtIdtCheck()) {=0D + return EFI_UNSUPPORTED;=0D + }=0D +=0D TimerPeriod =3D 0;=0D //=0D // Locate Timer Arch Protocol=0D @@ -912,6 +1002,13 @@ MpInitLibEnableDisableAP ( EFI_STATUS Status;=0D BOOLEAN TempStopCheckState;=0D =0D + //=0D + // Check whether Cr3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCr3GdtIdtCheck()) {=0D + return EFI_UNSUPPORTED;=0D + }=0D +=0D TempStopCheckState =3D FALSE;=0D //=0D // temporarily stop checkAllAPsStatus for initialize parameters.=0D 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.=0D gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOLEAN|0x= 3213210B=0D =0D + ## This value specifies whether need to check the Cr3/GDT/IDT value for = the APs.=0D + # @Prompt Enable Cr3/GDT/IDT value check for the APs.=0D + gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x3= 0000044=0D +=0D [PcdsFixedAtBuild]=0D ## List of exception vectors which need switching stack.=0D # This PCD will only take into effect if PcdCpuStackGuard is enabled.=0D 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 @@ =0D #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_PROMP= T #language en-US "Lock SMM Feature Control MSR"=0D =0D +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdEnableCpuApCr3GdtIdtCheck_PROMPT = #language en-US "Enable Cr3/GDT/IDT value check for the APs"=0D +=0D #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP = #language en-US "Lock SMM Feature Control MSR?

\n"=0D = "TRUE - locked.
\n"=0D = "FALSE - unlocked.
"=0D --=20 2.23.0.windows.1