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/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