From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web10.16330.1599145911563847634 for ; Thu, 03 Sep 2020 08:11:51 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: eric.dong@intel.com) IronPort-SDR: bTEn2Nlaf8/ra0WJiizHnTc6laJbDQPunLGWJQydf+sU++gt8ZpIpH3KTWXXRlSuYTHBVPwPsn j7/+gr6BaCrg== X-IronPort-AV: E=McAfee;i="6000,8403,9733"; a="154990545" X-IronPort-AV: E=Sophos;i="5.76,387,1592895600"; d="scan'208";a="154990545" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Sep 2020 08:11:50 -0700 IronPort-SDR: 5wrBJ/eMm/3fQFuxoN7JxWrdN4LHqDSZ0mGdhzOHmxYG2bILZrsrxKc2omHaSBct1zO9PXUhVs UtsyIxPl1/8Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.76,387,1592895600"; d="scan'208";a="503093858" Received: from ydong10-desktop.ccr.corp.intel.com ([10.239.154.145]) by fmsmga005.fm.intel.com with ESMTP; 03 Sep 2020 08:11:49 -0700 From: "Dong, Eric" To: devel@edk2.groups.io Cc: Ray Ni , Laszlo Ersek Subject: [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. Date: Thu, 3 Sep 2020 23:11:47 +0800 Message-Id: <20200903151147.1196-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.Base and IDT.Base to not above 32 bits restriction. Change-Id: I231180f45d9f542641082c57d001e38e3f6759d5 Signed-off-by: Eric Dong Cc: Ray Ni Cc: Laszlo Ersek ---=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 | 9 +++ UefiCpuPkg/Library/MpInitLib/MpLib.c | 76 +++++++++++++++++++ UefiCpuPkg/Library/MpInitLib/MpLib.h | 12 +++ UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 + UefiCpuPkg/UefiCpuPkg.dec | 4 + 6 files changed, 103 insertions(+) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Lib= rary/MpInitLib/DxeMpInitLib.inf index 1771575c69..20851f251a 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf @@ -76,3 +76,4 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase ## = SOMETIMES_CONSUMES=0D gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## = CONSUMES=0D gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase ## = CONSUMES=0D + gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck ## = CONSUMES \ No newline at end of file diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/M= pInitLib/DxeMpLib.c index 2c00d72dde..f598372c4d 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c @@ -394,6 +394,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 diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpIn= itLib/MpLib.c index 07426274f6..69a0372df7 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -1909,6 +1909,54 @@ CheckAllAPs ( return EFI_NOT_READY;=0D }=0D =0D +/**=0D + Check whether CR3/GDT/IDT value valid for AP.=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 ((IA32_DESCRIPTOR *) &Gdtr);=0D + if ((Gdtr.Base >=3D BASE_4GB) || (Gdtr.Base + Gdtr.Limit >=3D BASE_4GB))= {=0D + return FALSE;=0D + }=0D +=0D + AsmReadIdtr ((IA32_DESCRIPTOR *) &Idtr);=0D + if ((Idtr.Base >=3D BASE_4GB) || (Idtr.Base + Idtr.Limit >=3D BASE_4GB))= {=0D + return FALSE;=0D + }=0D +=0D + return TRUE;=0D +}=0D +=0D /**=0D MP Initialize Library initialization.=0D =0D @@ -2318,6 +2366,13 @@ SwitchBSPWorker ( return EFI_NOT_READY;=0D }=0D =0D + //=0D + // Check whether CR3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCR3GdtIdtCheck()) {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D +=0D CpuMpData->BSPInfo.State =3D CPU_SWITCH_STATE_IDLE;=0D CpuMpData->APInfo.State =3D CPU_SWITCH_STATE_IDLE;=0D CpuMpData->SwitchBspFlag =3D TRUE;=0D @@ -2420,6 +2475,13 @@ EnableDisableApWorker ( return EFI_NOT_FOUND;=0D }=0D =0D + //=0D + // Check whether CR3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCR3GdtIdtCheck()) {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D +=0D if (!EnableAP) {=0D SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateDisabled);=0D } else {=0D @@ -2607,6 +2669,13 @@ StartupAllCPUsWorker ( return EFI_DEVICE_ERROR;=0D }=0D =0D + //=0D + // Check whether CR3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCR3GdtIdtCheck()) {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D +=0D //=0D // Update AP state=0D //=0D @@ -2772,6 +2841,13 @@ StartupThisAPWorker ( return EFI_INVALID_PARAMETER;=0D }=0D =0D + //=0D + // Check whether CR3/GDT/IDT valid for AP.=0D + //=0D + if (!ValidCR3GdtIdtCheck()) {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D +=0D //=0D // Update AP state=0D //=0D diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpIn= itLib/MpLib.h index 02652eaae1..4ac5cb51e3 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h @@ -740,5 +740,17 @@ PlatformShadowMicrocode ( IN OUT CPU_MP_DATA *CpuMpData=0D );=0D =0D +/**=0D + Check whether CR3/GDT/IDT value valid for AP.=0D +=0D + @retval TRUE Pass the check.=0D + @retval FALSE Fail the check.=0D +=0D +**/=0D +BOOLEAN=0D +ValidCR3GdtIdtCheck (=0D + VOID=0D + );=0D +=0D #endif=0D =0D diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Lib= rary/MpInitLib/PeiMpInitLib.inf index 34abf25d43..0ca86fcdaa 100644 --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf @@ -65,6 +65,7 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled ## CONS= UMES=0D gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase ## SOME= TIMES_CONSUMES=0D gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase ## CONS= UMES=0D + gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck ## CONS= UMES=0D =0D [Ppis]=0D gEdkiiPeiShadowMicrocodePpiGuid ## SOMETIMES_CONSUMES=0D diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index d83c084467..467ec5e001 100644 --- a/UefiCpuPkg/UefiCpuPkg.dec +++ b/UefiCpuPkg/UefiCpuPkg.dec @@ -187,6 +187,10 @@ # @Prompt Configure stack size for Application Processor (AP)=0D gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize|0x8000|UINT32|0x00000003=0D =0D + ## This value specifies whether need to check the CR3/GDT/IDT value for = AP.=0D + # @Prompt Whether need to check the CR3/GDT/IDT value for AP=0D + gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x3= 0000044=0D +=0D ## Specifies stack size in the temporary RAM. 0 means half of TemporaryR= amSize.=0D # @Prompt Stack size in the temporary RAM.=0D gUefiCpuPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0|UINT32|0x1000100= 3=0D --=20 2.23.0.windows.1