Re: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098)
Guomin:
I add my comments.
-Original Message-
From: Wang, Jian J
Sent: 2020年7月22日 14:59
To: devel@edk2.groups.io; Jiang, Guomin
Cc: Wu, Hao A ; Bi, Dandan ; Gao,
Liming ; De, Debkumar ; Han, Harry
; West, Catharine
Subject: RE: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant
shadow when enable the Migrated PCD (CVE-2019-11098)
Guomin,
See my inline comments below.
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Guomin
> Jiang
> Sent: Monday, July 20, 2020 7:30 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J ; Wu, Hao A
> ; Bi, Dandan ; Gao, Liming
> ; De, Debkumar ; Han,
> Harry ; West, Catharine
>
> Subject: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid
> redundant shadow when enable the Migrated PCD (CVE-2019-11098)
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
>
> When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it will shadow the
> PEIMs, when it is disabled, PEIMs marked REGISTER_FOR_SHADOW will be
> shadowed as well and it is controled by PcdShadowPeimOnBoot and
> PcdShadowPeimOnS3Boot.
>
> To cover the shadow behavior, the change will always shadow PEIMs when
> enable PcdMigrateTemporaryRamFirmwareVolumes.
>
> When PcdMigrateTemporaryRamFirmwareVolumes is true, if enable
> PcdShadowPeiOnBoot, it will shadow some PEIMs twice and occupy more
> memory and waste more boot time. it is unnecessary, so the only valid
> choice is to enable PcdMigrateTemporaryRamFirmwareVolumes and disable
> PcdShadowPeimOnBoot.
You could add similar clarification in dec where the new PCD is defined.
>
> Signed-off-by: Guomin Jiang
> Cc: Jian J Wang
> Cc: Hao A Wu
> Cc: Dandan Bi
> Cc: Liming Gao
> Cc: Debkumar De
> Cc: Harry Han
> Cc: Catharine West
> ---
> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 11 +--
> MdeModulePkg/Core/Pei/Image/Image.c | 14 ++
> MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 18 +-
> 3 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 210b5b22f727..74cd5387c158 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1408,7 +1408,11 @@ PeiDispatcher (
>PeimFileHandle = NULL;
>EntryPoint = 0;
>
> - if ((Private->PeiMemoryInstalled) && (Private-
> >HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME ||
> PcdGetBool (PcdShadowPeimOnS3Boot))) {
> + if ((Private->PeiMemoryInstalled) &&
> + (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
> + || (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME)
> + || PcdGetBool (PcdShadowPeimOnS3Boot))
> +) {
> //
> // Once real memory is available, shadow the RegisterForShadow modules.
> And meanwhile
> // update the modules' status from PEIM_STATE_REGISTER_FOR_SHADOW
> to PEIM_STATE_DONE.
> @@ -1607,7 +1611,10 @@ PeiDispatcher (
> PeiCheckAndSwitchStack (SecCoreData, Private);
>
> if ((Private->PeiMemoryInstalled) && (Private-
> >Fv[FvCount].PeimState[PeimCount] == PEIM_STATE_REGISTER_FOR_SHADOW)
> && \
> -(Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot))) {
> +(PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
> + ||
> + (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME)
> + || PcdGetBool (PcdShadowPeimOnS3Boot))
> + ) {
>//
>// If memory is available we shadow images by default
> for performance reasons.
>// We call the entry point a 2nd time so the module knows it's
> shadowed.
Below condition also needs to consider new PCD
PcdMigrateTemporaryRamFirmwareVolumes.
> diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
> b/MdeModulePkg/Core/Pei/Image/Image.c
> index 0caeb63e26b4..85d1a84e4b67 100644
> --- a/MdeModulePkg/Core/Pei/Image/Image.c
> +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> @@ -328,8 +328,11 @@ LoadAndRelocatePeCoffImage (
>//
>// When Image has no reloc section, it can't be relocated into memory.
>//
> - if (ImageContext.RelocationsStripped &&
> (Private->PeiMemoryInstalled) &&
> ((!IsPeiModule) ||
> - (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
> IsRegisterForShadow)) || (IsS3Boot && PcdGetBool
> (PcdShadowPeimOnS3Boot {
&g
Re: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098)
Guomin,
See my inline comments below.
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Guomin
> Jiang
> Sent: Monday, July 20, 2020 7:30 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J ; Wu, Hao A ;
> Bi, Dandan ; Gao, Liming ; De,
> Debkumar ; Han, Harry ;
> West, Catharine
> Subject: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant
> shadow when enable the Migrated PCD (CVE-2019-11098)
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
>
> When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it will shadow the
> PEIMs, when it is disabled, PEIMs marked REGISTER_FOR_SHADOW will be
> shadowed as well and it is controled by PcdShadowPeimOnBoot and
> PcdShadowPeimOnS3Boot.
>
> To cover the shadow behavior, the change will always shadow PEIMs when
> enable PcdMigrateTemporaryRamFirmwareVolumes.
>
> When PcdMigrateTemporaryRamFirmwareVolumes is true, if enable
> PcdShadowPeiOnBoot, it will shadow some PEIMs twice and occupy more
> memory and waste more boot time. it is unnecessary, so the only valid
> choice is to enable PcdMigrateTemporaryRamFirmwareVolumes and disable
> PcdShadowPeimOnBoot.
You could add similar clarification in dec where the new PCD is defined.
>
> Signed-off-by: Guomin Jiang
> Cc: Jian J Wang
> Cc: Hao A Wu
> Cc: Dandan Bi
> Cc: Liming Gao
> Cc: Debkumar De
> Cc: Harry Han
> Cc: Catharine West
> ---
> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 11 +--
> MdeModulePkg/Core/Pei/Image/Image.c | 14 ++
> MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 18 +-
> 3 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 210b5b22f727..74cd5387c158 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1408,7 +1408,11 @@ PeiDispatcher (
>PeimFileHandle = NULL;
>EntryPoint = 0;
>
> - if ((Private->PeiMemoryInstalled) && (Private-
> >HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME ||
> PcdGetBool (PcdShadowPeimOnS3Boot))) {
> + if ((Private->PeiMemoryInstalled) &&
> + (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
> + || (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME)
> + || PcdGetBool (PcdShadowPeimOnS3Boot))
> +) {
> //
> // Once real memory is available, shadow the RegisterForShadow modules.
> And meanwhile
> // update the modules' status from PEIM_STATE_REGISTER_FOR_SHADOW to
> PEIM_STATE_DONE.
> @@ -1607,7 +1611,10 @@ PeiDispatcher (
> PeiCheckAndSwitchStack (SecCoreData, Private);
>
> if ((Private->PeiMemoryInstalled) && (Private-
> >Fv[FvCount].PeimState[PeimCount] == PEIM_STATE_REGISTER_FOR_SHADOW)
> && \
> -(Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot))) {
> +(PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
> + || (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME)
> + || PcdGetBool (PcdShadowPeimOnS3Boot))
> + ) {
>//
>// If memory is available we shadow images by default for
> performance
> reasons.
>// We call the entry point a 2nd time so the module knows it's
> shadowed.
> diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
> b/MdeModulePkg/Core/Pei/Image/Image.c
> index 0caeb63e26b4..85d1a84e4b67 100644
> --- a/MdeModulePkg/Core/Pei/Image/Image.c
> +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> @@ -328,8 +328,11 @@ LoadAndRelocatePeCoffImage (
>//
>// When Image has no reloc section, it can't be relocated into memory.
>//
> - if (ImageContext.RelocationsStripped && (Private->PeiMemoryInstalled) &&
> ((!IsPeiModule) ||
> - (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
> IsRegisterForShadow)) || (IsS3Boot && PcdGetBool
> (PcdShadowPeimOnS3Boot {
> + if (ImageContext.RelocationsStripped && (Private->PeiMemoryInstalled)
> + && ((!IsPeiModule) || PcdGetBool
> (PcdMigrateTemporaryRamFirmwareVolumes)
> + || (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
> IsRegisterForShadow))
> + || (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot)))
> + ) {
> DEBUG ((EFI_D_INFO|EFI_D_LOAD, "The image at 0x%08x without reloc
>
[edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098)
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it will shadow the
PEIMs, when it is disabled, PEIMs marked REGISTER_FOR_SHADOW will be
shadowed as well and it is controled by PcdShadowPeimOnBoot and
PcdShadowPeimOnS3Boot.
To cover the shadow behavior, the change will always shadow PEIMs when
enable PcdMigrateTemporaryRamFirmwareVolumes.
When PcdMigrateTemporaryRamFirmwareVolumes is true, if enable
PcdShadowPeiOnBoot, it will shadow some PEIMs twice and occupy more
memory and waste more boot time. it is unnecessary, so the only valid
choice is to enable PcdMigrateTemporaryRamFirmwareVolumes and disable
PcdShadowPeimOnBoot.
Signed-off-by: Guomin Jiang
Cc: Jian J Wang
Cc: Hao A Wu
Cc: Dandan Bi
Cc: Liming Gao
Cc: Debkumar De
Cc: Harry Han
Cc: Catharine West
---
MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 11 +--
MdeModulePkg/Core/Pei/Image/Image.c | 14 ++
MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 18 +-
3 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index 210b5b22f727..74cd5387c158 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -1408,7 +1408,11 @@ PeiDispatcher (
PeimFileHandle = NULL;
EntryPoint = 0;
- if ((Private->PeiMemoryInstalled) &&
(Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME ||
PcdGetBool (PcdShadowPeimOnS3Boot))) {
+ if ((Private->PeiMemoryInstalled) &&
+ (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
+ || (Private->HobList.HandoffInformationTable->BootMode !=
BOOT_ON_S3_RESUME)
+ || PcdGetBool (PcdShadowPeimOnS3Boot))
+) {
//
// Once real memory is available, shadow the RegisterForShadow modules.
And meanwhile
// update the modules' status from PEIM_STATE_REGISTER_FOR_SHADOW to
PEIM_STATE_DONE.
@@ -1607,7 +1611,10 @@ PeiDispatcher (
PeiCheckAndSwitchStack (SecCoreData, Private);
if ((Private->PeiMemoryInstalled) &&
(Private->Fv[FvCount].PeimState[PeimCount] == PEIM_STATE_REGISTER_FOR_SHADOW)
&& \
-(Private->HobList.HandoffInformationTable->BootMode !=
BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot))) {
+(PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
+ || (Private->HobList.HandoffInformationTable->BootMode !=
BOOT_ON_S3_RESUME)
+ || PcdGetBool (PcdShadowPeimOnS3Boot))
+ ) {
//
// If memory is available we shadow images by default for
performance reasons.
// We call the entry point a 2nd time so the module knows it's
shadowed.
diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
b/MdeModulePkg/Core/Pei/Image/Image.c
index 0caeb63e26b4..85d1a84e4b67 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -328,8 +328,11 @@ LoadAndRelocatePeCoffImage (
//
// When Image has no reloc section, it can't be relocated into memory.
//
- if (ImageContext.RelocationsStripped && (Private->PeiMemoryInstalled) &&
((!IsPeiModule) ||
- (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) || IsRegisterForShadow))
|| (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot {
+ if (ImageContext.RelocationsStripped && (Private->PeiMemoryInstalled)
+ && ((!IsPeiModule) || PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
+ || (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
IsRegisterForShadow))
+ || (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot)))
+ ) {
DEBUG ((EFI_D_INFO|EFI_D_LOAD, "The image at 0x%08x without reloc section
can't be loaded into memory\n", (UINTN) Pe32Data));
}
@@ -343,8 +346,11 @@ LoadAndRelocatePeCoffImage (
// On normal boot, PcdShadowPeimOnBoot decides whether load PEIM or PeiCore
into memory.
// On S3 boot, PcdShadowPeimOnS3Boot decides whether load PEIM or PeiCore
into memory.
//
- if ((!ImageContext.RelocationsStripped) && (Private->PeiMemoryInstalled) &&
((!IsPeiModule) ||
- (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) || IsRegisterForShadow))
|| (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot {
+ if ((!ImageContext.RelocationsStripped) && (Private->PeiMemoryInstalled)
+ && ((!IsPeiModule) || PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)
+ || (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) ||
IsRegisterForShadow))
+ || (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot)))
+ ) {
//
// Allocate more buffer to avoid buffer overflow.
//
diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
index 48605eeada86..ce8649f954a8 100644
--- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
+++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
@@ -322,7 +322,8 @@ PeiCore
