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.11281.1581709683818153536 for ; Fri, 14 Feb 2020 11:48:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dyGiUDbr; 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=1581709683; 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=2Vl9ga9Dj7WtB4U8LLgrFzulBmoY3qCsoRcIB7RHl7Y=; b=dyGiUDbrmXS61OC1m4k/NdMdDz0K+Ng8Md+Ha78IvTCJOnKRhpl9gMedybTOTK8D6JvIPq z0QmLq/N983AbjqiAGxEfs5x5fwPOF9XA70B8Pn5m7zWlAzzsECYQRS3LNrr2cfFiysSmr Br0elzdDrqrG7q+2iUp3fE3ybiPYaQA= 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-220-_h-Gh4_fOwmQ8sukySrqMg-1; Fri, 14 Feb 2020 14:47:58 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BC2F113EA; Fri, 14 Feb 2020 19:47:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-153.ams2.redhat.com [10.36.116.153]) by smtp.corp.redhat.com (Postfix) with ESMTP id B5B2F1001DD8; Fri, 14 Feb 2020 19:47:53 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 3/3] Ovmf: enable TPM 1.2 support To: devel@edk2.groups.io, marcandre.lureau@redhat.com, edk2-devel@lists.01.org Cc: stefanb@linux.ibm.com, simon.hardy@itdev.co.uk References: <20200213131222.157700-1-marcandre.lureau@redhat.com> <20200213131222.157700-4-marcandre.lureau@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 14 Feb 2020 20:47:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200213131222.157700-4-marcandre.lureau@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-MC-Unique: _h-Gh4_fOwmQ8sukySrqMg-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Marc-Andr=C3=A9, On 02/13/20 14:12, marcandre.lureau@redhat.com wrote: > From: Marc-Andr=C3=A9 Lureau >=20 > Enable TcgPei & TcgDxe modules to initialize a TPM 1.2 device and > measure boot environment. >=20 > Tpm12RequestUseTpm() returns success on any TPM interface, including > FIFO & CRB which are TPM 2.0. Check the actual interface with > Tpm12GetPtpInterfaceType(), and only detect 1.2 if it's a TIS. >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > OvmfPkg/OvmfPkgIa32.dsc | 15 +++++++++++++++ > OvmfPkg/OvmfPkgIa32.fdf | 2 ++ > OvmfPkg/OvmfPkgIa32X64.dsc | 15 +++++++++++++++ > OvmfPkg/OvmfPkgIa32X64.fdf | 2 ++ > OvmfPkg/OvmfPkgX64.dsc | 15 +++++++++++++++ > OvmfPkg/OvmfPkgX64.fdf | 2 ++ > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +++ > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 17 ++++++++++++++++- > 8 files changed, 70 insertions(+), 1 deletion(-) >=20 > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 38b013ad9543..02300886563e 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -206,6 +206,7 @@ > XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf >=20 > =20 >=20 > !if $(TPM_ENABLE) =3D=3D TRUE >=20 > + Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.in= f >=20 > Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf >=20 > Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/Dx= eTcg2PhysicalPresenceLib.inf >=20 > Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLi= bNull.inf >=20 > @@ -281,6 +282,7 @@ > =20 >=20 > !if $(TPM_ENABLE) =3D=3D TRUE >=20 > BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf >=20 > + Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDT= pm.inf >=20 > Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.= inf >=20 > !endif >=20 > =20 >=20 OK, these reflect commit [1] 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone", 2018-03-09). > @@ -361,6 +363,7 @@ > MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >=20 > QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.i= nf >=20 > !if $(TPM_ENABLE) =3D=3D TRUE >=20 > + Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg= .inf >=20 > Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.= inf >=20 > !endif >=20 > =20 >=20 This reflects commit [3] 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09). > @@ -633,6 +636,7 @@ > =20 >=20 > !if $(TPM_ENABLE) =3D=3D TRUE >=20 > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >=20 > + SecurityPkg/Tcg/TcgPei/TcgPei.inf >=20 > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { >=20 > >=20 > HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCry= ptoRouterPei.inf >=20 Mirrors commit [2] 4672a4892867 ("OvmfPkg: include Tcg2Pei module", 2018-03-09). > @@ -668,6 +672,7 @@ > NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificat= ionLib.inf >=20 > !endif >=20 > !if $(TPM_ENABLE) =3D=3D TRUE >=20 > + NULL|SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib= .inf >=20 > NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootL= ib.inf >=20 > !endif >=20 > } >=20 Mirrors commit [4] d5a002aba0aa ("OvmfPkg: plug DxeTpm2MeasureBootLib into SecurityStubDxe", 2018-03-09) > @@ -926,5 +931,15 @@ > } >=20 > !if $(TPM_CONFIG_ENABLE) =3D=3D TRUE >=20 > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf >=20 > +!endif >=20 > + SecurityPkg/Tcg/TcgDxe/TcgDxe.inf { >=20 > + >=20 > + Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceL= ibDTpm.inf >=20 > + } Again reflects commit [3] 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09). >=20 > +!if $(TPM_CONFIG_ENABLE) =3D=3D TRUE >=20 > + SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDxe.inf { >=20 > + >=20 > + PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >=20 > + } >=20 > !endif >=20 > !endif >=20 This matches commit [5] 3103389043bd ("OvmfPkg: Add TCG2 Configuration menu to the Device Manager menu", 2019-02-11). ... Which was later cleaned up by commit cf3ad972a210 ("OvmfPkg: reorganize TPM2 support in DSC/FDF files", 2020-01-09). > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index 2c7d6cccdfb0..b0ddc5a4ae73 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -161,6 +161,7 @@ INF UefiCpuPkg/CpuMpPei/CpuMpPei.inf > =20 >=20 > !if $(TPM_ENABLE) =3D=3D TRUE >=20 > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >=20 > +INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >=20 > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >=20 > !endif >=20 > =20 >=20 Mirrors commit [2] 4672a4892867 ("OvmfPkg: include Tcg2Pei module", 2018-03-09). > @@ -347,6 +348,7 @@ INF MdeModulePkg/Universal/Variable/RuntimeDxe/Varia= bleRuntimeDxe.inf > # TPM support >=20 > # >=20 > !if $(TPM_ENABLE) =3D=3D TRUE >=20 > +INF SecurityPkg/Tcg/TcgDxe/TcgDxe.inf >=20 > INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf >=20 > !if $(TPM_CONFIG_ENABLE) =3D=3D TRUE >=20 > INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf >=20 Again reflects commit [3] 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09). So, my requests thus far: (1) Please split this part of the patch into five separate patches, in parallel to commits [1] through [5]. The messages on the new patches need not be very long, they should basically repeat the original subject lines, customized for TPM-1.2, and refer to the TPM-2 commit that they mirror. (2) Where you add TcgDxe and TcgConfigDxe to the DSC file, I'd prefer if we didn't duplicate the TPM_CONFIG_ENABLE condition. Can you please add TcgDxe just above Tcg2Dxe, and TcgConfigDxe just above Tcg2ConfigDxe? Because, this would be consistent with the rest of the DSC file updates, as you (nicely) add the TPM-1.2 artifacts just above the TPM-2.0 ones. (3) In the FDF file, you forgot to add "SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDxe.inf" (paralleling commit [5] 3103389043bd ("OvmfPkg: Add TCG2 Configuration menu to the Device Manager menu", 2019-02-11)). [...] > diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2C= onfig/Tcg2ConfigPei.inf > index e34cd6210611..15f9b7cda099 100644 > --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > @@ -31,17 +31,20 @@ > PeimEntryPoint >=20 > DebugLib >=20 > PeiServicesLib >=20 > + Tpm12DeviceLib >=20 > Tpm2DeviceLib >=20 > =20 >=20 > [Guids] >=20 > gEfiTpmDeviceSelectedGuid ## PRODUCES ## GUID # Used as a PP= I GUID >=20 > gEfiTpmDeviceInstanceTpm20DtpmGuid ## SOMETIMES_CONSUMES >=20 > + gEfiTpmDeviceInstanceTpm12Guid ## SOMETIMES_CONSUMES >=20 > =20 >=20 > [Ppis] >=20 > gPeiTpmInitializationDonePpiGuid ## SOMETIMES_PRODUCES >=20 > =20 >=20 > [Pcd] >=20 > gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PR= ODUCES >=20 > + gEfiSecurityPkgTokenSpaceGuid.PcdActiveTpmInterfaceType ## SO= METIMES_CONSUMES (4) This shouldn't be necessary. The PCD is not referenced in this patch anywhere else. >=20 > =20 >=20 > [Depex] >=20 > TRUE >=20 > diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c b/OvmfPkg/Tcg/Tcg2Co= nfig/Tcg2ConfigPeim.c > index 99d571d9fa6d..ae3d4fc2c380 100644 > --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c > +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c > @@ -18,6 +18,7 @@ > #include >=20 > #include >=20 > #include >=20 > +#include >=20 > #include >=20 > =20 >=20 > STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmSelectedPpi =3D { >=20 > @@ -50,6 +51,19 @@ Tcg2ConfigPeimEntryPoint ( > =20 >=20 > DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__)); >=20 > =20 >=20 > + Status =3D Tpm12RequestUseTpm (); >=20 > + if (!EFI_ERROR (Status) && Tpm12GetPtpInterfaceType () =3D=3D PtpInter= faceTis) { >=20 > + DEBUG ((DEBUG_INFO, "%a: TPM1.2 detected\n", __FUNCTION__)); >=20 > + Size =3D sizeof (gEfiTpmDeviceInstanceTpm12Guid); >=20 > + Status =3D PcdSetPtrS ( >=20 > + PcdTpmInstanceGuid, >=20 > + &Size, >=20 > + &gEfiTpmDeviceInstanceTpm12Guid >=20 > + ); (5) The indentation is not correct; it should be two spaces to the right of the start of the word "PcdSetPtrS". (6) IIUC, we shouldn't use the Tpm12GetPtpInterfaceType() function here, per Jiewen's comment. (Sorry, I can't comment on patch#2.) >=20 > + ASSERT_EFI_ERROR (Status); >=20 > + goto done; (7) Use of "goto" is generally restricted to error handling; please use "else" here. (Independently, the label should start with a capital letter.) >=20 > + } >=20 > + >=20 > Status =3D Tpm2RequestUseTpm (); >=20 > if (!EFI_ERROR (Status)) { >=20 > DEBUG ((DEBUG_INFO, "%a: TPM2 detected\n", __FUNCTION__)); >=20 > @@ -61,7 +75,7 @@ Tcg2ConfigPeimEntryPoint ( > ); >=20 > ASSERT_EFI_ERROR (Status); >=20 > } else { >=20 > - DEBUG ((DEBUG_INFO, "%a: no TPM2 detected\n", __FUNCTION__)); >=20 > + DEBUG ((DEBUG_INFO, "%a: no TPM detected\n", __FUNCTION__)); >=20 > // >=20 > // If no TPM2 was detected, we still need to install >=20 > // TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon se= eing >=20 > @@ -73,6 +87,7 @@ Tcg2ConfigPeimEntryPoint ( > ASSERT_EFI_ERROR (Status); >=20 > } >=20 > =20 >=20 > +done: >=20 > // >=20 > // Selection done >=20 > // >=20 Thanks! Laszlo