Re: [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

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

On 09/03/20 17:11, 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.Base and IDT.Base to not above
> 32 bits restriction.
> 
> Change-Id: I231180f45d9f542641082c57d001e38e3f6759d5

(1) Please drop the Change-Id line.


(2) We should document that the motivation for this patch is a special
UEFI shell application that changes the GDT / CR3 to above 4GB.
Currently, neither the bugzilla, nor the commit message, nor the PCD
documentation in the DEC file explains this. So one is left wondering if
and why they should change the PCD to TRUE on their platform.

Please append the following two paragraphs to the commit message:

"""
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>
> ---
> 
> 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       |  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/Library/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
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ## CONSUMES
> \ No newline at end of file

(3) Can you insert this new line just after
"gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase" please? In order to
keep the "gUefiCpuPkgTokenSpaceGuid" PCDs grouped together.


> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72dde..f598372c4d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -394,6 +394,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()) {

(4) Missing space character after "ValidCR3GdtIdtCheck".

(Applies to all the other call sites as well.)


> +    CpuDeadLoop ();
> +  }
> +
>    CpuMpData = GetCpuMpData ();
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>    CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/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;
>  }
>  
> +/**
> +  Check whether CR3/GDT/IDT value valid for AP.
> +
> +  @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 ((IA32_DESCRIPTOR *) &Gdtr);

(5) The cast is superfluous, please remove it. According to <BaseLib.h>,
AsmReadGdtr() takes the following parameter:

  OUT     IA32_DESCRIPTOR           *Gdtr


> +  if ((Gdtr.Base >= BASE_4GB) || (Gdtr.Base + Gdtr.Limit >= BASE_4GB)) {
> +    return FALSE;
> +  }

I agree that both checks (even the second one) should use ">=", given
that Limit is an inclusive boundary.

My understanding is that we don't have to worry about any UINTN overflow
in the addition here, as Base and Limit come from the "live" GDTR.

(6) So we could even eliminate the first expression, and only do:

  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
    return FALSE;
  }

Because, given that we do not expect any overflows here, if this (2nd)
condition is FALSE, then the 1st condition must be FALSE too.

I don't insist on this simplification. If you also like the
simplification, then please
- update the commit message
- update the code comment above

because we will no longer check Base in itself.


> +
> +  AsmReadIdtr ((IA32_DESCRIPTOR *) &Idtr);

(7) The cast is superfluous.


> +  if ((Idtr.Base >= BASE_4GB) || (Idtr.Base + Idtr.Limit >= BASE_4GB)) {
> +    return FALSE;
> +  }

(8) Same comment as (6) -- please consider removing the 1st condition as
a simplification (if you disagree, that's OK too).


> +
> +  return TRUE;
> +}
> +
>  /**
>    MP Initialize Library initialization.
>  
> @@ -2318,6 +2366,13 @@ SwitchBSPWorker (
>      return EFI_NOT_READY;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    CpuMpData->BSPInfo.State = CPU_SWITCH_STATE_IDLE;
>    CpuMpData->APInfo.State  = CPU_SWITCH_STATE_IDLE;
>    CpuMpData->SwitchBspFlag = TRUE;

(9) I think we should return EFI_UNSUPPORTED, not EFI_INVALID_PARAMETER.


(10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".

Instead, the calls should be made in the DXE instance of the library
("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
functions:

- MpInitLibStartupAllAPs
- MpInitLibStartupThisAP
- MpInitLibSwitchBSP
- MpInitLibEnableDisableAP

Here's why:

(a) The symptom does not affect the PEI phase -- by the time the UEFI
application is executed, the PEI phase has ended; there's no need to
modify the PEI instance of the library.

(b) The currently proposed failure exits are too late. For example, in
the SwitchBSPWorker() function, by the time we exit, we have called
DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
DisableLvtInterrupts() -- and the error path does not restore the
original environment.

(c) According to the PI spec (v1.7), the StartupAllAPs(),
StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
first action in the above-listed DxeMpLib functions.

(Assuming the protocol members are called from an AP, and consequently
we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
*caller's* fault, per spec!)


> @@ -2420,6 +2475,13 @@ EnableDisableApWorker (
>      return EFI_NOT_FOUND;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    if (!EnableAP) {
>      SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateDisabled);
>    } else {
> @@ -2607,6 +2669,13 @@ StartupAllCPUsWorker (
>      return EFI_DEVICE_ERROR;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Update AP state
>    //
> @@ -2772,6 +2841,13 @@ StartupThisAPWorker (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Update AP state
>    //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/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
>    );
>  
> +/**
> +  Check whether CR3/GDT/IDT value valid for AP.
> +
> +  @retval  TRUE          Pass the check.
> +  @retval  FALSE         Fail the check.
> +
> +**/
> +BOOLEAN
> +ValidCR3GdtIdtCheck (

(11) In the function name, please write "Cr3", not "CR3". (See also:
AsmReadCr3().)


> +  VOID
> +  );
> +
>  #endif
>  
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/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                      ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                   ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                       ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck           ## CONSUMES
>  
>  [Ppis]
>    gEdkiiPeiShadowMicrocodePpiGuid        ## SOMETIMES_CONSUMES

(12) Superficially, comment (3) applies -- please keep the PCDs in the
UefiCpuPkg token space grouped together.

However, per my point (10), the PEI instance of the library should not
be modified at all; so please drop this hunk.


> 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)
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize|0x8000|UINT32|0x00000003
>  
> +  ## This value specifies whether need to check the CR3/GDT/IDT value for AP.
> +  # @Prompt Whether need to check the CR3/GDT/IDT value for AP
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x30000044
> +
>    ## Specifies stack size in the temporary RAM. 0 means half of TemporaryRamSize.
>    # @Prompt Stack size in the temporary RAM.
>    gUefiCpuPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0|UINT32|0x10001003
> 

OK, so this is [PcdsFixedAtBuild, PcdsPatchableInModule], not a
FeaturePCD. I proposed FeaturePCD because optimizing compilers are
supposed to eliminate it... But, I can see that BaseTools generates the
same kind of code for Fixed BOOLEAN PCDs as well. OK!

(13) I think you should modify the UNI file in accordance with the DEC file.

Thanks!
Laszlo