Re: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098)

["Liming Gao" <liming.gao@intel.com>]

Guomin:
  I add my comments. 

-----Original Message-----
From: Wang, Jian J <jian.j.wang@intel.com> 
Sent: 2020年7月22日 14:59
To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>; De, Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>; West, Catharine <catharine.west@intel.com>
Subject: RE: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098)

Guomin,

See my inline comments below.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin 
> Jiang
> Sent: Monday, July 20, 2020 7:30 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming 
> <liming.gao@intel.com>; De, Debkumar <debkumar.de@intel.com>; Han, 
> Harry <harry.han@intel.com>; West, Catharine 
> <catharine.west@intel.com>
> Subject: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid 
> redundant shadow when enable the Migrated PCD (CVE-2019-11098)
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
> 
> When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it will shadow the 
> PEIMs, when it is disabled, PEIMs marked REGISTER_FOR_SHADOW will be 
> shadowed as well and it is controled by PcdShadowPeimOnBoot and 
> PcdShadowPeimOnS3Boot.
> 
> To cover the shadow behavior, the change will always shadow PEIMs when 
> enable PcdMigrateTemporaryRamFirmwareVolumes.
> 
> When PcdMigrateTemporaryRamFirmwareVolumes is true, if enable 
> PcdShadowPeiOnBoot, it will shadow some PEIMs twice and occupy more 
> memory and waste more boot time. it is unnecessary, so the only valid 
> choice is to enable PcdMigrateTemporaryRamFirmwareVolumes and disable 
> PcdShadowPeimOnBoot.

You could add similar clarification in dec where the new PCD is defined.

> 
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Debkumar De <debkumar.de@intel.com>
> Cc: Harry Han <harry.han@intel.com>
> Cc: Catharine West <catharine.west@intel.com>
> ---
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 11 +++++++++--
>  MdeModulePkg/Core/Pei/Image/Image.c           | 14 ++++++++++----
>  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       | 18 +++++++++++++-----
>  3 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 210b5b22f727..74cd5387c158 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1408,7 +1408,11 @@ PeiDispatcher (
>    PeimFileHandle = NULL;
>    EntryPoint     = 0;
> 
> -  if ((Private->PeiMemoryInstalled) && (Private-
> >HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME ||
> PcdGetBool (PcdShadowPeimOnS3Boot))) {
> +  if ((Private->PeiMemoryInstalled) &&
> +      (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
> +       || (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME)
> +       || PcdGetBool (PcdShadowPeimOnS3Boot))
> +    ) {
>      //
>      // Once real memory is available, shadow the RegisterForShadow modules.
> And meanwhile
>      // update the modules' status from PEIM_STATE_REGISTER_FOR_SHADOW 
> to PEIM_STATE_DONE.
> @@ -1607,7 +1611,10 @@ PeiDispatcher (
>              PeiCheckAndSwitchStack (SecCoreData, Private);
> 
>              if ((Private->PeiMemoryInstalled) && (Private-
> >Fv[FvCount].PeimState[PeimCount] == PEIM_STATE_REGISTER_FOR_SHADOW)
> &&   \
> -                (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot))) {
> +                (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
> +                 || 
> + (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME)
> +                 || PcdGetBool (PcdShadowPeimOnS3Boot))
> +              ) {
>                //
>                // If memory is available we shadow images by default 
> for performance reasons.
>                // We call the entry point a 2nd time so the module knows it's shadowed.


Below condition also needs to consider new PCD PcdMigrateTemporaryRamFirmwareVolumes. 

> diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
> b/MdeModulePkg/Core/Pei/Image/Image.c
> index 0caeb63e26b4..85d1a84e4b67 100644
> --- a/MdeModulePkg/Core/Pei/Image/Image.c
> +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> @@ -328,8 +328,11 @@ LoadAndRelocatePeCoffImage (
>    //
>    // When Image has no reloc section, it can't be relocated into memory.
>    //
> -  if (ImageContext.RelocationsStripped && 
> (Private->PeiMemoryInstalled) &&
> ((!IsPeiModule) ||
> -      (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
> IsRegisterForShadow)) || (IsS3Boot && PcdGetBool
> (PcdShadowPeimOnS3Boot)))) {
> +  if (ImageContext.RelocationsStripped && (Private->PeiMemoryInstalled)
> +      && ((!IsPeiModule) || PcdGetBool
> (PcdMigrateTemporaryRamFirmwareVolumes)
> +          || (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
> IsRegisterForShadow))
> +          || (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot)))
> +     ) {
>      DEBUG ((EFI_D_INFO|EFI_D_LOAD, "The image at 0x%08x without reloc 
> section can't be loaded into memory\n", (UINTN) Pe32Data));
>    }
> 

PcdMigrateTemporaryRamFirmwareVolumes is not required here, because FV has been shadow. 
PEIM is not required to shadow again. 

> @@ -343,8 +346,11 @@ LoadAndRelocatePeCoffImage (
>    // On normal boot, PcdShadowPeimOnBoot decides whether load PEIM or 
> PeiCore into memory.
>    // On S3 boot, PcdShadowPeimOnS3Boot decides whether load PEIM or 
> PeiCore into memory.
>    //
> -  if ((!ImageContext.RelocationsStripped) && 
> (Private->PeiMemoryInstalled) &&
> ((!IsPeiModule) ||
> -      (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
> IsRegisterForShadow)) || (IsS3Boot && PcdGetBool
> (PcdShadowPeimOnS3Boot)))) {
> +  if ((!ImageContext.RelocationsStripped) && (Private->PeiMemoryInstalled)
> +      && ((!IsPeiModule) || PcdGetBool
> (PcdMigrateTemporaryRamFirmwareVolumes)
> +          || (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
> IsRegisterForShadow))
> +          || (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot)))
> +     ) {
>      //
>      // Allocate more buffer to avoid buffer overflow.
>      //
> diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> index 48605eeada86..ce8649f954a8 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> @@ -322,7 +322,8 @@ PeiCore (
>        // PEI Core and PEIMs to get high performance.
>        //
>        OldCoreData->ShadowedPeiCore = (PEICORE_FUNCTION_POINTER) 
> (UINTN) PeiCore;
> -      if ((HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME &&
> PcdGetBool (PcdShadowPeimOnS3Boot))
> +      if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
> +          || (HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME 
> + &&
> PcdGetBool (PcdShadowPeimOnS3Boot))
>            || (HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME 
> && PcdGetBool (PcdShadowPeimOnBoot))) {
>          OldCoreData->ShadowedPeiCore = ShadowPeiCore (OldCoreData);
>        }
> @@ -422,10 +423,17 @@ PeiCore (
>      }
>    } else {
>      if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) {
> -      if (PrivateData.HobList.HandoffInformationTable->BootMode ==
> BOOT_ON_S3_RESUME) {
> -        TempRamEvacuation = PcdGetBool (PcdShadowPeimOnS3Boot);
> -      } else {
> -        TempRamEvacuation = PcdGetBool (PcdShadowPeimOnBoot);
> +      TempRamEvacuation = TRUE;
> +
> +      //
> +      // When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it make
> sense only

make -> makes

> +      // when PcdShadowPeimOnBoot is FALSE. in this situation, all 
> + PEIMs will be
> copied

in -> In

> +      // and shadowed, and doesn't need shadow PEIMs again, it will 
> + occupy
> more
> +      // memory and waste more time if you enable it.
> +      //
> +      if (PcdGetBool (PcdShadowPeimOnBoot)) {
> +        DEBUG ((DEBUG_ERROR, "!!!IMPORTANT NOTICE!!!\nWhen you see 
> + the
> message, it mean that you enable the
> PcdMigrateTemporaryRamFirmwareVolumes and PcdShadowPeimOnBoot at the 
> same time\nIt make no sense because it will occupy more memory and 
> waste more time.\nYou must disable PcdShadowPeimOnBoot when you enable 
> PcdMigrateTemporaryRamFirmwareVolumes for performance reason.\n\n"));

Too long string literal. You can split it into more string literals separated by space.
For example,

DEBUG ((
  DEBUG_INFO,
  "String part 1"
  "String part 2"
  "String part 3"
  ));


> +        ASSERT ((PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) 
> + ==
> TRUE) && (PcdGetBool (PcdShadowPeimOnBoot) == FALSE));
>        }
>      }
> 

ASSERT should also handle PcdShadowPeimOnS3Boot.

Thanks
Liming

> --
> 2.25.1.windows.1
> 
> 
>