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@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.
... 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-