Guomin: I add my comments. -----Original Message----- From: Wang, Jian J <jian.j.w...@intel.com> Sent: 2020年7月22日 14:59 To: devel@edk2.groups.io; Jiang, Guomin <guomin.ji...@intel.com> Cc: Wu, Hao A <hao.a...@intel.com>; Bi, Dandan <dandan...@intel.com>; Gao, Liming <liming....@intel.com>; De, Debkumar <debkumar...@intel.com>; Han, Harry <harry....@intel.com>; West, Catharine <catharine.w...@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.w...@intel.com>; Wu, Hao A > <hao.a...@intel.com>; Bi, Dandan <dandan...@intel.com>; Gao, Liming > <liming....@intel.com>; De, Debkumar <debkumar...@intel.com>; Han, > Harry <harry....@intel.com>; West, Catharine > <catharine.w...@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.ji...@intel.com> > Cc: Jian J Wang <jian.j.w...@intel.com> > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Dandan Bi <dandan...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Cc: Debkumar De <debkumar...@intel.com> > Cc: Harry Han <harry....@intel.com> > Cc: Catharine West <catharine.w...@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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#63075): https://edk2.groups.io/g/devel/message/63075 Mute This Topic: https://groups.io/mt/75679712/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
