Re: [edk2-devel] [PATCH v2 9/9] UefiCpuPkg/CpuMpPei: Enable paging and set NP flag to avoid TOCTOU (CVE-2019-11098)

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

On 07/02/20 07:15, Guomin Jiang wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
> 
> To avoid the TOCTOU, enable paging and set Not Present flag so when
> access any code in the flash range, it will trigger #NP exception.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> ---
>  UefiCpuPkg/CpuMpPei/CpuMpPei.inf |  3 +++
>  UefiCpuPkg/CpuMpPei/CpuPaging.c  | 17 +++++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> index caead3ce34d4..fd50b55f06cb 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> @@ -46,6 +46,9 @@ [LibraryClasses]
>    BaseMemoryLib
>    CpuLib
>  
> +[Guids]
> +  gEdkiiMigratedFvInfoGuid                                             ## SOMETIMES_CONSUMES     ## HOB
> +
>  [Ppis]
>    gEfiPeiMpServicesPpiGuid                      ## PRODUCES
>    gEfiSecPlatformInformationPpiGuid             ## SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index d0cbebf70bbf..af4069b42cdb 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -12,6 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/CpuLib.h>
>  #include <Library/BaseLib.h>
> +#include <Guid/MigratedFvInfo.h>
>  
>  #include "CpuMpPei.h"
>  
> @@ -605,6 +606,8 @@ MemoryDiscoveredPpiNotifyCallback (
>    EFI_STATUS  Status;
>    BOOLEAN     InitStackGuard;
>    BOOLEAN     InterruptState;
> +  EDKII_MIGRATED_FV_INFO *MigratedFvInfo;
> +  EFI_PEI_HOB_POINTERS   Hob;
>  
>    InterruptState = SaveAndDisableInterrupts ();
>    Status = MigrateGdt ();
> @@ -617,9 +620,9 @@ MemoryDiscoveredPpiNotifyCallback (
>    // the task switch (for the sake of stack switch).
>    //
>    InitStackGuard = FALSE;
> -  if (IsIa32PaeSupported () && PcdGetBool (PcdCpuStackGuard)) {
> +  if (IsIa32PaeSupported ()) {
>      EnablePaging ();
> -    InitStackGuard = TRUE;
> +    InitStackGuard = PcdGetBool (PcdCpuStackGuard);
>    }
>  
>    Status = InitializeCpuMpWorker ((CONST EFI_PEI_SERVICES **)PeiServices);
> @@ -629,6 +632,16 @@ MemoryDiscoveredPpiNotifyCallback (
>      SetupStackGuardPage ();
>    }
>  
> +  Hob.Raw  = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid);
> +  while (Hob.Raw != NULL) {
> +    MigratedFvInfo = GET_GUID_HOB_DATA (Hob);
> +    ConvertMemoryPageAttributes (MigratedFvInfo->FvOrgBase, MigratedFvInfo->FvLength, 0);
> +
> +    Hob.Raw = GET_NEXT_HOB (Hob);
> +    Hob.Raw = GetNextGuidHob (&gEdkiiMigratedFvInfoGuid, Hob.Raw);
> +  }
> +  CpuFlushTlb ();
> +
>    return Status;
>  }
>  
> 

(1) This patch calls EnablePaging() even if no
"gEdkiiMigratedFvInfoGuid" HOB exists.

In other words, assume "PcdMigrateTemporaryRamFirmwareVolumes" is FALSE.

- In that case, PeiCore() will not call EvacuateTempRam().

- Consequently, zero "gEdkiiMigratedFvInfoGuid" HOBs are going to be built.

- Consequently, this patch will never call ConvertMemoryPageAttributes()

Why do we call EnablePaging() then (assuming IsIa32PaeSupported()
returns TRUE)?


(2) Consider the opposite case. Assume IsIa32PaeSupported() returns
FALSE. Further assume that at least one "gEdkiiMigratedFvInfoGuid" HOB
exists.

Then ConvertMemoryPageAttributes() will be called, without us ever
calling EnablePaging(). That doesn't seem right.

I suggest:


  BOOLEAN              InitStackGuard;
  EFI_PEI_HOB_POINTERS Hob;

  InitStackGuard = FALSE;
  Hob.Raw = NULL;

  //
  // Both the "stack guard" feature and the "temp RAM evacuation"
  // feature depend on IA32 PAE support.
  //
  if (IsIa32PaeSupported ()) {
    InitStackGuard = PcdGetBool (PcdCpuStackGuard);
    Hob.Raw = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid);
  }

  //
  // If either feature is active, then paging is required; enable it.
  //
  if (InitStackGuard || Hob.Raw != NULL) {
    EnablePaging ();
  }

  /* ... */

  if (InitStackGuard) {
    SetupStackGuardPage ();
  }

  /* note: no need to call GetFirstGuidHob() again! */
  while (Hob.Raw != NULL) {
    /* ... */
  }
  CpuFlushTlb ();


Thanks
Laszlo