From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com []) by mx.groups.io with SMTP id smtpd.web12.35600.1595244629521419095 for ; Mon, 20 Jul 2020 04:30:51 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=fail (domain: intel.com, ip: , mailfrom: guomin.jiang@intel.com) IronPort-SDR: IigbWWavEelamxY02kQCzuyAAuEfvhBF1PMT7FxF5nLbYhBOgdFHf3VH9Aufldq/rSRhHFishK bQSd+fTHtm5g== X-IronPort-AV: E=McAfee;i="6000,8403,9687"; a="137373239" X-IronPort-AV: E=Sophos;i="5.75,374,1589266800"; d="scan'208";a="137373239" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jul 2020 04:30:50 -0700 IronPort-SDR: qWwJO7eMMbVpSDegjb7nBduPm3MfKiYeWKM+tABplStRHLrtNc2hJKAeJVxgARb+Y0qeLSTKrW avOgHI//p5TQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,374,1589266800"; d="scan'208";a="271411329" Received: from guominji-mobl.ccr.corp.intel.com ([10.238.13.140]) by fmsmga008.fm.intel.com with ESMTP; 20 Jul 2020 04:30:47 -0700 From: "Guomin Jiang" To: devel@edk2.groups.io Cc: Jian J Wang , Hao A Wu , Dandan Bi , Liming Gao , Debkumar De , Harry Han , Catharine West Subject: [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098) Date: Mon, 20 Jul 2020 19:30:22 +0800 Message-Id: <20200720113022.675-11-guomin.jiang@intel.com> X-Mailer: git-send-email 2.25.1.windows.1 In-Reply-To: <20200720113022.675-1-guomin.jiang@intel.com> References: <20200720113022.675-1-guomin.jiang@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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 ( // PEI Core and PEIMs to get high performance. // OldCoreData->ShadowedPeiCore = (PEICORE_FUNCTION_POINTER) (UINTN) PeiCore; - if ((HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME && PcdGetBool (PcdShadowPeimOnS3Boot)) + if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) + || (HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME && PcdGetBool (PcdShadowPeimOnS3Boot)) || (HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME && PcdGetBool (PcdShadowPeimOnBoot))) { OldCoreData->ShadowedPeiCore = ShadowPeiCore (OldCoreData); } @@ -422,10 +423,17 @@ PeiCore ( } } else { if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) { - if (PrivateData.HobList.HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME) { - TempRamEvacuation = PcdGetBool (PcdShadowPeimOnS3Boot); - } else { - TempRamEvacuation = PcdGetBool (PcdShadowPeimOnBoot); + TempRamEvacuation = TRUE; + + // + // When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it make sense only + // when PcdShadowPeimOnBoot is FALSE. in this situation, all PEIMs will be copied + // and shadowed, and doesn't need shadow PEIMs again, it will occupy more + // memory and waste more time if you enable it. + // + if (PcdGetBool (PcdShadowPeimOnBoot)) { + DEBUG ((DEBUG_ERROR, "!!!IMPORTANT NOTICE!!!\nWhen you see the message, it mean that you enable the PcdMigrateTemporaryRamFirmwareVolumes and PcdShadowPeimOnBoot at the same time\nIt make no sense because it will occupy more memory and waste more time.\nYou must disable PcdShadowPeimOnBoot when you enable PcdMigrateTemporaryRamFirmwareVolumes for performance reason.\n\n")); + ASSERT ((PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) == TRUE) && (PcdGetBool (PcdShadowPeimOnBoot) == FALSE)); } } -- 2.25.1.windows.1