From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web09.1627.1582795703335438169 for ; Thu, 27 Feb 2020 01:28:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WSsT9cJc; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582795702; 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=YgJb2oMeZ3zTq304ocYk7ndRaSCzbDi/Y4+tkgppucE=; b=WSsT9cJcd4HR33cM0yts99MMqSWvUZNLSv0Pd6+zotQzid8nIzpnjerQDlU7/k3qV3Thfi uGHu6/eghtok2npAbdFDQ1LMz0W7rmiowuRIbLf8JaVOgg0LsUQ7cIMofvExCCIcJQdEQ5 +cOHTfSLsDfu6fUht9K1ibTtRaDRz8M= 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-305-2CuUSy2SNTuPLxEXDqgqng-1; Thu, 27 Feb 2020 04:28:14 -0500 X-MC-Unique: 2CuUSy2SNTuPLxEXDqgqng-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0623F18A6ECF; Thu, 27 Feb 2020 09:28:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-46.ams2.redhat.com [10.36.116.46]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1C5885D9CD; Thu, 27 Feb 2020 09:28:05 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 7/9] ArmVirtPkg/ArmVirtQemu: enable the DXE phase TPM2 support module To: devel@edk2.groups.io, ard.biesheuvel@linaro.org Cc: eric.auger@redhat.com, philmd@redhat.com, marcandre.lureau@redhat.com, stefanb@linux.ibm.com, leif@nuviainc.com References: <20200226190514.31395-1-ard.biesheuvel@linaro.org> <20200226190514.31395-8-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <4e8484d6-42fa-420f-0716-49bc49b5367e@redhat.com> Date: Thu, 27 Feb 2020 10:28:05 +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: <20200226190514.31395-8-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 02/26/20 20:05, Ard Biesheuvel wrote: > Enable the TPM2 support module in the DXE phase, and the associated > libraries and PCDs that it requires. This will be wired into the > measured boot support code in a subsequent patch. > > Note that Tcg2Dxe.inf is added to ArmVirtQemuFvMain.fdf.inc, which > is shared with other platforms in ArmVirtPkg, but as those will not > set the TPM2_ENABLE define, this change does not affect them. > > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/ArmVirtQemu.dsc | 23 ++++++++++++++++++++ > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 7 ++++++ > 2 files changed, 30 insertions(+) > > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc > index 8950116dacab..9fe5ab37611c 100644 > --- a/ArmVirtPkg/ArmVirtQemu.dsc > +++ b/ArmVirtPkg/ArmVirtQemu.dsc > @@ -77,6 +77,8 @@ [LibraryClasses.common] > > !if $(TPM2_ENABLE) =3D=3D TRUE > Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf > + Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/Dx= eTcg2PhysicalPresenceLib.inf This should not be needed. Or more precisely, not like this. In OvmfPkg, Marc-Andr=E9 added TPM2 PPI support in the following series (which was separately building upon the initial TPM2 enablement): [edk2] [PATCH v3 0/4] RFC: ovmf: Add support for TPM Physical Presence in= terface http://mid.mail-archive.com/20180518122304.31020-1-marcandre.lureau@redha= t.com 1 fe280ecbee11 OvmfPkg: add Tcg2PhysicalPresenceLibNull when !TPM2_ENABL= E 2 909ac65af113 OvmfPkg/IndustryStandard: add QemuTpm.h header 3 b9777bb42e4f OvmfPkg: add Tcg2PhysicalPresenceLibQemu 4 8d65d3b25e35 OvmfPkg/PlatformBootManagerLib: process TPM PPI request The above lib class resolution mirrors commit#3 from those. But it only really makes sense if, in the end, we intend to add a Tcg2PhysicalPresenceLibProcessRequest() call to the PlatformBootManagerLib instance (see commit#4). The present ArmVirtQemu patch generally mirrors OVMF commit 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09), from the initial TPM2 enablement work. In that commit, the Tcg2PhysicalPresenceLib class was resolved to the SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib= .inf instance only. (And then later it was udpated by commit#3, from the TPM2 PPI series.) Now... I guess it does make sense to mirror OVMF's lib class resolution here, even if we don't port the rest of the PPI stuff to ArmVirtQemu at this time; such as commit#1 and commit #4. (Note: I do agree we shouldn't port the PPI stuff now.) (1) So ultimtely my request is to add the following to the commit message: -------- This patch ports (parts of) the following OvmfPkg commits to ArmVirtQemu: - 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09) - b9777bb42e4f ("OvmfPkg: add Tcg2PhysicalPresenceLibQemu", 2018-05-22) -- only to match OVMF's current lib class resolutions -------- > + TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasu= rementLib.inf (2) I agree this is necessary, but it should come in differently. Please see OvmfPkg commit 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib if and only if TPM2_ENABLE", 2019-07-04). The gist is that, going forward, the TpmMeasurementLib resolution should depend on TPM2_ENABLE only, in ArmVirtQemu as well. But currently we have a SECURE_BOOT_ENABLE-dependent resolution in "ArmVirt.dsc.inc". Therefore, (2a) Please add another patch, before this one, replacing the current TpmMeasurementLib resolution in "ArmVirt.dsc.inc" with an *unconditional* resolution to TpmMeasurementLibNull. Please mention commit 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib if and only if TPM2_ENABLE", 2019-07-04) in the commit message. (2b) Please add yet another patch, to move / triplicate that (unconditional) TpmMeasurementLib Null resolution from the "ArmVirt.dsc.inc" file, to the individual DSC files, (2c) and in the current patch, please conditionalize the TpmMeasurementLib resolution, solely based on TPM2_ENABLE, (2d) also in the current patch, please also mention the commit below, in the commit message section that I request above in (1): ------ - 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib if and only if TPM2_ENABLE", 2019-07-04) ------ > !endif > > [LibraryClasses.common.PEIM] > @@ -91,6 +93,10 @@ [LibraryClasses.common.PEIM] > [LibraryClasses.common.DXE_DRIVER] > ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRep= ortStatusCodeLib.inf > > +!if $(TPM2_ENABLE) =3D=3D TRUE > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.= inf > +!endif > + > [LibraryClasses.common.UEFI_DRIVER] > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf > > @@ -469,6 +475,23 @@ [Components.common] > MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf > MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf > > + # > + # TPM2 support > + # > +!if $(TPM2_ENABLE) =3D=3D TRUE > + SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf { > + > + HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCry= ptoRouterDxe.inf > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLi= bRouterDxe.inf > + NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.i= nf > + NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha2= 56.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha3= 84.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha5= 12.inf > + NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf > + } > +!endif > + > # > # ACPI Support > # (3) Looks good; please extend the commit message block (1) with the following references: - b9130c866dc0 ("OvmfPkg: link Sha384 and Sha512 support into Tcg2Pei and Tcg2Dxe", 2018-08-16) - 5d3ef15da7c3 ("OvmfPkg: link SM3 support into Tcg2Pei and Tcg2Dxe", 2019-07-19) ... So ultimately, block (1) should refer to *five* commits: ------------ This patch ports (parts of) the following OvmfPkg commits to ArmVirtQemu: - 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09) - b9777bb42e4f ("OvmfPkg: add Tcg2PhysicalPresenceLibQemu", 2018-05-22) -- only to match OVMF's current lib class resolutions - 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib if and only if TPM2_ENABLE", 2019-07-04) - b9130c866dc0 ("OvmfPkg: link Sha384 and Sha512 support into Tcg2Pei and Tcg2Dxe", 2018-08-16) - 5d3ef15da7c3 ("OvmfPkg: link SM3 support into Tcg2Pei and Tcg2Dxe", 2019-07-19) ------------ > diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQem= uFvMain.fdf.inc > index 31f615a9d0f9..63247fd10058 100644 > --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc > +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc > @@ -182,3 +182,10 @@ [FV.FvMain] > # Ramdisk support > # > INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf > + > + # > + # TPM2 support > + # > +!if $(TPM2_ENABLE) =3D=3D TRUE > + INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf > +!endif > (4) Can you hoist this addition please, so that it follow "UsbMassStorageDxe.inf"? Because that would reflect the DSC listing order more closely. Thanks! Laszlo