From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0DBED2243695B for ; Fri, 23 Feb 2018 09:25:35 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 85B994007873; Fri, 23 Feb 2018 17:31:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-112.rdu2.redhat.com [10.10.120.112]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1EF3D213AEED; Fri, 23 Feb 2018 17:31:34 +0000 (UTC) To: marcandre.lureau@redhat.com, edk2-devel@lists.01.org Cc: qemu-devel@nongnu.org, javierm@redhat.com, pjones@redhat.com, jiewen.yao@intel.com References: <20180223132311.26555-1-marcandre.lureau@redhat.com> <20180223132311.26555-3-marcandre.lureau@redhat.com> From: Laszlo Ersek Message-ID: <080742f0-0f3c-bf8e-83d7-e473417cc172@redhat.com> Date: Fri, 23 Feb 2018 18:31:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180223132311.26555-3-marcandre.lureau@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Fri, 23 Feb 2018 17:31:36 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Fri, 23 Feb 2018 17:31:36 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 2/7] ovmf: link with Tcg2ConfigPei module X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Feb 2018 17:25:36 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 02/23/18 14:23, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau > > This module initializes TPM device type based on variable and > detection. (1) I suggest we say the following here: "The Tcg2ConfigPei module informs the firmware globally about the TPM device type, by setting the PcdTpmInstanceGuid PCD to the appropriate GUID value. The original module under SecurityPkg can perform device detection, or read a cached value from a non-volatile UEFI variable. OvmfPkg's clone of the module always performs the hardware detection." Becase... > The module requires VariablePei, which is built with > MEM_VARSTORE_EMU_ENABLE=FALSE. (2) ... as I hinted in my response to your blurb, and also as suggested by "Tcg2ConfigPei.inf", we should clone Tcg2ConfigPei for OVMF, and *trim it* quite a bit. - The new location should be "OvmfPkg/Tcg/Tcg2Config/". - We need not copy the ".uni" file (also drop MODULE_UNI_FILE from the INF file) - Re-generate FILE_GUID in the INF file with "uuidgen" - Remove all PEI-phase variable access; always perform the hw detection. - I would even suggest removing support for TPM1.2. Just check whether TPM2 is available or not. (3) Ultimately, this is what the module should do: - Check the QEMU hardware for TPM2 availability only - If found, set the dynamic PCD "PcdTpmInstanceGuid" to &gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of the firmware about the TPM type. - Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD. In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting and the consumption of the "TPM type" PCD. - If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid. (Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if no TPM2 is available. So in that case our Tcg2ConfigPei must do it.) (4) Regarding the TPM detection itself. It looks like DetectTpmDevice() [SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c] calls a number of TPM1.2 functions. If the earliest one fails, it assumes "no TPM" at all, but if only a later call fails, it deduces, from the 1.2 failure, that TPM2 exists. I think we can do better than this, in our Tcg2ConfigPei clone: - We should call Tpm2RequestUseTpm() directly, from "SecurityPkg/Include/Library/Tpm2DeviceLib.h". - And, Tpm2Startup(), from "SecurityPkg/Include/Library/Tpm2CommandLib.h", will be called by Tcg2Pei. (5) Finally, there's no need to set "PcdTpmInitializationPolicy" to anything. I don't see it consumed by any module that we should include in OVMF. (More on this below.) (6) Now, I realize Tcg2Pei *apparently* depends on gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the PEI phase) as well. That's a bug in the INF file (the [depex] section). If you grep the Tcg2Pei module source for the GUID, the [depex] section is the only hit. Can you please submit a separate patch that removes it from the depex? > CC: Laszlo Ersek > CC: Stefan Berger > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Marc-André Lureau > --- > OvmfPkg/OvmfPkgX64.dsc | 20 ++++++++++++++++++++ > OvmfPkg/OvmfPkgX64.fdf | 3 +++ > 2 files changed, 23 insertions(+) Is there any particular reason to exclude the Ia32 and Ia32X64 builds? If not, then please modify all three sets of dsc/fdf files identically. > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 32c57b04e1..b5cbe8430f 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -40,6 +40,7 @@ (7) Please implement the following git settings in your edk2 clone: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05 (in particular "xfuncname") and https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09 This will help reviewers see what section of the DSC / FDF / INI / DEC files are modified by a patch hunk. > DEFINE SMM_REQUIRE = FALSE > DEFINE TLS_ENABLE = FALSE > DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE > + DEFINE TPM2_ENABLE = FALSE > > # > # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to > @@ -209,6 +210,11 @@ > OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf > > +!if $(TPM2_ENABLE) == TRUE > + Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf > + Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf > +!endif > + (8) For the patch, as posted, resolving Tpm2CommandLib looks unneeded, because "SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" doesn't seem to depend on that lib class. However, for the OvmfPkg clone of Tcg2ConfigPei that I'm suggesting here, *only* the Tpm2CommandLib resolution will be necessary. > [LibraryClasses.common] > BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > > @@ -272,6 +278,10 @@ > QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf > PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf > +!if $(TPM2_ENABLE) > + Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf > +!endif (9) Again, only the Tpm2DeviceLib resolution should be necessary (for our clone). (10) Furthermore, is there any particular reason you add this resolution only for PEIMs? Are you going to add different resolutions later, for different module types? > > [LibraryClasses.common.DXE_CORE] > HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf > @@ -558,6 +568,12 @@ > > gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 > > +!if $(TPM2_ENABLE) == TRUE > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0x8b, 0x19, 0x4f, 0xe8, 0x46, 0xab, 0x93, 0x1c, 0x53, 0x67, 0x1b, 0x90, 0xcc} (11) This is wrong, IMO. The value you set here is "gEfiTpmDeviceInstanceTpm12Guid", which I can't explain. In order for the PCD to behave dynamically, we should indeed provide a dynamic default. But that default value should be all-bits-zero (put differently, "gEfiTpmDeviceInstanceNoneGuid"). The actual value (based on hardware detection) should be set by our Tcg2ConfigPei clone (see near the top). > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1 > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1 > +!endif (12) There is no need to set PcdTpmInitializationPolicy, it should not be used (either set or read) by any module we include in OVMF. (13) There is also no need to set PcdTpm2InitializationPolicy. While we *will* include Tcg2Pei, that module only consumes the PCD, so actual dynamic behavior is not needed. Furthermore, the top-level default in "SecurityPkg/SecurityPkg.dsc" is already 1, so we can simply inherit that. It should cause Tcg2Pei to perform a full init on the TPM2 chip. > + > ################################################################################ > # > # Components Section - list of all EDK II Modules needed by this Platform. > @@ -629,6 +645,10 @@ > > MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf > > +!if $(TPM2_ENABLE) == TRUE > + SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +!endif > + (14) Please move this addition higher up in the DSC file, so that it lands at the end of: # # PEI Phase modules # just above # # DXE Phase modules # > !if $(SECURE_BOOT_ENABLE) == TRUE > MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf { > > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index bb46a409d9..dc35d0a1f7 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -168,6 +168,9 @@ INF UefiCpuPkg/CpuMpPei/CpuMpPei.inf > INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf > INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf > !endif > +!if $(TPM2_ENABLE) == TRUE > +INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +!endif > > ################################################################################ > > (15) This seems correct, yes; with the remark that once you drop the dependence on PEI phase variable access, the addition should follow INF UefiCpuPkg/CpuMpPei/CpuMpPei.inf directly. Thanks! Laszlo