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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hi
Comment for the warning: 
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xC)
> > WARNING: TPM2 Event log has HashAlg unsupported by PCR bank (0xD)

The reason is that: The DSC added all HASH algorithm to the TCG2 driver. (SHA1/SHA256/SHA384/SHA512/SM3).
But the current TPM hardware device does not support SHA384 (0xC) and SHA512 (0xD).

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
  }


It is warning because the Firmware Image *may* want to support another TPM2 which has such capability.
It just means the *current* TPM2 does not support this hash.
The platform owner may decide to clean up the warning by remove the SHA384/SHA512 null lib instance
support for current TPM2, or leave them as is for another TPM2.


BTW: Is there any document on how to enable TPM2 on QEMU ?
I would like to have a try. :-)

Thank you
Yao Jiewen


> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 8, 2020 10:45 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for
> TPM2 measured boot
> 
> (CC Marc-André and Jiewen)
> 
> On 01/08/20 15:13, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 18:37, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 01/07/20 10:48, Ard Biesheuvel wrote:
> 
> >>> +  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
> 
> Hmmm. My TPM2 "knowledge" is insufficient to judge and/or explain these
> warnings.
> 
> Jiewen, Marc-André, can you help with this perhaps?
> 
> >>> +!if $(TPM2_ENABLE) == TRUE
> >>> +
> gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_V
> ERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
> >>> +
> gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTc
> g2ConfigFormSetGuid|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.
> 
> "PcdTcgPhysicalPresenceInterfaceVer" is used by
> SmmTcg2PhysicalPresenceLib, Tcg2ConfigDxe, and Tcg2Smm. None of those
> are inclued in either OvmfPkg or ArmVirtPkg, so I think
> "PcdTcgPhysicalPresenceInterfaceVer" should be dropped.
> 
> ... Small correction: Tcg2ConfigDxe is included for TPM2_CONFIG_ENABLE.
> For me, TPM2_CONFIG_ENABLE is uncharted (and most likely: broken)
> territory. We added it in commit 3103389043bd because Stefan Berger
> really wanted it -- I insisted it be sequestered with a dedicated build
> flag (for "containing the damage"), and that's how we ended up with
> TPM2_CONFIG_ENABLE.
> 
> Therefore, if we add PcdTcgPhysicalPresenceInterfaceVer *only* when
> TPM2_CONFIG_ENABLE is TRUE, I'm fine. (I basically don't care about
> TPM2_CONFIG_ENABLE==TRUE -- I wanted the dedicated flag so I can
> *afford* not caring about those modules.)
> 
> 
> Regarding "PcdTpm2AcpiTableRev": it is *consumed* by Tcg2Dxe too, so we
> might want to set it, if we're not pleased with the default. But, as far
> as I understand, we still only need it to be under [PcdsDynamicHii] if
> we want to configure it through HII (usually: the display browser),
> which is again not the case unless we have TPM2_CONFIG_ENABLE.
> 
> So in the end, I'd like to see both PCDs either removed, or made
> dependent on TPM2_CONFIG_ENABLE == TRUE.
> 
> Thanks!
> Laszlo