Re: [edk2-devel] [PATCH v3 7/9] ArmVirtPkg/ArmVirtQemu: enable the DXE phase TPM2 support module

["Laszlo Ersek" <lersek@redhat.com>]

On 02/26/20 20:05, Ard Biesheuvel wrote:
> Enable the TPM2 support module in the DXE phase, and the associated
> libraries and PCDs that it requires. This will be wired into the
> measured boot support code in a subsequent patch.
>
> Note that Tcg2Dxe.inf is added to ArmVirtQemuFvMain.fdf.inc, which
> is shared with other platforms in ArmVirtPkg, but as those will not
> set the TPM2_ENABLE define, this change does not affect them.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc           | 23 ++++++++++++++++++++
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  7 ++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 8950116dacab..9fe5ab37611c 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -77,6 +77,8 @@ [LibraryClasses.common]
>
>  !if $(TPM2_ENABLE) == TRUE
>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> +  Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf

This should not be needed. Or more precisely, not like this.

In OvmfPkg, Marc-André added TPM2 PPI support in the following series
(which was separately building upon the initial TPM2 enablement):

  [edk2] [PATCH v3 0/4] RFC: ovmf: Add support for TPM Physical Presence interface
  http://mid.mail-archive.com/20180518122304.31020-1-marcandre.lureau@redhat.com

  1  fe280ecbee11 OvmfPkg: add Tcg2PhysicalPresenceLibNull when !TPM2_ENABLE
  2  909ac65af113 OvmfPkg/IndustryStandard: add QemuTpm.h header
  3  b9777bb42e4f OvmfPkg: add Tcg2PhysicalPresenceLibQemu
  4  8d65d3b25e35 OvmfPkg/PlatformBootManagerLib: process TPM PPI request

The above lib class resolution mirrors commit#3 from those.

But it only really makes sense if, in the end, we intend to add a
Tcg2PhysicalPresenceLibProcessRequest() call to the
PlatformBootManagerLib instance (see commit#4).

The present ArmVirtQemu patch generally mirrors OVMF commit 0c0a50d6b3ff
("OvmfPkg: include Tcg2Dxe module", 2018-03-09), from the initial TPM2
enablement work. In that commit, the Tcg2PhysicalPresenceLib class was
resolved to the

  SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf

instance only. (And then later it was udpated by commit#3, from the TPM2
PPI series.)

Now... I guess it does make sense to mirror OVMF's lib class resolution
here, even if we don't port the rest of the PPI stuff to ArmVirtQemu at
this time; such as commit#1 and commit #4. (Note: I do agree we
shouldn't port the PPI stuff now.)

(1) So ultimtely my request is to add the following to the commit
message:

--------
This patch ports (parts of) the following OvmfPkg commits to
ArmVirtQemu:

- 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09)

- b9777bb42e4f ("OvmfPkg: add Tcg2PhysicalPresenceLibQemu", 2018-05-22)
               -- only to match OVMF's current lib class resolutions
--------

> +  TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf

(2) I agree this is necessary, but it should come in differently. Please
see OvmfPkg commit 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib if
and only if TPM2_ENABLE", 2019-07-04).

The gist is that, going forward, the TpmMeasurementLib resolution should
depend on TPM2_ENABLE only, in ArmVirtQemu as well. But currently we
have a SECURE_BOOT_ENABLE-dependent resolution in "ArmVirt.dsc.inc".

Therefore,

(2a) Please add another patch, before this one, replacing the current
TpmMeasurementLib resolution in "ArmVirt.dsc.inc" with an
*unconditional* resolution to TpmMeasurementLibNull.

Please mention commit 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib
if and only if TPM2_ENABLE", 2019-07-04) in the commit message.

(2b) Please add yet another patch, to move / triplicate that
(unconditional) TpmMeasurementLib Null resolution from the
"ArmVirt.dsc.inc" file, to the individual DSC files,

(2c) and in the current patch, please conditionalize the
TpmMeasurementLib resolution, solely based on TPM2_ENABLE,

(2d) also in the current patch, please also mention the commit below, in
the commit message section that I request above in (1):

------
- 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib if and only if
                TPM2_ENABLE", 2019-07-04)
------

>  !endif
>
>  [LibraryClasses.common.PEIM]
> @@ -91,6 +93,10 @@ [LibraryClasses.common.PEIM]
>  [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
>
> @@ -469,6 +475,23 @@ [Components.common]
>    MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf
>    MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
>
> +  #
> +  # TPM2 support
> +  #
> +!if $(TPM2_ENABLE) == TRUE
> +  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> +    <LibraryClasses>
> +      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> +      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
> +      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.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
>    #

(3) Looks good; please extend the commit message block (1) with the
following references:

- b9130c866dc0 ("OvmfPkg: link Sha384 and Sha512 support into Tcg2Pei
                and Tcg2Dxe", 2018-08-16)
- 5d3ef15da7c3 ("OvmfPkg: link SM3 support into Tcg2Pei and Tcg2Dxe",
                2019-07-19)

... So ultimately, block (1) should refer to *five* commits:

------------
This patch ports (parts of) the following OvmfPkg commits to
ArmVirtQemu:

- 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09)

- b9777bb42e4f ("OvmfPkg: add Tcg2PhysicalPresenceLibQemu", 2018-05-22)
               -- only to match OVMF's current lib class resolutions

- 1ec05b81e59f ("OvmfPkg: use DxeTpmMeasurementLib if and only if
                TPM2_ENABLE", 2019-07-04)

- b9130c866dc0 ("OvmfPkg: link Sha384 and Sha512 support into Tcg2Pei
                and Tcg2Dxe", 2018-08-16)

- 5d3ef15da7c3 ("OvmfPkg: link SM3 support into Tcg2Pei and Tcg2Dxe",
                2019-07-19)
------------


> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index 31f615a9d0f9..63247fd10058 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -182,3 +182,10 @@ [FV.FvMain]
>    # Ramdisk support
>    #
>    INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> +
> +  #
> +  # TPM2 support
> +  #
> +!if $(TPM2_ENABLE) == TRUE
> +  INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> +!endif
>

(4) Can you hoist this addition please, so that it follow
"UsbMassStorageDxe.inf"? Because that would reflect the DSC listing
order more closely.

Thanks!
Laszlo