Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Tue, 7 Jan 2020 at 18:37, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
> > ---
> >  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 {
> >      <LibraryClasses>
> >        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 {
> > +    <LibraryClasses>
> > +      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 {
>     <LibraryClasses>
>       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 {
> > +    <LibraryClasses>
> > +      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
>