From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::236; helo=mail-wm0-x236.google.com; envelope-from=marcandre.lureau@gmail.com; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x236.google.com (mail-wm0-x236.google.com [IPv6:2a00:1450:400c:c09::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A6431224E692C for ; Thu, 1 Mar 2018 06:53:35 -0800 (PST) Received: by mail-wm0-x236.google.com with SMTP id z81so12436891wmb.4 for ; Thu, 01 Mar 2018 06:59:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=kq7FCbPTKujME1pPh1UxAp0l/FSVI+rM03mxbIrFjRw=; b=mSk3LaQP6fSfcYRYWr31+Oxnm9ciTP7qn/wuBqgXZRF0/7LwXhyO4gnq9ggYe9XLZp VpdQI/oB03VC7EwrBNAwWi0WHQo9JMrRBhig3zIU7nVDlo2NZhYcSVlB/6wQRpngn2CO BexHz91Rq3XjKLkJ4JfN9kkXUbIpWDv9bZLJJHqYC2fRdP7C0TMZzV0SXYtxyuMh0a6p WEWqsagulCsH6oW80/MaEyGu56mY0Tngy8shsyoigQoyCoKtqMqMt3x+M3VuXClxaXJT ysfpqTHljnTVRuizVpIy8EygrRk+mtrj7gNBAmBHl+KMvNHBvBE0iHb6GnU/lEmQ3pnY N1sA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=kq7FCbPTKujME1pPh1UxAp0l/FSVI+rM03mxbIrFjRw=; b=nMYHvtiwk7s7sDJuRR/E/Ue7MxTO7mA+Fc8/AOx4mFJRLtPI9BeqcEKvrNPSg6Ttfo EHsHl/ceXpMUyU2vz/bkWoPzcOeXXnNwzyVv14nPIJRx+OApxKNczumYxuMX02ZVA9KC Ua50hzniWulPHGOfS5tXuv08oyPHYbNweVIjusLO2p+X12ND8S3OlI4FLfZY+wmCdjv3 BWpXSJjjRxqcc7q4wCAPhFEOXicvZiivUeoE4mNjV2gA5bW7YSewEc7ERZQQEYTAssvJ /yvUDw9R+hfA5+kHQOeQCbJ+cjl8CAX4Zt0wQYO04bsjrMPANFMsBNH4fxJB3WH4jCES 0c+g== X-Gm-Message-State: APf1xPD/bmPhbtzeB5PxPIohy9XNndNWwBJTnplga+0dD5IgtWFVrkpl QGKJ6A5pemuBUfB+31dB35fsaBxdv1TuBLs91KdmkQYm X-Google-Smtp-Source: AG47ELtnG9hPhpsEiDQrbFoNBZRAZPrvYWC8a5gC7sTgf9EOE5pIeep11uWW+Nvz9VUnmgmKA9YzGymWRFb5oYLun4Y= X-Received: by 10.28.138.6 with SMTP id m6mr2187938wmd.146.1519916382525; Thu, 01 Mar 2018 06:59:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.223.185.67 with HTTP; Thu, 1 Mar 2018 06:59:41 -0800 (PST) In-Reply-To: <080742f0-0f3c-bf8e-83d7-e473417cc172@redhat.com> References: <20180223132311.26555-1-marcandre.lureau@redhat.com> <20180223132311.26555-3-marcandre.lureau@redhat.com> <080742f0-0f3c-bf8e-83d7-e473417cc172@redhat.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Thu, 1 Mar 2018 15:59:41 +0100 Message-ID: To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Peter Jones , Jiewen Yao , QEMU , Javier Martinez Canillas 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: Thu, 01 Mar 2018 14:53:36 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Fri, Feb 23, 2018 at 6:31 PM, Laszlo Ersek wrote: > On 02/23/18 14:23, marcandre.lureau@redhat.com wrote: >> From: Marc-Andr=C3=A9 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." > ok > Becase... > >> The module requires VariablePei, which is built with >> MEM_VARSTORE_EMU_ENABLE=3DFALSE. > > (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. > ok > > (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.) ok > > (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 exis= ts. > > 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= . ok > > (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.) > ok > > (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? I don't get how you came to that conclusion, both SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c and SecurityPkg/Tcg/Tcg2Config/TpmDetection.c match. Apparently, the variable is used in s3 mode, in DetectTpmDevice(). I'll drop it from OvmfPkg version for now. > > >> CC: Laszlo Ersek >> CC: Stefan Berger >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Marc-Andr=C3=A9 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. I'd rather keep this as a TODO item for now, since we are not close to a final version, and it's annoying to have to fix each files etc.. > >> 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-gi= t-guide-for-edk2-contributors-and-maintainers#contrib-05 > > (in particular "xfuncname") > > and > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-gi= t-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. > done >> DEFINE SMM_REQUIRE =3D FALSE >> DEFINE TLS_ENABLE =3D FALSE >> DEFINE MEM_VARSTORE_EMU_ENABLE =3D TRUE >> + DEFINE TPM2_ENABLE =3D FALSE >> >> # >> # Flash size selection. Setting FD_SIZE_IN_KB on the command line dir= ectly to >> @@ -209,6 +210,11 @@ >> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTree= Lib/BaseOrderedCollectionRedBlackTreeLib.inf >> XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf >> >> +!if $(TPM2_ENABLE) =3D=3D TRUE >> + Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.i= nf >> + 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. > iirc, I tried removing it and it failed to link. Anyway, next version will use tpm2 only. >> [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/Tpm12DeviceLibD= Tpm.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? > I followed SecurityPkg dsc, they use Tpm2DeviceLibRouterDxe.inf and Tpm2DeviceLibTcg2.inf for some reason I don't quite understand yet. >> >> [LibraryClasses.common.DXE_CORE] >> HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf >> @@ -558,6 +568,12 @@ >> >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x0= 0 >> >> +!if $(TPM2_ENABLE) =3D=3D TRUE >> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0= x8b, 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). ok > >> + 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. > ok >> + >> #######################################################################= ######### >> # >> # Components Section - list of all EDK II Modules needed by this Platfo= rm. >> @@ -629,6 +645,10 @@ >> >> MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf >> >> +!if $(TPM2_ENABLE) =3D=3D 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 > # > done >> !if $(SECURE_BOOT_ENABLE) =3D=3D 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) =3D=3D 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 > done > directly. > > Thanks! > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel --=20 Marc-Andr=C3=A9 Lureau