Re: [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

["Laszlo Ersek" <lersek@redhat.com>]

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