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.web11.33483.1599204651378607566 for ; Fri, 04 Sep 2020 00:30:51 -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-372-cPuCZiSBMoaFxqQtL-rPHw-1; Fri, 04 Sep 2020 03:30:49 -0400 X-MC-Unique: cPuCZiSBMoaFxqQtL-rPHw-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 2F38D57050; Fri, 4 Sep 2020 07:30:48 +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 2397A81197; Fri, 4 Sep 2020 07:30:46 +0000 (UTC) Subject: Re: [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20200904015156.1273-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <84bfca3c-1cb4-c21e-eefc-322c41f0e258@redhat.com> Date: Fri, 4 Sep 2020 09:30:46 +0200 MIME-Version: 1.0 In-Reply-To: <20200904015156.1273-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US 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 > 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()) { (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?

\n" > "TRUE - locked.
\n" > "FALSE - unlocked.
" > (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