From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.8995.1578418645849984392 for ; Tue, 07 Jan 2020 09:37:26 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=F0mSbV1C; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578418645; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5T0t1vPJBPuZa0M/7QDfLZGRUON7Kb/pPxk6vr+bXzE=; b=F0mSbV1C1dTQIlPG4W5QzVzkflX40myhYLXQXk/OIbgXQP7Cx/031Bb+dSgAqybzTIbEW5 /6741F6hC+wtZMoZTfgsS9XHcV9kpoSQq9rgzl1/SiWbEFdUi+1oHyW3lI2CbO7kQhtFo5 8VI7Ag2fzjMskcA7GYl7vdjvXpIlSGc= 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-54-rDviU0IaNHe9oTF8zXSDAQ-1; Tue, 07 Jan 2020 12:37:21 -0500 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 A960E184B214; Tue, 7 Jan 2020 17:37:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-126.ams2.redhat.com [10.36.117.126]) by smtp.corp.redhat.com (Postfix) with ESMTP id B61E65C1B0; Tue, 7 Jan 2020 17:37:19 +0000 (UTC) Subject: Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot To: Ard Biesheuvel , devel@edk2.groups.io References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-5-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <27a930b2-bbf8-a1d2-075f-6f33ce03b460@redhat.com> Date: Tue, 7 Jan 2020 18:37:18 +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: <20200107094800.4488-5-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: rDviU0IaNHe9oTF8zXSDAQ-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 01/07/20 10:48, Ard Biesheuvel wrote: > Duplicate the TPM2_ENABLE and TPM2_CONFIG_ENABLE build time flags that > already exist in OvmfPkg, and wire them up in the .DSC and .FDF so > that setting those flags produces a ArmVirtQemu build that implements > measured boot using a TPM provided by QEMU and described in the device > tree. > > Note that the TPM2 driver stack relies on a PEI phase being implemented, > so there is no point in enabling this for ArmVirtQemuKernel or ArmVirtXen. > > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/ArmVirtQemu.dsc | 71 ++++++++++++++++++++ > ArmVirtPkg/ArmVirtQemu.fdf | 5 ++ > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++ > 3 files changed, 86 insertions(+) > > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc > index 7ae6702ac1f0..0a37f613ae23 100644 > --- a/ArmVirtPkg/ArmVirtQemu.dsc > +++ b/ArmVirtPkg/ArmVirtQemu.dsc > @@ -29,6 +29,8 @@ [Defines] > # > DEFINE TTY_TERMINAL = FALSE > DEFINE SECURE_BOOT_ENABLE = FALSE > + DEFINE TPM2_ENABLE = FALSE > + DEFINE TPM2_CONFIG_ENABLE = FALSE > > # > # Network definition > @@ -74,12 +76,32 @@ [LibraryClasses.common] > PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf > PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > > +!if $(TPM2_ENABLE) == TRUE > + Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf > + Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf > + Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf > + TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf > +!else > + Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf > + TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf > +!endif > + > [LibraryClasses.common.PEIM] > ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > > +!if $(TPM2_ENABLE) == TRUE > + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > + ResetSystemLib|MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf (1) So, according to my suggestion / question under patch #3, this would be resolved to an ArmVirtPkg specific instance. > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf > +!endif > + > [LibraryClasses.common.DXE_DRIVER] > ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf > > +!if $(TPM2_ENABLE) == TRUE > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf > +!endif > + > [LibraryClasses.common.UEFI_DRIVER] > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf > > @@ -177,6 +199,8 @@ [PcdsFixedAtBuild.common] > > gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|3 > > + gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE) > + (2) This should be a [PcdsFeatureFlag] in the DSC file too (per patch#2 request). > [PcdsFixedAtBuild.AARCH64] > # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point, > # if the entry point version is >= 3.0. AARCH64 OSes cannot assume the > @@ -237,9 +261,26 @@ [PcdsDynamicDefault.common] > gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE > > +!if $(TPM2_ENABLE) == TRUE > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} OK > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0 > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1 > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy|1 > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2ScrtmPolicy|1 > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1 > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1 > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|3 > + gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|3 (3) Why is it necessary to provide dynamic defaults for these? Are we missing something important in OVMF, or are these specific defaults that we like for ArmVirtPkg? In the latter case, I think we should add them with a separate patch, as the commit message here refers to OvmfPkg. > +!endif > + > [PcdsDynamicHii] > gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS > > +!if $(TPM2_ENABLE) == TRUE > + gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS > + gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS > +!endif (4) Same as (3) -- I don't know what these do and why we need them here, unlike in OvmfPkg. If they really belong here (in this patch), can you explain in the commit message? > + > ################################################################################ > # > # Components Section - list of all EDK II Modules needed by this Platform > @@ -295,6 +336,9 @@ [Components.common] > MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf { > > NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf > +!if $(TPM2_ENABLE) == TRUE > + NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf > +!endif > } > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf > OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf > @@ -430,6 +474,33 @@ [Components.common] > MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf > MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf > > +!if $(TPM2_ENABLE) == TRUE > + OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > + > + HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf > + NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf > + } Hrmpf, this is a bit messy. Not this patch, but the original OvmfPkg DSC code. The general idea is to keep the "PEI phase modules" and "DXE phase modules" nicely separated. Unfortunately, that's already broken in the OvmfPkg DSC files, as follows: --------- # # PEI Phase modules # ... !if $(TPM2_ENABLE) == TRUE OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf } !if $(TPM2_CONFIG_ENABLE) == TRUE SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf !endif !endif # # DXE Phase modules # --------- In other words, in OvmfPkg the Tcg2ConfigDxe driver is added under "PEI Phase modules", and not under "DXE Phase modules". Conversely, in the present patch for ArmVirtQemu, "Tcg2ConfigPei.inf" and "Tcg2Pei.inf" would be listed near "USB Support" (DXE phase). So... I hope I won't annoy you too much by asking that we should please fix up both :) (5) Please disentangle the OvmfPkg DSC files: please move "Tcg2ConfigDxe.inf" near "Tcg2Dxe.inf". In a separate patch. :) (6a) In ArmVirtPkg (in this patch), please move "Tcg2ConfigPei.inf" and "Tcg2Pei.inf" under # # PEI Phase modules # (6b) furthermore, for the DXE modules, please add a new header "TPM2 Support": --------------- # # USB Support # ... # # TPM2 Support # !if $(TPM2_ENABLE) == TRUE SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf { ... } !if $(TPM2_CONFIG_ENABLE) == TRUE SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf !endif !endif # # ACPI Support # ... --------------- > +!if $(TPM2_CONFIG_ENABLE) == TRUE > + SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf > +!endif > + SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf { > + > + Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf > + NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf > + HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf > + NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf > + NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf > + } > +!endif > + > # > # ACPI Support > # > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf > index 2c8936a1ae15..d866e62c529b 100644 > --- a/ArmVirtPkg/ArmVirtQemu.fdf > +++ b/ArmVirtPkg/ArmVirtQemu.fdf > @@ -113,6 +113,11 @@ [FV.FVMAIN_COMPACT] > INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf > INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > > +!if $(TPM2_ENABLE) == TRUE > + INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > +!endif > + > FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 { > SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE { > SECTION FV_IMAGE = FVMAIN Looks good. > diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc > index 31f615a9d0f9..d481e4b2b8fb 100644 > --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc > +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc > @@ -182,3 +182,13 @@ [FV.FvMain] > # Ramdisk support > # > INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf > + > + # > + # TPM2 support > + # > +!if $(TPM2_ENABLE) == TRUE > + INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf > +!if $(TPM2_CONFIG_ENABLE) == TRUE > + INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf > +!endif > +!endif > I think this also belongs in "ArmVirtQemu.fdf": "ArmVirtQemuFvMain.fdf.inc" is for sharing between ArmVirtQemu and ArmVirtQemuKernel, and the commit message correctly states that ArmVirtQemuKernel is not a suitable platform for TPM2. ArmVirtQemu-only stuff should go into "ArmVirtQemu.fdf". Now... I realize what I'm requesting would look quite awkward, because "Tcg2Dxe.inf" and "Tcg2ConfigDxe.inf" would have to be listed in "ArmVirtQemu.fdf" right after !include ArmVirtQemuFvMain.fdf.inc and that indeed looks super ugly. We could move the [FV.FvMain] section header from "ArmVirtQemuFvMain.fdf.inc" to both "ArmVirtQemu.fdf" and "ArmVirtQemuKernel.fdf": ---- [FV.FvMain] !include ArmVirtQemuFvMain.fdf.inc ---- and then listing "Tcg2Dxe.inf" and "Tcg2ConfigDxe.inf" in just "ArmVirtQemu.fdf", after the include directive, would not look *that* ugly. (The section header would be visible nearby.) But that's a lot of churn little benefit. So ultimately I'm OK with this too, just please (7) mention in the commit message: "ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc is being modified -- despite ArmVirtQemuKernel being unaffected -- for keeping the contexts of the referring !include directives simple." Thanks! Laszlo