From: "Laszlo Ersek" <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>, devel@edk2.groups.io
Subject: Re: [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot
Date: Tue, 7 Jan 2020 18:37:18 +0100 [thread overview]
Message-ID: <27a930b2-bbf8-a1d2-075f-6f33ce03b460@redhat.com> (raw)
In-Reply-To: <20200107094800.4488-5-ard.biesheuvel@linaro.org>
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.
> +!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
next prev parent reply other threads:[~2020-01-07 17:37 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-07 9:47 [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Ard Biesheuvel
2020-01-07 9:47 ` [PATCH 1/4] OvmfPkg/Tcg2ConfigPei: introduce a signalling PPI to depex on Ard Biesheuvel
2020-01-07 11:58 ` Laszlo Ersek
2020-01-07 9:47 ` [PATCH 2/4] ArmVirtPkg/PlatformPeiLib: discover the TPM base address from the DT Ard Biesheuvel
2020-01-07 15:42 ` Laszlo Ersek
2020-01-08 14:41 ` Ard Biesheuvel
2020-01-09 13:04 ` Laszlo Ersek
2020-01-07 9:47 ` [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI Ard Biesheuvel
2020-01-07 16:50 ` Laszlo Ersek
2020-01-07 16:55 ` [edk2-devel] " Ard Biesheuvel
2020-01-07 18:47 ` Laszlo Ersek
2020-01-08 9:59 ` Ard Biesheuvel
2020-01-07 9:48 ` [PATCH 4/4] ArmVirtPkg/ArmVirtQemu: add optional support for TPM2 measured boot Ard Biesheuvel
2020-01-07 17:37 ` Laszlo Ersek [this message]
2020-01-08 14:13 ` Ard Biesheuvel
2020-01-08 14:45 ` Laszlo Ersek
2020-01-09 0:51 ` Yao, Jiewen
2020-01-09 13:07 ` Laszlo Ersek
2020-01-10 0:32 ` Yao, Jiewen
2020-01-13 1:55 ` [edk2-devel] " Gary Lin
2020-01-13 15:56 ` Laszlo Ersek
2020-01-07 11:55 ` [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu Laszlo Ersek
2020-01-07 12:04 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27a930b2-bbf8-a1d2-075f-6f33ce03b460@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox