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.biesheu...@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.
> +!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 {
> <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. :)
(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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52995): https://edk2.groups.io/g/devel/message/52995
Mute This Topic: https://groups.io/mt/69499023/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
