Re: [edk2-devel] [PATCH v2 4/9] MdeModulePkg/DxeIplPeim: Register for shadow on S3 shadowed boot (CVE-2019-11098)

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

On 07/02/20 07:15, Guomin Jiang wrote:
> From: Jian J Wang <jian.j.wang@intel.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
> 
> 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>
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 3 +++
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c  | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)

(1) The commit message is empty, and therefore useless. Please explain
why this change is being made.

> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 3f1702854660..4ab54594ed66 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -121,6 +121,9 @@ [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy       ## SOMETIMES_CONSUMES
>  
> +[Pcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot          ## CONSUMES
> +
>  [Depex]
>    gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
>  
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> index d48028cea0dd..9e1831c69819 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -77,7 +77,7 @@ PeimInitializeDxeIpl (
>  
>    BootMode = GetBootModeHob ();
>  
> -  if (BootMode != BOOT_ON_S3_RESUME) {
> +  if (BootMode != BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot)) {
>      Status = PeiServicesRegisterForShadow (FileHandle);
>      if (Status == EFI_SUCCESS) {
>        //
> 

(2) The above check does not seem complete. I think it should consider
"PcdMigrateTemporaryRamFirmwareVolumes".

I don't exactly understand the impact of the change, but it seems to
potentially affect even such platforms that set
"PcdMigrateTemporaryRamFirmwareVolumes" to FALSE; and that seems wrong.

Thanks
Laszlo