Re: [edk2-devel] [PATCH v2 1/9] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore (CVE-2019-11098)

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

On 07/02/20 07:15, Guomin Jiang wrote:

> diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> index cca57c4c0686..802cd239e2eb 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> @@ -418,6 +418,22 @@ PeiCore (
>        ProcessPpiListFromSec ((CONST EFI_PEI_SERVICES **) &PrivateData.Ps, PpiList);
>      }
>    } else {
> +    if (
> +      (!(PrivateData.HobList.HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnBoot)) ||
> +      ((PrivateData.HobList.HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnS3Boot))
> +      ) {

(1) This condition is expressed very confusingly.

First, it is unhelpful to express the condition

  BootMode != BOOT_ON_S3_RESUME

as

  !(BootMode == BOOT_ON_S3_RESUME)

Second, we can simplify this a lot, by selecting the PCD explicitly,
dependent on boot mode, that controls shadowing.

Put differently, if the boot mode is *not* S3, and PcdShadowPeimOnBoot
is FALSE (and so we will not shadow the PEI core), then it makes no
sense to check the boot mode *again*, on the next line.

So I suggest:

  BOOLEAN Shadow;

  if (PrivateData.HobList.HandoffInformationTable->BootMode ==
      BOOT_ON_S3_RESUME) {
    Shadow = PcdGetBool (PcdShadowPeimOnS3Boot);
  } else {
    Shadow = PcdGetBool (PcdShadowPeimOnBoot);
  }

  if (Shadow) {
    //
    // ...
    //
  }


> +      DEBUG ((DEBUG_VERBOSE, "PPI lists before temporary RAM evacuation:\n"));
> +      DumpPpiList (&PrivateData);
> +
> +      //
> +      // Migrate installed content from Temporary RAM to Permanent RAM
> +      //
> +      EvacuateTempRam (&PrivateData, SecCoreData);
> +
> +      DEBUG ((DEBUG_VERBOSE, "PPI lists after temporary RAM evacuation:\n"));
> +      DumpPpiList (&PrivateData);
> +    }
> +
>      //
>      // Try to locate Temporary RAM Done Ppi.
>      //

Thanks
Laszlo