On 07/02/20 07:15, Guomin Jiang wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614 > > Add total switch to enable or disable TOCTOU feature, the vulnerability is > critical, so the switch is on normally but if you can disable it according > to your needs. > > 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> > Signed-off-by: Guomin Jiang <guomin.ji...@intel.com> > --- > MdeModulePkg/Core/Pei/PeiMain.inf | 1 + > MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 5 +++-- > MdeModulePkg/MdeModulePkg.dec | 5 +++++ > 3 files changed, 9 insertions(+), 2 deletions(-)
(1) The subject line of the patch is wrong. The expression "TOCTOU feature" makes no sense. Instead, the patch adds a PCD for controlling the "temporary RAM evacuation" feature that is implemented in patch#1 in this series. Please fix both the subject line, and the commit message -- as both contain the wrong expression "TOCTOU feature". > > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf > b/MdeModulePkg/Core/Pei/PeiMain.inf > index c80d16b4efa6..0cf357371a16 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.inf > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf > @@ -111,6 +111,7 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot > ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot > ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack > ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdMigrateTemporaryRamFirmwareVolumes > ## CONSUMES > > # [BootMode] > # S3_RESUME ## SOMETIMES_CONSUMES > diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > index 802cd239e2eb..bc78c3f8ad59 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > @@ -419,8 +419,9 @@ PeiCore ( > } > } else { > if ( > - (!(PrivateData.HobList.HandoffInformationTable->BootMode == > BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnBoot)) || > - ((PrivateData.HobList.HandoffInformationTable->BootMode == > BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnS3Boot)) > + ((!(PrivateData.HobList.HandoffInformationTable->BootMode == > BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnBoot)) || > + ((PrivateData.HobList.HandoffInformationTable->BootMode == > BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnS3Boot))) && > + PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) (2) The indentation of the new code is wrong. Before the patch, we have (A) || (B) After the patch, we have ((A) || (B)) && C The indentation of the line with "B" is wrong. It should be: ((A) || (B)) && C But, anyway, I've suggested under patch#1 a different way for expressing the same condition. So ultimately, in this patch, we should produce: BOOLEAN Shadow; Shadow = FALSE; 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); > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index 5e25cbe98ada..0a5a167f3e8b 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1223,6 +1223,11 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] > # @Prompt Shadow Peim and PeiCore on boot > gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|TRUE|BOOLEAN|0x30001029 > > + ## Indicate if to evacuate from temporary to permanent memory. > + # TRUE - Evacuate from temporary memory (3) Please drop the word "from". > + # FALSE - Keep the original behavior (4) You mean "original" as "before patch#1". Because, if the PCD is set to FALSE, then the feature introduced in patch#1 is disabled. But the word "original" lacks context when someone looks at the DEC file, later. Please explain unambiguously what happens when the PCD is set to FALSE. > + > gEfiMdeModulePkgTokenSpaceGuid.PcdMigrateTemporaryRamFirmwareVolumes|TRUE|BOOLEAN|0x3000102A > + > ## The mask is used to control memory profile behavior.<BR><BR> > # BIT0 - Enable UEFI memory profile.<BR> > # BIT1 - Enable SMRAM profile.<BR> > Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62029): https://edk2.groups.io/g/devel/message/62029 Mute This Topic: https://groups.io/mt/75252666/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-