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.web12.15391.1594227838969057986 for ; Wed, 08 Jul 2020 10:03:59 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=inRW/aL8; 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=1594227838; 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=EERaqomWa38AF+UkA3Plb0b3Zh/tiA5NNPpOMnA4ZPY=; b=inRW/aL8SWyg2vkbOfKa9Tzr714EDp2qI9XQyU2Yfynckss+z/LqfwJ8tFzXxj4I9ulLgI o70fu0CL6a7izcFDFoOOiixvymkpFCNBpA6yTRfNvsUh/sBUD7aE6GS3gRkWepgAr/i6+Y FGwpifMdkR7aDh66FRLqGzcYm2TUK1Y= 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-90-Xu5c8t5kPd-xQ1so2q3llA-1; Wed, 08 Jul 2020 13:03:49 -0400 X-MC-Unique: Xu5c8t5kPd-xQ1so2q3llA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8A4FE802782; Wed, 8 Jul 2020 17:03:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-73.ams2.redhat.com [10.36.112.73]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7029E5C1B2; Wed, 8 Jul 2020 17:03:05 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 1/9] MdeModulePkg: Add new PCD to control the evacuate temporary memory feature (CVE-2019-11098) To: devel@edk2.groups.io, guomin.jiang@intel.com Cc: Jian J Wang , Hao A Wu References: <20200708081059.691-1-guomin.jiang@intel.com> <20200708081059.691-2-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: <7c84d2c5-6cb0-f6d0-4ed4-a203122ce1cd@redhat.com> Date: Wed, 8 Jul 2020 19:03:04 +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: <20200708081059.691-2-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com 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/08/20 10:10, Guomin Jiang wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614 > > The security researcher found that we can get control after NEM disable. > > The reason is that the flash content reside in NEM at startup and the > code will get the content from flash directly after disable NEM. > > To avoid this vulnerability, the feature will copy the PEIMs from > temporary memory to permanent memory and only execute the code in > permanent memory. > > The vulnerability is exist in physical platform and haven't report in > virtual platform, so the virtual can disable the feature currently. > > Cc: Jian J Wang > Cc: Hao A Wu > Signed-off-by: Guomin Jiang > --- > MdeModulePkg/MdeModulePkg.dec | 7 +++++++ > MdeModulePkg/MdeModulePkg.uni | 6 ++++++ > 2 files changed, 13 insertions(+) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index 843e963ad34b..16db17d0a873 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1220,6 +1220,13 @@ [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 > + # 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. > + gEfiMdeModulePkgTokenSpaceGuid.PcdMigrateTemporaryRamFirmwareVolumes|FALSE|BOOLEAN|0x3000102A > + > ## The mask is used to control memory profile behavior.

> # BIT0 - Enable UEFI memory profile.
> # BIT1 - Enable SMRAM profile.
> diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni > index 2007e0596c4f..5235dee561ad 100644 > --- a/MdeModulePkg/MdeModulePkg.uni > +++ b/MdeModulePkg/MdeModulePkg.uni > @@ -214,6 +214,12 @@ > "TRUE - Shadow PEIM on S3 boot path after memory is ready.
\n" > "FALSE - Not shadow PEIM on S3 boot path after memory is ready.
" > > +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMigrateTemporaryRamFirmwareVolumes_HELP #language en-US "Enable the feature that evacuate temporary memory to permanent memory or not.

\n" > + "It will allocate page to save the temporary PEIMs resided in NEM(or CAR) to the permanent memory and change all pointers pointed to the NEM(or CAR) to permanent memory.

\n" > + "After then, there are no pointer pointed to NEM(or CAR) and TOCTOU volnerability can be avoid.

\n" > + > +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMigrateTemporaryRamFirmwareVolumes_PROMPT #language en-US "Enable the feature that evacuate temporary memory to permanent memory or not" > + > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdAcpiDefaultOemId_PROMPT #language en-US "Default OEM ID for ACPI table creation" > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdAcpiDefaultOemId_HELP #language en-US "Default OEM ID for ACPI table creation, its length must be 0x6 bytes to follow ACPI specification." > I think the commit message and the documentation could be improved, but I'll leave those to the MdeModulePkg owners. I'm not sure whether it's the best idea to set the DEC default of the new PCD to FALSE. In the v2 review, I suggested a TRUE default value, and explicitly setting it to FALSE for virtualized / emulated platforms, in platform DSC files. I think that would be the safest option for physical platforms. However, I assume you must have discussed the TRUE default's impact internally at Intel, and deliberately chose to go with the FALSE default. So I'm fine with that; it will certainly do the right thing for virtual platforms. From that perspective: Acked-by: Laszlo Ersek If you update the documentation on this patch (DEC comments, UNI file, commit message, etc), then please *keep* my Acked-by on this patch, in future versions. Thanks! Laszlo