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.6361.1582713444053062799 for ; Wed, 26 Feb 2020 02:37:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GzhbAl2V; 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=1582713443; 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=yWa8soz1U0WLQQ/Q3sT9NGST0nJbrxeA9JnxHoKmPlk=; b=GzhbAl2VFBeydjNUwuhI7ahp3YwNmirwh3YEtkshPEBc3xSS9feuVCCC8m0xYBCVGp0pcI P9vlLLie22Mlbt+t1od0A3t+OvWdY71ebfM9QZjLO++TQkrxampkM+GcvML8fal/+Xj1yK JqNc12JYDHAujsleaiSrW2J1Tt+bC00= 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-8-1A263tazP-W6OB1X21y0Aw-1; Wed, 26 Feb 2020 05:37:16 -0500 X-MC-Unique: 1A263tazP-W6OB1X21y0Aw-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 5770513E4; Wed, 26 Feb 2020 10:37:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-185.ams2.redhat.com [10.36.116.185]) by smtp.corp.redhat.com (Postfix) with ESMTP id A118A5C54A; Wed, 26 Feb 2020 10:37:11 +0000 (UTC) Subject: Re: [PATCH v3 2/6] OvmfPkg: detect TPM 1.2 in Tcg2ConfigPei To: marcandre.lureau@redhat.com, devel@edk2.groups.io Cc: simon.hardy@itdev.co.uk, stefanb@linux.ibm.com References: <20200226093459.1131530-1-marcandre.lureau@redhat.com> <20200226093459.1131530-3-marcandre.lureau@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 26 Feb 2020 11:37:10 +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: <20200226093459.1131530-3-marcandre.lureau@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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 On 02/26/20 10:34, marcandre.lureau@redhat.com wrote: > From: Marc-Andr=C3=A9 Lureau >=20 > Complement commit 6cf1880fb5b ("OvmfPkg: add customized Tcg2ConfigPei > clone", 2018-03-09) by detecting TPM 1.2 devices. >=20 > Since Tpm12RequestUseTpm() returns success on any TPM interface, > (including FIFO & CRB which are TPM 2.0), try to send a GetTicks TPM > 1.2 command to probe the version. In case of failure, fallback on TPM > 2.0 path. >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > OvmfPkg/OvmfPkgIa32.dsc | 2 + > OvmfPkg/OvmfPkgIa32X64.dsc | 2 + > OvmfPkg/OvmfPkgX64.dsc | 2 + > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 + > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 81 +++++++++++++++++++----- > 5 files changed, 75 insertions(+), 15 deletions(-) >=20 > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 38b013ad9543..293e95a2ae81 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -206,6 +206,7 @@ > XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf > =20 > !if $(TPM_ENABLE) =3D=3D TRUE > + Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.in= f > Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf > Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/Dx= eTcg2PhysicalPresenceLib.inf > Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLi= bNull.inf > @@ -281,6 +282,7 @@ > =20 > !if $(TPM_ENABLE) =3D=3D TRUE > BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > + Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDT= pm.inf > Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.= inf > !endif > =20 > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index e075f0766935..5cfa3fc849fe 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -211,6 +211,7 @@ > XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf > =20 > !if $(TPM_ENABLE) =3D=3D TRUE > + Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.in= f > Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf > Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/Dx= eTcg2PhysicalPresenceLib.inf > Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLi= bNull.inf > @@ -286,6 +287,7 @@ > =20 > !if $(TPM_ENABLE) =3D=3D TRUE > BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > + Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDT= pm.inf > Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.= inf > !endif > =20 > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 3b1ebf123b51..78481a62e021 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -211,6 +211,7 @@ > XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf > =20 > !if $(TPM_ENABLE) =3D=3D TRUE > + Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.in= f > Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf > Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/Dx= eTcg2PhysicalPresenceLib.inf > Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLi= bNull.inf > @@ -286,6 +287,7 @@ > =20 > !if $(TPM_ENABLE) =3D=3D TRUE > BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > + Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDT= pm.inf > Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.= inf > !endif > =20 > diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2C= onfig/Tcg2ConfigPei.inf > index e34cd6210611..f380b86b5d89 100644 > --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > @@ -31,11 +31,14 @@ > PeimEntryPoint > DebugLib > PeiServicesLib > + Tpm12CommandLib > + Tpm12DeviceLib > Tpm2DeviceLib > =20 > [Guids] > gEfiTpmDeviceSelectedGuid ## PRODUCES ## GUID # Used as a PP= I GUID > gEfiTpmDeviceInstanceTpm20DtpmGuid ## SOMETIMES_CONSUMES > + gEfiTpmDeviceInstanceTpm12Guid ## SOMETIMES_CONSUMES > =20 > [Ppis] > gPeiTpmInitializationDonePpiGuid ## SOMETIMES_PRODUCES > diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c b/OvmfPkg/Tcg/Tcg2Co= nfig/Tcg2ConfigPeim.c > index 99d571d9fa6d..770229ed9912 100644 > --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c > +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c > @@ -18,6 +18,8 @@ > #include > #include > #include > +#include > +#include > #include > =20 > STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmSelectedPpi =3D { > @@ -32,6 +34,43 @@ STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmInitializatio= nDonePpiList =3D { > NULL > }; > =20 > +#pragma pack(1) > + > +typedef struct { > + TPM_RSP_COMMAND_HDR Hdr; > + TPM_CURRENT_TICKS CurrentTicks; > +} TPM_RSP_GET_TICKS; > + > +#pragma pack() (1) Please insert a space character before the opening parens. > + > +/** > + Probe for the TPM for 1.2 version, by sending TPM1.2 GetTicks > + > + Sending a TPM1.2 command to a TPM2 should return a TPM1.2 > + header (tag =3D 0xc4) and error code (TPM_BADTAG =3D 0x1e) > +**/ > +static EFI_STATUS (2) Please break EFI_STATUS to a separate line. (I'm not asking for replacing "static" with "STATIC" because of -- so that's fine.) > +TestTpm12 ( > + ) > +{ > + EFI_STATUS Status; > + TPM_RQU_COMMAND_HDR Command; > + TPM_RSP_GET_TICKS Response; > + UINT32 Length; > + > + Command.tag =3D SwapBytes16 (TPM_TAG_RQU_COMMAND); > + Command.paramSize =3D SwapBytes32 (sizeof (Command)); > + Command.ordinal =3D SwapBytes32 (TPM_ORD_GetTicks); > + > + Length =3D sizeof (Response); > + Status =3D Tpm12SubmitCommand (sizeof (Command), (UINT8 *)&Command, &L= ength, (UINT8 *)&Response); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + return EFI_SUCCESS; > +} > + > /** > The entry point for Tcg2 configuration driver. > =20 > @@ -50,27 +89,39 @@ Tcg2ConfigPeimEntryPoint ( > =20 > DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__)); > =20 > - Status =3D Tpm2RequestUseTpm (); > - if (!EFI_ERROR (Status)) { > - DEBUG ((DEBUG_INFO, "%a: TPM2 detected\n", __FUNCTION__)); > - Size =3D sizeof (gEfiTpmDeviceInstanceTpm20DtpmGuid); > + Status =3D Tpm12RequestUseTpm (); > + if (!EFI_ERROR (Status) && TestTpm12 () =3D=3D EFI_SUCCESS) { (3) Please write EFI_ERROR (TestTpm12 ()); or else break the TestTpm12 () invocation out to a separate assignment to Status, and then check Status again. > + DEBUG ((DEBUG_INFO, "%a: TPM1.2 detected\n", __FUNCTION__)); > + Size =3D sizeof (gEfiTpmDeviceInstanceTpm12Guid); > Status =3D PcdSetPtrS ( > PcdTpmInstanceGuid, > &Size, > - &gEfiTpmDeviceInstanceTpm20DtpmGuid > + &gEfiTpmDeviceInstanceTpm12Guid > ); > ASSERT_EFI_ERROR (Status); > } else { > - DEBUG ((DEBUG_INFO, "%a: no TPM2 detected\n", __FUNCTION__)); > - // > - // If no TPM2 was detected, we still need to install > - // TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon se= eing > - // the default (all-bits-zero) contents of PcdTpmInstanceGuid, thus = we have > - // to install the PPI in its place, in order to unblock any dependen= t > - // PEIMs. > - // > - Status =3D PeiServicesInstallPpi (&mTpmInitializationDonePpiList); > - ASSERT_EFI_ERROR (Status); > + Status =3D Tpm2RequestUseTpm (); > + if (!EFI_ERROR (Status)) { > + DEBUG ((DEBUG_INFO, "%a: TPM2 detected\n", __FUNCTION__)); > + Size =3D sizeof (gEfiTpmDeviceInstanceTpm20DtpmGuid); > + Status =3D PcdSetPtrS ( > + PcdTpmInstanceGuid, > + &Size, > + &gEfiTpmDeviceInstanceTpm20DtpmGuid > + ); > + ASSERT_EFI_ERROR (Status); > + } else { > + DEBUG ((DEBUG_INFO, "%a: no TPM detected\n", __FUNCTION__)); > + // > + // If no TPM2 was detected, we still need to install > + // TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon = seeing > + // the default (all-bits-zero) contents of PcdTpmInstanceGuid, thu= s we have > + // to install the PPI in its place, in order to unblock any depend= ent > + // PEIMs. > + // > + Status =3D PeiServicesInstallPpi (&mTpmInitializationDonePpiList); > + ASSERT_EFI_ERROR (Status); > + } > } > =20 > // >=20 Very happy about this version. With (1) through (3) fixed: Reviewed-by: Laszlo Ersek Thanks! Laszlo