From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.779.1595458401803469496 for ; Wed, 22 Jul 2020 15:53:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=BrUus2Pq; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1595458401; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Lkjf3BzdFzqFbp6M2Z9VRMXvuWsZlDaUl0/sVfVkW2w=; b=BrUus2PqD1+K9RdIYRvAeFQn+ijeaqlzLJ1b/Mgw/dISdYHm4EwEF6Tq2mc+WGEtORnEPo uJr1LfUOYNFWt2PmqWPlthDYed8jsER3uyJEBI8m+z+q0SyWN0QUNusE2MMfR/2InYPOwT cjCLYskNvwCIr52LU0BAsHm2j8lQrsM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-204-kIkJy0_bOlafnfKpg6iBeA-1; Wed, 22 Jul 2020 18:53:11 -0400 X-MC-Unique: kIkJy0_bOlafnfKpg6iBeA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 370D957; Wed, 22 Jul 2020 22:53:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-129.ams2.redhat.com [10.36.113.129]) by smtp.corp.redhat.com (Postfix) with ESMTP id DFEA960C05; Wed, 22 Jul 2020 22:53:07 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v7 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098) To: devel@edk2.groups.io, guomin.jiang@intel.com Cc: Jian J Wang , Hao A Wu , Dandan Bi , Liming Gao , Debkumar De , Harry Han , Catharine West References: <20200722083657.739-1-guomin.jiang@intel.com> <20200722083657.739-11-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 23 Jul 2020 00:53:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200722083657.739-11-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 07/22/20 10:36, Guomin Jiang wrote: > 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 > PcdShadowPeimOnBoot or PcdShadowPeimOnS3Boot, 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 and PcdShadowPeimOnS3Boot. > > 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/MdeModulePkg.dec | 11 +++++--- > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 14 +++++++--- > MdeModulePkg/Core/Pei/Image/Image.c | 6 ++--- > MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 26 +++++++++++++++---- > 4 files changed, 42 insertions(+), 15 deletions(-) (1) This patch should be split, and the parts coming out of it should be squashed into: - patch#1 ("MdeModulePkg: Add new PCD to control the evacuate temporary memory feature"), - and patch#2 ("MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore"). It's wrong to introduce a feature with a known bug in one patch of a series, and then fix it separately in a later patch in the same series. If the bug is known in the earlier patch, at the time of posting the series, then the earlier patch should be updated to *not* introduce the bug in the first place. Note: when you do the above squashing, you're going to modify both patches #1 and #2 significantly, so you'll have to drop all feedback tags from them (received thus far), and reviewers will have to re-check both of those patches. > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index e0ad9373e62f..5220202b233b 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1223,11 +1223,14 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] > # @Prompt Shadow Peim and PeiCore on boot > gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|TRUE|BOOLEAN|0x30001029 > > - ## Enable the feature that evacuate temporary memory to permanent memory or not > + ## Enable the feature that evacuate temporary memory to permanent memory or not

> # Set FALSE as default, if the developer need this feature to avoid this vulnerability, please > - # enable it in dsc file. > - # TRUE - Evacuate temporary memory, the actions include copy memory, convert PPI pointers and so on. > - # FALSE - Do nothing, for example, no copy memory, no convert PPI pointers and so on. > + # enable it and disable PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot in dsc file at the same time.
> + # The reason is that PcdMigrateTemporaryRamFirmwareVolumes will make all PEIMs be shadowed and > + # it is unnecessary that shadow PEIMs which is controled by PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot > + # again, it will occupy more memory and waste more time if you enable it.
> + # TRUE - Evacuate temporary memory, the actions include copy memory, convert PPI pointers and so on.
> + # FALSE - Do nothing, for example, no copy memory, no convert PPI pointers and so on.
> # @Prompt Evacuate temporary memory to permanent memory > gEfiMdeModulePkgTokenSpaceGuid.PcdMigrateTemporaryRamFirmwareVolumes|FALSE|BOOLEAN|0x3000102A > So yes, this belongs in patch#1. The rest of the updates should go into patch#2. > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > index 667d9273bb91..11ff5e693304 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. (2) The arrangement of the || operators is not idiomatic. It should be: if (Private->PeiMemoryInstalled && (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) || Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot))) { > @@ -1607,13 +1611,17 @@ 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. > // > //PERF_START (PeiServices, L"PEIM", PeimFileHandle, 0); > - if ((Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME) && !PcdGetBool (PcdShadowPeimOnBoot)) { > + if ((Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME) && !PcdGetBool (PcdShadowPeimOnBoot) > + && !PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) { > // > // Load PEIM into Memory for Register for shadow PEIM. > // (3) Same as (2) -- for both modifications above --; the operators should be placed at the ends of the lines. > diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c > index 0caeb63e26b4..f9b570ba1f47 100644 > --- a/MdeModulePkg/Core/Pei/Image/Image.c > +++ b/MdeModulePkg/Core/Pei/Image/Image.c > @@ -299,7 +299,7 @@ LoadAndRelocatePeCoffImage ( > IsRegisterForShadow = FALSE; > if ((Private->CurrentFileHandle == FileHandle) > && (Private->Fv[Private->CurrentPeimFvCount].PeimState[Private->CurrentPeimCount] == PEIM_STATE_REGISTER_FOR_SHADOW)) { > - IsRegisterForShadow = TRUE; > + IsRegisterForShadow = TRUE && !PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes); > } > > // (4) This looks awkward. Consider that (TRUE && Predicate) is just Predicate So please write IsRegisterForShadow = !PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes); > @@ -319,8 +319,7 @@ LoadAndRelocatePeCoffImage ( > // Check whether the file type is PEI module. > // > IsPeiModule = FALSE; > - if (FileInfo.FileType == EFI_FV_FILETYPE_PEI_CORE || > - FileInfo.FileType == EFI_FV_FILETYPE_PEIM || > + if (FileInfo.FileType == EFI_FV_FILETYPE_PEIM || > FileInfo.FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER) { > IsPeiModule = TRUE; > } > @@ -342,6 +341,7 @@ LoadAndRelocatePeCoffImage ( > // Allocate Memory for the image when memory is ready, and image is relocatable. > // On normal boot, PcdShadowPeimOnBoot decides whether load PEIM or PeiCore into memory. > // On S3 boot, PcdShadowPeimOnS3Boot decides whether load PEIM or PeiCore into memory. > + // PeiCore is specificial case, it will separate from IsPeiModule. > // > if ((!ImageContext.RelocationsStripped) && (Private->PeiMemoryInstalled) && ((!IsPeiModule) || > (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) || IsRegisterForShadow)) || (IsS3Boot && PcdGetBool (PcdShadowPeimOnS3Boot)))) { (5) I don't understand this change. The LoadAndRelocatePeCoffImage() function is called also when PcdMigrateTemporaryRamFirmwareVolumes is FALSE; namely from PeiLoadImageLoadImage(). So this change seems unrelated. It's possible that this is a fix that's needed separately, for a pre-existent bug, but then it should get its own dedicated patch. (Apologies if this change has been introduced since v5 because someone requested it -- I must have missed the request.) > diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > index 48605eeada86..3f168bb56893 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); > } (6) Same as (2), the operators should be placed at the ends of the lines. > @@ -422,10 +423,25 @@ 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 makes sense only > + // when both PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot is FALSE. > + // The reason is that PcdMigrateTemporaryRamFirmwareVolumes will make all PEIMs > + // be shadowed and it is unnecessary that shadow PEIMs which is controled by > + // PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot again, it will occupy more > + // memory and waste more time if you enable it. > + // > + if (PcdGetBool (PcdShadowPeimOnBoot) || PcdGetBool (PcdShadowPeimOnS3Boot)) { > + DEBUG (( > + DEBUG_ERROR, > + "!!!IMPORTANT NOTICE!!!\n" > + "When you see the message, it mean that you enable the PcdShadowPeimOnBoot or PcdShadowPeimOnS3Boot when enable PcdMigrateTemporaryRamFirmwareVolumes\n" > + "It make no sense because it will occupy more memory and waste more time.\n" > + "You must disable PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot when enable PcdMigrateTemporaryRamFirmwareVolumes for performance reason.\n\n")); > + ASSERT ((PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) == TRUE) && > + (PcdGetBool (PcdShadowPeimOnBoot) == FALSE) && (PcdGetBool (PcdShadowPeimOnS3Boot) == FALSE)); > } > } > > (7) This logic change obviates the "TempRamEvacuation" variable. After this modification, "TempRamEvacuation" stands exactly for PcdMigrateTemporaryRamFirmwareVolumes, so "TempRamEvacuation" doesn't need to exist. Subsequent to the code introduced above, you have the following context: if (TempRamEvacuation) { DEBUG ((DEBUG_VERBOSE, "PPI lists before temporary RAM evacuation:\n")); DumpPpiList (&PrivateData); // // Migrate installed content from Temporary RAM to Permanent RAM // EvacuateTempRam (&PrivateData, SecCoreData); DEBUG ((DEBUG_VERBOSE, "PPI lists after temporary RAM evacuation:\n")); DumpPpiList (&PrivateData); } Now that you set TempRamEvacuation to constant TRUE, you end up with a check like if (TRUE) { ... EvacuateTempRam (&PrivateData, SecCoreData); ... } So simply remove "TempRamEvacuation" altogether, and *unnest* (fuse) the code -- the EvacuateTempRam() call -- that depends on "TempRamEvacuation", into the "if" statement that checks "PcdMigrateTemporaryRamFirmwareVolumes". (8) The DEBUG message is superfluous. We already have explanation in the DEC file, and in a comment just above the ASSERT(). Plus, when the ASSERT() fails, it will also print the condition that failed -- including the PCD names. So please drop the DEBUG. (9) Explicitly comparing BOOLEANs against FALSE or TRUE is bad practice. PcdGetBool (X) == TRUE should be replaced with PcdGetBool (X) and PcdGetBool (X) == FALSE is simply !PcdGetBool (X). (10) Checking PcdMigrateTemporaryRamFirmwareVolumes in the ASSERT is superfluous: you are already in a code block that depends on PcdMigrateTemporaryRamFirmwareVolumes. See the "if" statement higher up. (11) When you have: ASSERT (Cond1 && Cond2); it is better to write: ASSERT (Cond1); ASSERT (Cond2); Because the behavior is exactly the same, but using separate ASSERT()s gives more specific information when the original condition fails. The failure message will report which *sub-condition* fails. All in all, this is what I propose for fixing points (7) through (11), expressed here as an incremental patch: > diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > index 3f168bb56893..2b399e589d85 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > @@ -175,11 +175,8 @@ PeiCore ( > EFI_PEI_PCI_CFG2_PPI *PciCfg; > EFI_HOB_HANDOFF_INFO_TABLE *HandoffInformationTable; > EFI_PEI_TEMPORARY_RAM_DONE_PPI *TemporaryRamDonePpi; > UINTN Index; > - BOOLEAN TempRamEvacuation; > - > - TempRamEvacuation = FALSE; > > // > // Retrieve context passed into PEI Core > // > @@ -422,31 +419,19 @@ PeiCore ( > ProcessPpiListFromSec ((CONST EFI_PEI_SERVICES **) &PrivateData.Ps, PpiList); > } > } else { > if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) { > - TempRamEvacuation = TRUE; > - > // > // When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it makes sense only > // when both PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot is FALSE. > // The reason is that PcdMigrateTemporaryRamFirmwareVolumes will make all PEIMs > // be shadowed and it is unnecessary that shadow PEIMs which is controled by > // PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot again, it will occupy more > // memory and waste more time if you enable it. > // > - if (PcdGetBool (PcdShadowPeimOnBoot) || PcdGetBool (PcdShadowPeimOnS3Boot)) { > - DEBUG (( > - DEBUG_ERROR, > - "!!!IMPORTANT NOTICE!!!\n" > - "When you see the message, it mean that you enable the PcdShadowPeimOnBoot or PcdShadowPeimOnS3Boot when enable PcdMigrateTemporaryRamFirmwareVolumes\n" > - "It make no sense because it will occupy more memory and waste more time.\n" > - "You must disable PcdShadowPeimOnBoot and PcdShadowPeimOnS3Boot when enable PcdMigrateTemporaryRamFirmwareVolumes for performance reason.\n\n")); > - ASSERT ((PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) == TRUE) && > - (PcdGetBool (PcdShadowPeimOnBoot) == FALSE) && (PcdGetBool (PcdShadowPeimOnS3Boot) == FALSE)); > - } > - } > + ASSERT (!PcdGetBool (PcdShadowPeimOnBoot)); > + ASSERT (!PcdGetBool (PcdShadowPeimOnS3Boot)); > > - if (TempRamEvacuation) { > DEBUG ((DEBUG_VERBOSE, "PPI lists before temporary RAM evacuation:\n")); > DumpPpiList (&PrivateData); > > // Thanks Laszlo