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.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>
> Signed-off-by: Guomin Jiang <guomin.jiang@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-