From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web11.13652.1595402850325116517 for ; Wed, 22 Jul 2020 00:27:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=SjWEEdqa; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: liming.gao@intel.com) IronPort-SDR: 205YZqaSTLSOf2t0D+KZQr/dEww79QaOQSZFwGkgxQ/I7DT6j2OmwVFGx7rLzuJy85iP0OaY9q gWD7SzvXwPkg== X-IronPort-AV: E=McAfee;i="6000,8403,9689"; a="130366661" X-IronPort-AV: E=Sophos;i="5.75,381,1589266800"; d="scan'208";a="130366661" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jul 2020 00:27:29 -0700 IronPort-SDR: nqXY/zC9Zhdbl4sZXHesNCwjBHm6mnQOUld20LAsTQHvBds8wew7vdNZI4xPJMaR0+bBo3d2dh zSKl2TCoWHdQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,381,1589266800"; d="scan'208";a="432287140" Received: from orsmsx602.amr.corp.intel.com ([10.22.229.15]) by orsmga004.jf.intel.com with ESMTP; 22 Jul 2020 00:27:29 -0700 Received: from orsmsx602.amr.corp.intel.com (10.22.229.15) by ORSMSX602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Wed, 22 Jul 2020 00:27:28 -0700 Received: from ORSEDG002.ED.cps.intel.com (10.7.248.5) by orsmsx602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Wed, 22 Jul 2020 00:27:28 -0700 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (104.47.58.172) by edgegateway.intel.com (134.134.137.101) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Jul 2020 00:27:27 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BQlHcc5MkjIII8rzcJLtaTNEMJqBNIjTcnxwq1zfXYGTAHZfwwkVHx5huQEkvOY+GTu9RwixVhKtnlQaCbzMu/wQBK+M6jTVd9mdCKfveQxM5xlwhjSZWssylSqo6eX5Em/lxlShT+ztiS/X6w3BXA2QAzlG+/dQym+uC7ggsbEM8qhtCJOlHgRS/OxXppdVd3RWAyqgecaigkXHUocRgX8B+wI8eNMI7jXmuhVFFHNLTPDggpTrM/ymwT1dARLZ4EIwfdMXAGe4HydK3HpmUGh5nM1lWEi+yVvIDogWWyOzMBlXN5iphk0CqhzOYc+wSYQLtX8SlW1+6ORpRabxTQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=m/ieM8PvWek6STVvRuM2R6AzeQSqk8C+4MX1tM1TL3Y=; b=YrwwXUJJ9oCWtXq52E+qBRIbSB0ElirKzEv2g0HG3KpTfoVVOm5cLGDABP9i9x65ZKQCs5ecLEpF+bY94sMZGDB0IXUDRSPXEXKUbNQeKvwfi/kIYkPUaZpcKUt/KGStt3/65SvacJ7MpGbf4tEoIdFkAiqxk8SbuMvdGWmqxSkKBu8IL7WxIaqlvxMfDqDEjjaf0lJigpm0+cMtgRCUSacBwvjxvBKwIorqHDgBz1bQx/95EK1hO+n/ez/Ul/EhITtWPzb1kYSJpHxy0jz7yT/hurYd9kPjYSQ0+VXVmNTGHPKhqC6+snrnYezNpHsXlS0nOmQah2hu0v2gGPryRQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=m/ieM8PvWek6STVvRuM2R6AzeQSqk8C+4MX1tM1TL3Y=; b=SjWEEdqatFgpo0kkGtmR/Ip+vURQYqHk55demllKPpVcbj8iL31zdbiG2FCXwRY2/GGhmyQsoOzwoClJGYExCCbzO5buElNZdvS1AQcUKFzcnM/nPqernPP7wQfPzp1ZvHun4HCsl/69T/I6eWjmb9mq++66AZvHd6DcDNkUDKE= Received: from MWHPR11MB1630.namprd11.prod.outlook.com (2603:10b6:301:e::7) by MW3PR11MB4698.namprd11.prod.outlook.com (2603:10b6:303:5a::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3195.17; Wed, 22 Jul 2020 07:27:26 +0000 Received: from MWHPR11MB1630.namprd11.prod.outlook.com ([fe80::7847:b564:1b55:b67e]) by MWHPR11MB1630.namprd11.prod.outlook.com ([fe80::7847:b564:1b55:b67e%6]) with mapi id 15.20.3216.020; Wed, 22 Jul 2020 07:27:26 +0000 From: "Liming Gao" To: "Wang, Jian J" , "devel@edk2.groups.io" , "Jiang, Guomin" CC: "Wu, Hao A" , "Bi, Dandan" , "De, Debkumar" , "Han, Harry" , "West, Catharine" Subject: Re: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098) Thread-Topic: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redundant shadow when enable the Migrated PCD (CVE-2019-11098) Thread-Index: AQHWXolJjFfH8344o0+7oik59z0c1KkTLggAgAAGtPA= Date: Wed, 22 Jul 2020 07:27:25 +0000 Message-ID: References: <20200720113022.675-1-guomin.jiang@intel.com> <20200720113022.675-11-guomin.jiang@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-version: 11.2.0.6 dlp-product: dlpe-windows dlp-reaction: no-action authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.198.147.217] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 8c323b20-4c1e-42ee-5fd9-08d82e10aeeb x-ms-traffictypediagnostic: MW3PR11MB4698: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: k8eb/xO9L6TWvHcZC2BU1GBaovb7hC6kjtjrgJaAuHuE4IN0b0nHh61jYCinEdoNvTMeg0+S/VxWUqXiMaWqOQXY+3T3cITbSqo64It2EGcbTn5EgwS6ZhQCtv51wrNVOBLNxEsqYXERRiNq+p8ehcEJr9NRo5sTczZpn06yzn1pviA+dwn6Kqa93tdyb29JcviNZuyiliLj+uhO8lHVsZN13UnfvJgh2+qI+T9/JkKUuAuV55HiEryCoqmJK8cGd6C/Tb2szfm/x8wgkWNW1j0Q/9YC1wZXnRY17NSFOh+71CBRHfv77x+R2qhLhyge2objaV+kCoygEJ19OwsEYXpK17onwuRfQ/iCUMbthT9uX8CXDdYfnbWp3F5tfxToFd1kFDbiwcd/mRBMSNiGxA== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MWHPR11MB1630.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(366004)(39860400002)(376002)(136003)(346002)(396003)(7696005)(186003)(86362001)(8676002)(966005)(71200400001)(478600001)(8936002)(66946007)(66476007)(66556008)(64756008)(76116006)(4326008)(6636002)(66446008)(316002)(9686003)(55016002)(107886003)(26005)(2906002)(53546011)(6506007)(54906003)(110136005)(5660300002)(52536014)(83380400001)(19627235002)(33656002);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: SK7/ZYaarom75cYhTQqYUSleSvIemeDhdxaVXS499yKhkEd6BT/VzfgL3en1TpNRHKj58soDUQ4uRikyetHfkOFJY1T0BhP/V5bKZrB9fQalRANf12ju2BgS8tCDP1eVQm0S7wSFtGZP3xr3qd5szd1wsnaar+ufgUGjAoCMOpemUBUNIvKvRZ4jbS2xDO5Lqt42Gw+wlmx/Sr8JgYynIsRhwmh6WNtlGkhAUD/UDs79I2OsnOa/OOuHmBvwj7iS/0kgq9u+vWaQk/t+9QbYVXPPUi4Bq7Q9WIkadgobWCx7jtk0z5HZJ+VGBi+I1BZXTKFhRcELEVlLwLxCUM8IsnGCE6hmiejzyluAvRFibWY+oqlBmAZDVQl0FvGGwPiSom04zwVAtrG3TK3LPpNlp6/3GLAE3KJiHI6kIj38q343cBmuiSZFQSJ/ppTazyops5/cPTcpOjdNxpuvOVZKexs6s20HND3hy5L2QSUwcs8WFFVlBM6hH1J8ebGfk3DO MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB1630.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8c323b20-4c1e-42ee-5fd9-08d82e10aeeb X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jul 2020 07:27:25.9801 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ZuVB6VgLJGE8zhNjZelxxfpWPqf6snIvhk/WqCqMqEneGSChGIfbWrOb7WGkv6kYbfamJRbNhcL3yGPHbHUfbQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4698 Return-Path: liming.gao@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable Guomin: I add my comments.=20 -----Original Message----- From: Wang, Jian J =20 Sent: 2020=1B$BG/=1B(B7=1B$B7n=1B(B22=1B$BF|=1B(B 14:59 To: devel@edk2.groups.io; Jiang, Guomin Cc: Wu, Hao A ; Bi, Dandan ; Gao,= Liming ; De, Debkumar ; Han, = Harry ; West, Catharine Subject: RE: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid redund= ant shadow when enable the Migrated PCD (CVE-2019-11098) Guomin, See my inline comments below. > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Guomin=20 > Jiang > Sent: Monday, July 20, 2020 7:30 PM > To: devel@edk2.groups.io > Cc: Wang, Jian J ; Wu, Hao A=20 > ; Bi, Dandan ; Gao, Liming=20 > ; De, Debkumar ; Han,=20 > Harry ; West, Catharine=20 > > Subject: [edk2-devel] [PATCH v6 10/10] MdeModulePkg/Core: Avoid=20 > redundant shadow when enable the Migrated PCD (CVE-2019-11098) >=20 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D1614 >=20 > When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it will shadow the= =20 > PEIMs, when it is disabled, PEIMs marked REGISTER_FOR_SHADOW will be=20 > shadowed as well and it is controled by PcdShadowPeimOnBoot and=20 > PcdShadowPeimOnS3Boot. >=20 > To cover the shadow behavior, the change will always shadow PEIMs when= =20 > enable PcdMigrateTemporaryRamFirmwareVolumes. >=20 > When PcdMigrateTemporaryRamFirmwareVolumes is true, if enable=20 > PcdShadowPeiOnBoot, it will shadow some PEIMs twice and occupy more=20 > memory and waste more boot time. it is unnecessary, so the only valid=20 > choice is to enable PcdMigrateTemporaryRamFirmwareVolumes and disable=20 > PcdShadowPeimOnBoot. You could add similar clarification in dec where the new PCD is defined. >=20 > 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(-) >=20 > 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 =3D NULL; > EntryPoint =3D 0; >=20 > - if ((Private->PeiMemoryInstalled) && (Private- > >HobList.HandoffInformationTable->BootMode !=3D BOOT_ON_S3_RESUME || > PcdGetBool (PcdShadowPeimOnS3Boot))) { > + if ((Private->PeiMemoryInstalled) && > + (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) > + || (Private->HobList.HandoffInformationTable->BootMode !=3D > BOOT_ON_S3_RESUME) > + || PcdGetBool (PcdShadowPeimOnS3Boot)) > + ) { > // > // Once real memory is available, shadow the RegisterForShadow modu= les. > And meanwhile > // update the modules' status from PEIM_STATE_REGISTER_FOR_SHADOW= =20 > to PEIM_STATE_DONE. > @@ -1607,7 +1611,10 @@ PeiDispatcher ( > PeiCheckAndSwitchStack (SecCoreData, Private); >=20 > if ((Private->PeiMemoryInstalled) && (Private- > >Fv[FvCount].PeimState[PeimCount] =3D=3D PEIM_STATE_REGISTER_FOR_SHADOW) > && \ > - (Private->HobList.HandoffInformationTable->BootMode != =3D > BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot))) { > + (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) > + ||=20 > + (Private->HobList.HandoffInformationTable->BootMode !=3D > BOOT_ON_S3_RESUME) > + || PcdGetBool (PcdShadowPeimOnS3Boot)) > + ) { > // > // If memory is available we shadow images by default=20 > for performance reasons. > // We call the entry point a 2nd time so the module knows= it's shadowed. Below condition also needs to consider new PCD PcdMigrateTemporaryRamFirmw= areVolumes.=20 > 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 &&=20 > (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= =20 > section can't be loaded into memory\n", (UINTN) Pe32Data)); > } >=20 PcdMigrateTemporaryRamFirmwareVolumes is not required here, because FV has= been shadow.=20 PEIM is not required to shadow again.=20 > @@ -343,8 +346,11 @@ LoadAndRelocatePeCoffImage ( > // On normal boot, PcdShadowPeimOnBoot decides whether load PEIM or= =20 > PeiCore into memory. > // On S3 boot, PcdShadowPeimOnS3Boot decides whether load PEIM or=20 > PeiCore into memory. > // > - if ((!ImageContext.RelocationsStripped) &&=20 > (Private->PeiMemoryInstalled) && > ((!IsPeiModule) || > - (!IsS3Boot && (PcdGetBool (PcdShadowPeimOnBoot) || > IsRegisterForShadow)) || (IsS3Boot && PcdGetBool > (PcdShadowPeimOnS3Boot)))) { > + if ((!ImageContext.RelocationsStripped) && (Private->PeiMemoryInstall= ed) > + && ((!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 =3D (PEICORE_FUNCTION_POINTER)=20 > (UINTN) PeiCore; > - if ((HandoffInformationTable->BootMode =3D=3D BOOT_ON_S3_RESUME &= & > PcdGetBool (PcdShadowPeimOnS3Boot)) > + if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) > + || (HandoffInformationTable->BootMode =3D=3D BOOT_ON_S3_RESUM= E=20 > + && > PcdGetBool (PcdShadowPeimOnS3Boot)) > || (HandoffInformationTable->BootMode !=3D BOOT_ON_S3_RESUME= =20 > && PcdGetBool (PcdShadowPeimOnBoot))) { > OldCoreData->ShadowedPeiCore =3D ShadowPeiCore (OldCoreData); > } > @@ -422,10 +423,17 @@ PeiCore ( > } > } else { > if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) { > - if (PrivateData.HobList.HandoffInformationTable->BootMode =3D=3D > BOOT_ON_S3_RESUME) { > - TempRamEvacuation =3D PcdGetBool (PcdShadowPeimOnS3Boot); > - } else { > - TempRamEvacuation =3D PcdGetBool (PcdShadowPeimOnBoot); > + TempRamEvacuation =3D TRUE; > + > + // > + // When PcdMigrateTemporaryRamFirmwareVolumes is TRUE, it make > sense only make -> makes > + // when PcdShadowPeimOnBoot is FALSE. in this situation, all=20 > + PEIMs will be > copied in -> In > + // and shadowed, and doesn't need shadow PEIMs again, it will=20 > + occupy > more > + // memory and waste more time if you enable it. > + // > + if (PcdGetBool (PcdShadowPeimOnBoot)) { > + DEBUG ((DEBUG_ERROR, "!!!IMPORTANT NOTICE!!!\nWhen you see=20 > + the > message, it mean that you enable the > PcdMigrateTemporaryRamFirmwareVolumes and PcdShadowPeimOnBoot at the=20 > same time\nIt make no sense because it will occupy more memory and=20 > waste more time.\nYou must disable PcdShadowPeimOnBoot when you enable= =20 > PcdMigrateTemporaryRamFirmwareVolumes for performance reason.\n\n")); Too long string literal. You can split it into more string literals separa= ted by space. For example, DEBUG (( DEBUG_INFO, "String part 1" "String part 2" "String part 3" )); > + ASSERT ((PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)=20 > + =3D=3D > TRUE) && (PcdGetBool (PcdShadowPeimOnBoot) =3D=3D FALSE)); > } > } >=20 ASSERT should also handle PcdShadowPeimOnS3Boot. Thanks Liming > -- > 2.25.1.windows.1 >=20 >=20 >=20