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

2020-07-03 Thread Laszlo Ersek 
On 07/03/20 16:00, Laszlo Ersek wrote:
> On 07/02/20 07:15, Guomin Jiang wrote:
>> From: Jian J Wang 
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
>>
>> Cc: Jian J Wang 
>> Cc: Hao A Wu 
>> Cc: Dandan Bi 
>> Cc: Liming Gao 
>> Signed-off-by: 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62039): https://edk2.groups.io/g/devel/message/62039
Mute This Topic: https://groups.io/mt/75252663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-07-03 Thread Laszlo Ersek 
On 07/02/20 07:15, Guomin Jiang wrote:
> From: Jian J Wang 
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614
> 
> Cc: Jian J Wang 
> Cc: Hao A Wu 
> Cc: Dandan Bi 
> Cc: Liming Gao 
> Signed-off-by: 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62034): https://edk2.groups.io/g/devel/message/62034
Mute This Topic: https://groups.io/mt/75252663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-07-01 Thread Guomin Jiang 
From: Jian J Wang 

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

Cc: Jian J Wang 
Cc: Hao A Wu 
Cc: Dandan Bi 
Cc: Liming Gao 
Signed-off-by: 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61945): https://edk2.groups.io/g/devel/message/61945
Mute This Topic: https://groups.io/mt/75252663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-