Topics

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

Guomin Jiang
 

From: Jian J Wang <jian.j.wang@...>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Liming Gao <liming.gao@...>
Signed-off-by: Jian J Wang <jian.j.wang@...>
---
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 3 +++
MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)

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.25.1.windows.1

Laszlo Ersek
 

On 07/02/20 07:15, Guomin Jiang wrote:
From: Jian J Wang <jian.j.wang@...>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Liming Gao <liming.gao@...>
Signed-off-by: Jian J Wang <jian.j.wang@...>
---
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

Laszlo Ersek
 

On 07/03/20 16:00, Laszlo Ersek wrote:
On 07/02/20 07:15, Guomin Jiang wrote:
From: Jian J Wang <jian.j.wang@...>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Liming Gao <liming.gao@...>
Signed-off-by: Jian J Wang <jian.j.wang@...>
---
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.
... On further consideration, this patch seems to be fixing a
preexistent bug that is not related to the CVE at all. I think this
issue was simply exposed when testing the new feature. Is that right?

If that's correct, then please explain this very clearly in the commit
message.

Thanks,
Laszlo