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

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

Hi Guomin,

On 07/08/20 10:10, Guomin Jiang wrote:

[...]

>  7. Add PcdMigrateTemporaryRamFirmwareVolumes to control if enable the
>     feature or not. when the PCD disable, the EvacuateTempRam() will
>     never be called.
> 
> The function control flow as below:
>   PeiCore()
>     DumpPpiList()
>     EvacuateTempRam()
>       ConvertPeiCorePpiPointers()
>         ConvertPpiPointersFv()
>       MigratePeimsInFv()
>         MigratePeim()
>           PeiGetPe32Data()
>           LoadAndRelocatePeCoffImageInPlace()
>       MigrateSecModulesInFv()
>       ConvertPpiPointersFv()
>       ConvertStatusCodeCallbacks()
>       ConvertFvHob()
>       RemoveFvHobsInTemporaryMemory()
>     DumpPpiList()

[...]

> diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> index cca57c4c0686..7be6e9f3b06c 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> @@ -176,6 +176,7 @@ PeiCore (
>    EFI_HOB_HANDOFF_INFO_TABLE  *HandoffInformationTable;
>    EFI_PEI_TEMPORARY_RAM_DONE_PPI *TemporaryRamDonePpi;
>    UINTN                       Index;
> +  BOOLEAN                     Shadow;
>  
>    //
>    // Retrieve context passed into PEI Core
> @@ -418,6 +419,27 @@ PeiCore (
>        ProcessPpiListFromSec ((CONST EFI_PEI_SERVICES **) &PrivateData.Ps, PpiList);
>      }
>    } else {
> +    if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) {
> +      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.
>      //

[...]

this is almost good, from my perspective, but you forgot to initialize
Shadow to FALSE. That's a problem because, if
PcdMigrateTemporaryRamFirmwareVolumes is FALSE, then Shadow is never
assigned (it will have an indeterminate value).

In my review here:

  https://edk2.groups.io/g/devel/message/62029

I inculded

  Shadow = FALSE;

too.

Please include this assignment either at the top of the function, or
just before you fetch "PcdMigrateTemporaryRamFirmwareVolumes".

Thanks!
Laszlo