From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web11.8400.1578492830700947879 for ; Wed, 08 Jan 2020 06:13:51 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=t1oAZ4ZR; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id c9so3513741wrw.8 for ; Wed, 08 Jan 2020 06:13:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=wZYkS80kJjaRq50U2Oi2f9c7r84IokGH5WPuXGne15E=; b=t1oAZ4ZRFB+ldvwGK5jh4rDXGyxZE2DqYRqCfcexgNw0cNGSgajgNwR7PzOh22PWQ/ uFNHMyHOszmUb4f2m+P3I57vpszkvWjrVPWnyUMUoi2DaelTXoD7aMtTXk+4xq5WCHiU TZEf29Bux+ZdA9FI3ZDD8ROYCV2+VzNUkkfPXJoe+qpXPBXMiiyYVb08CO9fv626n0U1 xxVlCdf2rOyDXx6GF/1ENdAu1SWt1PGQ+0j8vRMGsFiQEnjgkWU0euxASKO37DWeaXPO PYHp7xWgn2V9DGGsA1BcvTJLbZx8bECqQGOxb05ioqecqbxYxZ9qTMWCsApLi0UaAkAA VSkg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wZYkS80kJjaRq50U2Oi2f9c7r84IokGH5WPuXGne15E=; b=Jhh5pGw4kAnUS58nB36LRIQt2jTMrHUh2YesLGTt5iH+I3VebjNBAk+DJVUfqjCBas bbn9Eg0jQcCxcfjBjtrxLbHQjfM9iZ24D5irA/f9bli/LsXu+SN9Prgf3TzhGo6uT6Pg lsRDTOwlQYtzAcwFpVy24i+ijGWNQjCmfFqtr1P0ECQ0DMwUtQm2M+nZLd53lNmWdN29 ufjFdLoG7rcBI9Fd8ujL1ChJNapOVJrsQTzT6+A/oFjLooOXk9Uh5qUidq1AJFNiBcu6 ljHg2iGLoH0kbZH3bnJSYIU974XEeLqeKWJ3a4dEIAs+Dw9zVodB6qtOU/ulRZ01A+Hq 8Dtw== X-Gm-Message-State: APjAAAX1YifQ3HWzAlAQLp8rRhKWL6ENv8mfowiMXd1CmvFwarZ0hcr2 a2wzxgM2EuocPPK51LRbeOuGLZ7Fw9sTWMCojaCPCw== X-Google-Smtp-Source: APXvYqxbO57Mt9kkEg13kCGDNBHG/voZqjh7Dsh3xrN9GCq+PmSJtUjUIM+nj4xE5Zp3WR/mEJHUTyOcqSBdeEFNp0U= X-Received: by 2002:adf:cf0a:: with SMTP id o10mr4608220wrj.325.1578492828987; Wed, 08 Jan 2020 06:13:48 -0800 (PST) MIME-Version: 1.0 References: <20200107094800.4488-1-ard.biesheuvel@linaro.org> <20200107094800.4488-5-ard.biesheuvel@linaro.org> <27a930b2-bbf8-a1d2-075f-6f33ce03b460@redhat.com> In-Reply-To: <27a930b2-bbf8-a1d2-075f-6f33ce03b460@redhat.com> From: "Ard Biesheuvel" Date: Wed, 8 Jan 2020 15:13:37 +0100 Message-ID: Subject: Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot To: Laszlo Ersek Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" On Tue, 7 Jan 2020 at 18:37, Laszlo Ersek wrote: > > 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. > The policy ones can be dropped, but I see warnings like these WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC) WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD) FinalEventsTable->NumberOfEvents - 0x3 Size - 0x15A SupportedEventLogs - 0x00000003 LogFormat - 0x00000001 LogFormat - 0x00000002 WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC) WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD) if I leave PcdTpm2HashMask at its default value > > +!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? > These are related to the TPM2 ACPI table that describes the physical presence interface to the OS, but I'm not even sure we can support this on ARM today, given that it relies on SMIs so I can drop these for now, I think. > > + > > ################################################################################ > > # > > # 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. :) > NP > (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 >