Re: [edk2-devel] [PATCH v2 7/9] MdeModulePkg/Core: Add switch to enable or disable TOCTOU feature (CVE-2019-11098)

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

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