[Qemu-devel] [PATCH 0/7] RFC: ovmf: preliminary TPM2 support

[Qemu-devel] [PATCH 0/7] RFC: ovmf: preliminary TPM2 support

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

The following series adds basic TPM2 support for OVMF-on-QEMU (I
haven't tested TPM1, for lack of interest). It links with the modules
to initializes the device in PEI phase, and do measurements (both PEI
and DXE). The Tcg2Dxe module provides the Tcg2 protocol which allows
the guest to access the measurement log and other facilities.

DxeTpm2MeasureBootLib seems to do its job at measuring images that are
not measured in PEI phase (such as PCI PXE rom)

Tcg2ConfigDxe is mostly interesting for debugging for now.

A major lack is the support for Physical Present Interface (PPI, more
below).

Linux guests seem to work fine. But windows guest generally complains
about the lack of PPI interface (most HLK tests require it, tpm.msc
admin interactions too). I haven't done "real" use-cases tests, as I
lack experience with TPM usage. Any help appreciated to test the TPM.

Tcg2ConfigPei requires variable access, therefore
<https://bugzilla.tianocore.org/show_bug.cgi?id=386> must be solved
first. I used "[edk2] [PATCH v2 0/8] OvmfPkg: add the Variable PEIM,
defragment the UEFI memmap" as a base for this series.

I build edk2 with:

$ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE  -DMEM_VARSTORE_EMU_ENABLE=FALSE

I test with qemu & swtpm/libtpms (tpm2 branches, swtpm_setup.sh --tpm2 --tpm-state tpmstatedir)

$ swtpm socket --tpmstate tpmstatedir --ctrl type=unixio,path=tpmsock  --tpm2 &
$ qemu .. -chardev socket,id=chrtpm,path=tpmsock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-crb,tpmdev=tpm0

PPI is problematic, because we generally don't want or need SMM, and
qemu is preferred to provide the ACPI tables. We therefore exclude
using Tcg2Smm for now (which also brings other problems). Stefan
Berger has been prototyping qemu code that provides PPI ACPI
interface, but there is some complication regarding memory location,
using a fixed address. My understanding is that the firmware
(seabios/edk2) should allocate the required memory itself (using qemu
linker script for ex) and patch the ACPI table. Then it's hopefully
only a matter of hooking Tcg2PhysicalPresenceLibProcessRequest() as
was done by Stefan in
https://github.com/stefanberger/edk2/commits/tpm2. The main problem I
see with this approach is that the location should remain stable
across reboots (not necessarily poweroff, edk2 uses nvram variables
for PPI flags). More investigation and help needed to support PPI!

Thanks

Related bug:
https://bugzilla.tianocore.org/show_bug.cgi?id=594

Marc-André Lureau (7):
  SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib dependency
  ovmf: link with Tcg2ConfigPei module
  HACK: HobLib: workaround infinite loop
  ovmf: link with Tcg2Pei module
  ovmf: link with Tcg2Dxe module
  ovmf: link with Tcg2ConfigDxe module
  ovmf: add DxeTpm2MeasureBootLib

 MdePkg/Library/PeiHobLib/HobLib.c   |  4 +++
 OvmfPkg/OvmfPkgX64.dsc              | 49 ++++++++++++++++++++++++++++++++++++-
 OvmfPkg/OvmfPkgX64.fdf              |  9 +++++++
 SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c   |  2 --
 SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf |  1 -
 5 files changed, 61 insertions(+), 4 deletions(-)

-- 
2.16.1.73.g5832b7e9f2

[Qemu-devel] [PATCH 1/7] SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib dependency

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Apparently, unnecessary. Avoids extra build dependency and churn.

CC: Laszlo Ersek <lersek@redhat.com>
CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c   | 2 --
 SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 -
 2 files changed, 3 deletions(-)

diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
index a7ae3354b5..3758fc6a41 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
@@ -18,7 +18,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <IndustryStandard/UefiTcgPlatform.h>
 #include <Ppi/FirmwareVolumeInfo.h>
 #include <Ppi/FirmwareVolumeInfo2.h>
-#include <Ppi/LockPhysicalPresence.h>
 #include <Ppi/TpmInitialized.h>
 #include <Ppi/FirmwareVolume.h>
 #include <Ppi/EndOfPeiPhase.h>
@@ -44,7 +43,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Library/MemoryAllocationLib.h>
 #include <Library/ReportStatusCodeLib.h>
 #include <Library/ResetSystemLib.h>
-#include <Library/Tcg2PhysicalPresenceLib.h>
 
 #define PERF_ID_TCG2_PEI  0x3080
 
diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
index f7b85444d9..bc910c3baf 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
@@ -58,7 +58,6 @@
   PerformanceLib
   MemoryAllocationLib
   ReportStatusCodeLib
-  Tcg2PhysicalPresenceLib
   ResetSystemLib
 
 [Guids]
-- 
2.16.1.73.g5832b7e9f2

[Qemu-devel] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This module initializes TPM device type based on variable and
detection.

The module requires VariablePei, which is built with
MEM_VARSTORE_EMU_ENABLE=FALSE.

CC: Laszlo Ersek <lersek@redhat.com>
CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 OvmfPkg/OvmfPkgX64.dsc | 20 ++++++++++++++++++++
 OvmfPkg/OvmfPkgX64.fdf |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 32c57b04e1..b5cbe8430f 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -40,6 +40,7 @@
   DEFINE SMM_REQUIRE             = FALSE
   DEFINE TLS_ENABLE              = FALSE
   DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
+  DEFINE TPM2_ENABLE             = FALSE
 
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -209,6 +210,11 @@
   OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
   XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
 
+!if $(TPM2_ENABLE) == TRUE
+  Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
+  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
+!endif
+
 [LibraryClasses.common]
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
 
@@ -272,6 +278,10 @@
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
+!if $(TPM2_ENABLE)
+  Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
+  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
+!endif
 
 [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -558,6 +568,12 @@
 
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
 
+!if $(TPM2_ENABLE) == TRUE
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0x8b, 0x19, 0x4f, 0xe8, 0x46, 0xab, 0x93, 0x1c, 0x53, 0x67, 0x1b, 0x90, 0xcc}
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
+!endif
+
 ################################################################################
 #
 # Components Section - list of all EDK II Modules needed by this Platform.
@@ -629,6 +645,10 @@
 
   MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
 
+!if $(TPM2_ENABLE) == TRUE
+  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+!endif
+
 !if $(SECURE_BOOT_ENABLE) == TRUE
   MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index bb46a409d9..dc35d0a1f7 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -168,6 +168,9 @@ INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
 INF  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
 INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
 !endif
+!if $(TPM2_ENABLE) == TRUE
+INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+!endif
 
 ################################################################################
 
-- 
2.16.1.73.g5832b7e9f2

[Qemu-devel] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Without this hack, GetNextHob() loops infinitely with the next patch.
I don't understand the reason.

The loop is triggered by the GetFirstGuidHob (&gTpmErrorHobGuid) call.

CC: Laszlo Ersek <lersek@redhat.com>
CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 MdePkg/Library/PeiHobLib/HobLib.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MdePkg/Library/PeiHobLib/HobLib.c b/MdePkg/Library/PeiHobLib/HobLib.c
index 5c0eeb992f..ed3c5fbd6d 100644
--- a/MdePkg/Library/PeiHobLib/HobLib.c
+++ b/MdePkg/Library/PeiHobLib/HobLib.c
@@ -89,6 +89,10 @@ GetNextHob (
     if (Hob.Header->HobType == Type) {
       return Hob.Raw;
     }
+    if (GET_HOB_LENGTH (HobStart) == 0) {
+        DEBUG ((DEBUG_INFO, "FIXME: GetNextHob length == 0"));
+        return NULL;
+    }
     Hob.Raw = GET_NEXT_HOB (Hob);
   }
   return NULL;
-- 
2.16.1.73.g5832b7e9f2

[Qemu-devel] [PATCH 4/7] ovmf: link with Tcg2Pei module

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This module will initialize TPM device, measure reported FVs and BIOS
version.

CC: Laszlo Ersek <lersek@redhat.com>
CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 OvmfPkg/OvmfPkgX64.dsc | 7 +++++++
 OvmfPkg/OvmfPkgX64.fdf | 1 +
 2 files changed, 8 insertions(+)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index b5cbe8430f..34a7c2778e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -279,6 +279,8 @@
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
 !if $(TPM2_ENABLE)
+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+  HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
 !endif
@@ -647,6 +649,11 @@
 
 !if $(TPM2_ENABLE) == TRUE
   SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
+    <LibraryClasses>
+      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
+      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
+  }
 !endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index dc35d0a1f7..9558142a42 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -170,6 +170,7 @@ INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
 !endif
 !if $(TPM2_ENABLE) == TRUE
 INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
 !endif
 
 ################################################################################
-- 
2.16.1.73.g5832b7e9f2

[Qemu-devel] [PATCH 5/7] ovmf: link with Tcg2Dxe module

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This module measures and log the boot environment. It also produces
the Tcg2 protocol, which allows for example to read the log from OS:

[    0.000000] efi: EFI v2.70 by EDK II
[    0.000000] efi:  SMBIOS=0x3fa1f000  ACPI=0x3fbb6000  ACPI 2.0=0x3fbb6014  MEMATTR=0x3e7d4318  TPMEventLog=0x3db21018

$ python chipsec_util.py tpm parse_log binary_bios_measurements

[CHIPSEC] Version 1.3.5.dev2
[CHIPSEC] API mode: using OS native API (not using CHIPSEC kernel module)
[CHIPSEC] Executing command 'tpm' with args ['parse_log', '/tmp/binary_bios_measurements']

PCR: 0	type: EV_S_CRTM_VERSION               	size: 0x2	digest: 1489f923c4dca729178b3e3233458550d8dddf29
	+ version:
PCR: 0	type: EV_EFI_PLATFORM_FIRMWARE_BLOB   	size: 0x10	digest: fd39ced7c0d2a61f6830c78c7625f94826b05bcc
	+ base: 0x820000	length: 0xe0000
PCR: 0	type: EV_EFI_PLATFORM_FIRMWARE_BLOB   	size: 0x10	digest: 39ebc6783b72bc1e73c7d5bcfeb5f54a3f105d4c
	+ base: 0x900000	length: 0xa00000
PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x35	digest: 57cd4dc19442475aa82743484f3b1caa88e142b8
PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x24	digest: 9b1387306ebb7ff8e795e7be77563666bbf4516e
PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x26	digest: 9afa86c507419b8570c62167cb9486d9fc809758
PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x24	digest: 5bf8faa078d40ffbd03317c93398b01229a0e1e0
PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x26	digest: 734424c9fe8fc71716c42096f4b74c88733b175e
PCR: 7	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x3e	digest: 252f8ebb85340290b64f4b06a001742be8e5cab6
PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x6e	digest: 22a4f6ee9af6dba01d3528deb64b74b582fc182b
PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x80	digest: b7811d5bf30a7efd4e385c6179fe10d9290bb9e8
PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x84	digest: 425e502c24fc924e231e0a62327b6b7d1f704573
PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x9a	digest: 0b5d2c98ac5de6148a4a1490ff9d5df69039f04e
PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0xbd	digest: 20bd5f402271d57a88ea314fe35c1705956b1f74
PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x88	digest: df5d6605cb8f4366d745a8464cfb26c1efdc305c
PCR: 4	type: EV_EFI_ACTION                   	size: 0x28	digest: cd0fdb4531a6ec41be2753ba042637d6e5f7f256
PCR: 0	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
PCR: 1	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
PCR: 2	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
PCR: 3	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
PCR: 4	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
PCR: 5	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473

$ tpm2_pcrlist
sha1 :
  0  : 35bd1786b6909daad610d7598b1d620352d33b8a
  1  : ec0511e860206e0af13c31da2f9e943fb6ca353d
  2  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
  3  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
  4  : 45a323382bd933f08e7f0e256bc8249e4095b1ec
  5  : d16d7e629fd8d08ca256f9ad3a3a1587c9e6cc1b
  6  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
  7  : 518bd167271fbb64589c61e43d8c0165861431d8
  8  : 0000000000000000000000000000000000000000
  9  : 0000000000000000000000000000000000000000
  10 : 0000000000000000000000000000000000000000
  11 : 0000000000000000000000000000000000000000
  12 : 0000000000000000000000000000000000000000
  13 : 0000000000000000000000000000000000000000
  14 : 0000000000000000000000000000000000000000
  15 : 0000000000000000000000000000000000000000
  16 : 0000000000000000000000000000000000000000
  17 : ffffffffffffffffffffffffffffffffffffffff
  18 : ffffffffffffffffffffffffffffffffffffffff
  19 : ffffffffffffffffffffffffffffffffffffffff
  20 : ffffffffffffffffffffffffffffffffffffffff
  21 : ffffffffffffffffffffffffffffffffffffffff
  22 : ffffffffffffffffffffffffffffffffffffffff
  23 : 0000000000000000000000000000000000000000
sha256 :
  0  : 9ae903dbae3357ac00d223660bac19ea5c021499a56201104332ab966631ce2c
  1  : acc611d90245cf04e77b0ca94901f90e7fa54770f0426f53c3049b532243d1b8
  2  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
  3  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
  4  : 7a94ffe8a7729a566d3d3c577fcb4b6b1e671f31540375f80eae6382ab785e35
  5  : a5ceb755d043f32431d63e39f5161464620a3437280494b5850dc1b47cc074e0
  6  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
  7  : 65caf8dd1e0ea7a6347b635d2b379c93b9a1351edc2afc3ecda700e534eb3068
  8  : 0000000000000000000000000000000000000000000000000000000000000000
  9  : 0000000000000000000000000000000000000000000000000000000000000000
  10 : 0000000000000000000000000000000000000000000000000000000000000000
  11 : 0000000000000000000000000000000000000000000000000000000000000000
  12 : 0000000000000000000000000000000000000000000000000000000000000000
  13 : 0000000000000000000000000000000000000000000000000000000000000000
  14 : 0000000000000000000000000000000000000000000000000000000000000000
  15 : 0000000000000000000000000000000000000000000000000000000000000000
  16 : 0000000000000000000000000000000000000000000000000000000000000000
  17 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
  18 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
  19 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
  20 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
  21 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
  22 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
  23 : 0000000000000000000000000000000000000000000000000000000000000000
sha384 :

The PhysicalPresenceLib is required, it sets some variables, but the
firmware doesn't act on it yet.

CC: Laszlo Ersek <lersek@redhat.com>
CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 OvmfPkg/OvmfPkgX64.dsc | 15 +++++++++++++++
 OvmfPkg/OvmfPkgX64.fdf |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 34a7c2778e..9bd0709f98 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -213,6 +213,8 @@
 !if $(TPM2_ENABLE) == TRUE
   Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
   Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
+  Tcg2PhysicalPresenceLib|SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
+  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
 !endif
 
 [LibraryClasses.common]
@@ -364,6 +366,11 @@
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
+!if $(TPM2_ENABLE)
+  HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
+  Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
+  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
+!endif
 
 [LibraryClasses.common.UEFI_APPLICATION]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -654,6 +661,14 @@
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
   }
+
+  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
+    <LibraryClasses>
+      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
+      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
+      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
+      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
+  }
 !endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 9558142a42..b8dd7ecae4 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -397,6 +397,10 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
 INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
 !endif
 
+!if $(TPM2_ENABLE) == TRUE
+INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
+!endif
+
 ################################################################################
 
 [FV.FVMAIN_COMPACT]
-- 
2.16.1.73.g5832b7e9f2

[Qemu-devel] [PATCH 6/7] ovmf: link with Tcg2ConfigDxe module

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The module allows to tweak and interact with the TPM. Note that many
actions are broken due to implementation of qemu TPM (providing it's
own ACPI table), and the lack of PPI implementation.

CC: Laszlo Ersek <lersek@redhat.com>
CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 OvmfPkg/OvmfPkgX64.dsc | 2 ++
 OvmfPkg/OvmfPkgX64.fdf | 1 +
 2 files changed, 3 insertions(+)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 9bd0709f98..2281bd5ff8 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -669,6 +669,8 @@
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
   }
+
+  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
 !endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index b8dd7ecae4..985404850f 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -399,6 +399,7 @@ INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
 
 !if $(TPM2_ENABLE) == TRUE
 INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
+INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
 !endif
 
 ################################################################################
-- 
2.16.1.73.g5832b7e9f2

[Qemu-devel] [PATCH 7/7] ovmf: add DxeTpm2MeasureBootLib

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The library registers a security management handler, to measure images
that are not measure in PEI phase.

This seems to work for example with the qemu PXE rom:

Loading driver at 0x0003E6C2000 EntryPoint=0x0003E6C9076 8086100e.efi

And the following binary_bios_measurements log entry seems to be
added:

PCR: 2	type: EV_EFI_BOOT_SERVICES_DRIVER     	size: 0x4e	digest: 70a22475e9f18806d2ed9193b48d80d26779d9a4

CC: Laszlo Ersek <lersek@redhat.com>
CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 OvmfPkg/OvmfPkgX64.dsc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2281bd5ff8..92ed9f3b0c 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -677,7 +677,10 @@
   MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
     <LibraryClasses>
       NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
-	}
+!if $(TPM2_ENABLE) == TRUE
+      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
+!endif
+  }
 !else
   MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
 !endif
-- 
2.16.1.73.g5832b7e9f2

Re: [Qemu-devel] [edk2] [PATCH 0/7] RFC: ovmf: preliminary TPM2 support

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> The following series adds basic TPM2 support for OVMF-on-QEMU (I
> haven't tested TPM1, for lack of interest). It links with the modules
> to initializes the device in PEI phase, and do measurements (both PEI
> and DXE). The Tcg2Dxe module provides the Tcg2 protocol which allows
> the guest to access the measurement log and other facilities.
> 
> DxeTpm2MeasureBootLib seems to do its job at measuring images that are
> not measured in PEI phase (such as PCI PXE rom)
> 
> Tcg2ConfigDxe is mostly interesting for debugging for now.
> 
> A major lack is the support for Physical Present Interface (PPI, more
> below).
> 
> Linux guests seem to work fine. But windows guest generally complains
> about the lack of PPI interface (most HLK tests require it, tpm.msc
> admin interactions too). I haven't done "real" use-cases tests, as I
> lack experience with TPM usage. Any help appreciated to test the TPM.
> 
> Tcg2ConfigPei requires variable access, therefore
> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> must be solved
> first. I used "[edk2] [PATCH v2 0/8] OvmfPkg: add the Variable PEIM,
> defragment the UEFI memmap" as a base for this series.
> 
> I build edk2 with:
> 
> $ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE  -DMEM_VARSTORE_EMU_ENABLE=FALSE
> 
> I test with qemu & swtpm/libtpms (tpm2 branches, swtpm_setup.sh --tpm2 --tpm-state tpmstatedir)
> 
> $ swtpm socket --tpmstate tpmstatedir --ctrl type=unixio,path=tpmsock  --tpm2 &
> $ qemu .. -chardev socket,id=chrtpm,path=tpmsock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-crb,tpmdev=tpm0

Thanks for this work -- extra thanks for the instructions regarding the
software TPM backend.

> PPI is problematic, because we generally don't want or need SMM, and
> qemu is preferred to provide the ACPI tables. We therefore exclude
> using Tcg2Smm for now (which also brings other problems). Stefan
> Berger has been prototyping qemu code that provides PPI ACPI
> interface, but there is some complication regarding memory location,
> using a fixed address. My understanding is that the firmware
> (seabios/edk2) should allocate the required memory itself (using qemu
> linker script for ex) and patch the ACPI table. Then it's hopefully
> only a matter of hooking Tcg2PhysicalPresenceLibProcessRequest() as
> was done by Stefan in
> https://github.com/stefanberger/edk2/commits/tpm2. The main problem I
> see with this approach is that the location should remain stable
> across reboots (not necessarily poweroff, edk2 uses nvram variables
> for PPI flags). More investigation and help needed to support PPI!

Indeed the main requirement for the OS to queue PPI operations for the
next boot of the firmware seems a "semi-persistent" storage. "Semi-"
because we target (warm) reboot, not complete poweroff plus re-launch.

The ACPI linker/loader is not suitable for this. It is great for making
the firmware allocate memory, but such allocations are never expected to
be stable. At the first boot, the firmware can allocate some blob just
fine, patch ACPI tables with the allocation address, and even write back
the allocation address to QEMU (for some device model to use).
Furthermore, the kernel can populate this blob. However, at reboot, the
firmware won't know where to look.

The firmware could use / set aside a RAM area at a dedicated
(pre-defined) memory address for this. However, that is incompatible
with our goal that as much as possible ACPI stuff should be generated
by, and come from, QEMU. We should also minimize the differences between
SeaBIOS and OVMF.

So, we need some kind of emulated NVRAM for the PPI opcode storage, such
that both its contents and its address survive a warm reboot.

- For SeaBIOS this means dedicated hw support from QEMU (hence Stefan's
"virtual memory device" as part of the TPM MMIO register block).

- For OVMF, in theory the pflash chip could be used, via UEFI variables.
However, this requires QEMU-generated AML to call into the firmware
(namely the UEFI variable driver). This is only possible with SMM (there
is no calling convention, from AML to the firmware, other than
formatting a request buffer and raising an SMI). That would mean many
complications, and also limit the feature to Q35.

So, as long as we'd like to target both firmwares eventually, the PPI
opcode storage should be carved out of the TPM MMIO register block, and
OVMF should be taught to consume the opcodes from there.


I'll try to go through these patches soon.

FWIW, the dependency on Tcg2ConfigPei is not great. I've tried to
upstream my series for TianoCore BZ#386 several times, and I've always
failed.

In
<http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503A97A4CF@shsmsx102.ccr.corp.intel.com>,
Jiewen wrote,

"Tcg2ConfigPei/Dxe are platform sample driver. A platform may have its
own version based upon platform requirement [...]"

So, because Tcg2ConfigPei seems to consume the UEFI variable only for
speeding up TPM detection, I guess we could add a "trimmed clone" to
OvmfPkg that performed the hw detection unconditionally (not relying on
any cached state). This would make sense in a virt world especially
because the TPM device model might disappear from, and reapper in, the
domain config, from one cold boot to the next.

Laszlo

> 
> Thanks
> 
> Related bug:
> https://bugzilla.tianocore.org/show_bug.cgi?id=594
> 
> Marc-André Lureau (7):
>   SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib dependency
>   ovmf: link with Tcg2ConfigPei module
>   HACK: HobLib: workaround infinite loop
>   ovmf: link with Tcg2Pei module
>   ovmf: link with Tcg2Dxe module
>   ovmf: link with Tcg2ConfigDxe module
>   ovmf: add DxeTpm2MeasureBootLib
> 
>  MdePkg/Library/PeiHobLib/HobLib.c   |  4 +++
>  OvmfPkg/OvmfPkgX64.dsc              | 49 ++++++++++++++++++++++++++++++++++++-
>  OvmfPkg/OvmfPkgX64.fdf              |  9 +++++++
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c   |  2 --
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf |  1 -
>  5 files changed, 61 insertions(+), 4 deletions(-)
> 

Re: [Qemu-devel] [edk2] [PATCH 1/7] SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib dependency

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Apparently, unnecessary. Avoids extra build dependency and churn.
> 
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c   | 2 --
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> index a7ae3354b5..3758fc6a41 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> @@ -18,7 +18,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <IndustryStandard/UefiTcgPlatform.h>
>  #include <Ppi/FirmwareVolumeInfo.h>
>  #include <Ppi/FirmwareVolumeInfo2.h>
> -#include <Ppi/LockPhysicalPresence.h>
>  #include <Ppi/TpmInitialized.h>
>  #include <Ppi/FirmwareVolume.h>
>  #include <Ppi/EndOfPeiPhase.h>
> @@ -44,7 +43,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/ReportStatusCodeLib.h>
>  #include <Library/ResetSystemLib.h>
> -#include <Library/Tcg2PhysicalPresenceLib.h>
>  
>  #define PERF_ID_TCG2_PEI  0x3080
>  
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> index f7b85444d9..bc910c3baf 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> @@ -58,7 +58,6 @@
>    PerformanceLib
>    MemoryAllocationLib
>    ReportStatusCodeLib
> -  Tcg2PhysicalPresenceLib
>    ResetSystemLib
>  
>  [Guids]
> 

Good catch.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This module initializes TPM device type based on variable and
> detection.

(1) I suggest we say the following here:

"The Tcg2ConfigPei module informs the firmware globally about the TPM
device type, by setting the PcdTpmInstanceGuid PCD to the appropriate
GUID value. The original module under SecurityPkg can perform device
detection, or read a cached value from a non-volatile UEFI variable.
OvmfPkg's clone of the module always performs the hardware detection."

Becase...

> The module requires VariablePei, which is built with
> MEM_VARSTORE_EMU_ENABLE=FALSE.

(2) ... as I hinted in my response to your blurb, and also as suggested
by "Tcg2ConfigPei.inf", we should clone Tcg2ConfigPei for OVMF, and
*trim it* quite a bit.

- The new location should be "OvmfPkg/Tcg/Tcg2Config/".

- We need not copy the ".uni" file (also drop MODULE_UNI_FILE from the
INF file)

- Re-generate FILE_GUID in the INF file with "uuidgen"

- Remove all PEI-phase variable access; always perform the hw detection.

- I would even suggest removing support for TPM1.2. Just check whether
TPM2 is available or not.


(3) Ultimately, this is what the module should do:

- Check the QEMU hardware for TPM2 availability only

- If found, set the dynamic PCD "PcdTpmInstanceGuid" to
  &gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
  the firmware about the TPM type.

- Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
  PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
  In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
  and the consumption of the "TPM type" PCD.

- If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
  (Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
  no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)


(4) Regarding the TPM detection itself. It looks like DetectTpmDevice()
[SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c] calls a number of TPM1.2
functions. If the earliest one fails, it assumes "no TPM" at all, but if
only a later call fails, it deduces, from the 1.2 failure, that TPM2 exists.

I think we can do better than this, in our Tcg2ConfigPei clone:

- We should call Tpm2RequestUseTpm() directly, from
"SecurityPkg/Include/Library/Tpm2DeviceLib.h".

- And, Tpm2Startup(), from
"SecurityPkg/Include/Library/Tpm2CommandLib.h", will be called by Tcg2Pei.


(5) Finally, there's no need to set "PcdTpmInitializationPolicy" to
anything. I don't see it consumed by any module that we should include
in OVMF. (More on this below.)


(6) Now, I realize Tcg2Pei *apparently* depends on
gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the
PEI phase) as well. That's a bug in the INF file (the [depex] section).
If you grep the Tcg2Pei module source for the GUID, the [depex] section
is the only hit. Can you please submit a separate patch that removes it
from the depex?


> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 20 ++++++++++++++++++++
>  OvmfPkg/OvmfPkgX64.fdf |  3 +++
>  2 files changed, 23 insertions(+)

Is there any particular reason to exclude the Ia32 and Ia32X64 builds?

If not, then please modify all three sets of dsc/fdf files identically.

> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 32c57b04e1..b5cbe8430f 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -40,6 +40,7 @@

(7) Please implement the following git settings in your edk2 clone:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

(in particular "xfuncname")

and

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09

This will help reviewers see what section of the DSC / FDF / INI / DEC
files are modified by a patch hunk.

>    DEFINE SMM_REQUIRE             = FALSE
>    DEFINE TLS_ENABLE              = FALSE
>    DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
> +  DEFINE TPM2_ENABLE             = FALSE
>  
>    #
>    # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> @@ -209,6 +210,11 @@
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>  
> +!if $(TPM2_ENABLE) == TRUE
> +  Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
> +  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> +!endif
> +

(8) For the patch, as posted, resolving Tpm2CommandLib looks unneeded,
because "SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" doesn't seem to
depend on that lib class.

However, for the OvmfPkg clone of Tcg2ConfigPei that I'm suggesting
here, *only* the Tpm2CommandLib resolution will be necessary.

>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>  
> @@ -272,6 +278,10 @@
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
> +!if $(TPM2_ENABLE)
> +  Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
> +  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
> +!endif

(9) Again, only the Tpm2DeviceLib resolution should be necessary (for
our clone).

(10) Furthermore, is there any particular reason you add this resolution
only for PEIMs? Are you going to add different resolutions later, for
different module types?

>  
>  [LibraryClasses.common.DXE_CORE]
>    HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
> @@ -558,6 +568,12 @@
>  
>    gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>  
> +!if $(TPM2_ENABLE) == TRUE
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0x8b, 0x19, 0x4f, 0xe8, 0x46, 0xab, 0x93, 0x1c, 0x53, 0x67, 0x1b, 0x90, 0xcc}

(11) This is wrong, IMO. The value you set here is
"gEfiTpmDeviceInstanceTpm12Guid", which I can't explain.

In order for the PCD to behave dynamically, we should indeed provide a
dynamic default. But that default value should be all-bits-zero (put
differently, "gEfiTpmDeviceInstanceNoneGuid"). The actual value (based
on hardware detection) should be set by our Tcg2ConfigPei clone (see
near the top).

> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
> +!endif

(12) There is no need to set PcdTpmInitializationPolicy, it should not
be used (either set or read) by any module we include in OVMF.

(13) There is also no need to set PcdTpm2InitializationPolicy. While we
*will* include Tcg2Pei, that module only consumes the PCD, so actual
dynamic behavior is not needed. Furthermore, the top-level default in
"SecurityPkg/SecurityPkg.dsc" is already 1, so we can simply inherit
that. It should cause Tcg2Pei to perform a full init on the TPM2 chip.

> +
>  ################################################################################
>  #
>  # Components Section - list of all EDK II Modules needed by this Platform.
> @@ -629,6 +645,10 @@
>  
>    MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>  
> +!if $(TPM2_ENABLE) == TRUE
> +  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +!endif
> +

(14) Please move this addition higher up in the DSC file, so that it
lands at the end of:

  #
  # PEI Phase modules
  #

just above

  #
  # DXE Phase modules
  #

>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>      <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index bb46a409d9..dc35d0a1f7 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -168,6 +168,9 @@ INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>  INF  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
>  INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>  !endif
> +!if $(TPM2_ENABLE) == TRUE
> +INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +!endif
>  
>  ################################################################################
>  
> 

(15) This seems correct, yes; with the remark that once you drop the
dependence on PEI phase variable access, the addition should follow

INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf

directly.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Without this hack, GetNextHob() loops infinitely with the next patch.
> I don't understand the reason.
> 
> The loop is triggered by the GetFirstGuidHob (&gTpmErrorHobGuid) call.
> 
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  MdePkg/Library/PeiHobLib/HobLib.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c b/MdePkg/Library/PeiHobLib/HobLib.c
> index 5c0eeb992f..ed3c5fbd6d 100644
> --- a/MdePkg/Library/PeiHobLib/HobLib.c
> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
> @@ -89,6 +89,10 @@ GetNextHob (
>      if (Hob.Header->HobType == Type) {
>        return Hob.Raw;
>      }
> +    if (GET_HOB_LENGTH (HobStart) == 0) {
> +        DEBUG ((DEBUG_INFO, "FIXME: GetNextHob length == 0"));
> +        return NULL;
> +    }
>      Hob.Raw = GET_NEXT_HOB (Hob);
>    }
>    return NULL;
> 

Strange. The HobLength field is supposed to include the size of the HOB header, so it should never be zero.

Furthermore, the PEI core initializes the HOB list; it should be terminated with an End-of-HOB-List HOB:

PeiCore()                             [MdeModulePkg/Core/Pei/PeiMain/PeiMain.c]
  InitializeMemoryServices()          [MdeModulePkg/Core/Pei/Memory/MemoryServices.c]
    PeiCoreBuildHobHandoffInfoTable() [MdeModulePkg/Core/Pei/Hob/Hob.c]

I tried to reproduce this issue by:
- applying patches 1, 2, and 4
- in function PeimEntryMA(), file "SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c", moving the GetFirstGuidHob (&gTpmErrorHobGuid) call to the top of the function.

It didn't hang for me.

Laszlo

Re: [Qemu-devel] [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Andrew Fish]

> On Feb 23, 2018, at 5:23 AM, marcandre.lureau@redhat.com wrote:
> 
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Without this hack, GetNextHob() loops infinitely with the next patch.
> I don't understand the reason.
> 
> The loop is triggered by the GetFirstGuidHob (&gTpmErrorHobGuid) call.
> 
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> MdePkg/Library/PeiHobLib/HobLib.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c b/MdePkg/Library/PeiHobLib/HobLib.c
> index 5c0eeb992f..ed3c5fbd6d 100644
> --- a/MdePkg/Library/PeiHobLib/HobLib.c
> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
> @@ -89,6 +89,10 @@ GetNextHob (
>     if (Hob.Header->HobType == Type) {
>       return Hob.Raw;
>     }
> +    if (GET_HOB_LENGTH (HobStart) == 0) {

As Laszlo points out this error condition is likely memory corruption. Thus it would be better to check for all know illegal values? 

if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)

Thanks,

Andrew Fish

> +        DEBUG ((DEBUG_INFO, "FIXME: GetNextHob length == 0"));
> +        return NULL;
> +    }
>     Hob.Raw = GET_NEXT_HOB (Hob);
>   }
>   return NULL;
> -- 
> 2.16.1.73.g5832b7e9f2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Qemu-devel] [edk2] [PATCH 1/7] SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib dependency

[Yao, Jiewen]

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> marcandre.lureau@redhat.com
> Sent: Friday, February 23, 2018 9:23 PM
> To: edk2-devel@lists.01.org
> Cc: qemu-devel@nongnu.org; javierm@redhat.com; pjones@redhat.com; Yao,
> Jiewen <jiewen.yao@intel.com>; lersek@redhat.com
> Subject: [edk2] [PATCH 1/7] SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib
> dependency
> 
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Apparently, unnecessary. Avoids extra build dependency and churn.
> 
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c   | 2 --
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> index a7ae3354b5..3758fc6a41 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> @@ -18,7 +18,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <IndustryStandard/UefiTcgPlatform.h>
>  #include <Ppi/FirmwareVolumeInfo.h>
>  #include <Ppi/FirmwareVolumeInfo2.h>
> -#include <Ppi/LockPhysicalPresence.h>
>  #include <Ppi/TpmInitialized.h>
>  #include <Ppi/FirmwareVolume.h>
>  #include <Ppi/EndOfPeiPhase.h>
> @@ -44,7 +43,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/ReportStatusCodeLib.h>
>  #include <Library/ResetSystemLib.h>
> -#include <Library/Tcg2PhysicalPresenceLib.h>
> 
>  #define PERF_ID_TCG2_PEI  0x3080
> 
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> index f7b85444d9..bc910c3baf 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> @@ -58,7 +58,6 @@
>    PerformanceLib
>    MemoryAllocationLib
>    ReportStatusCodeLib
> -  Tcg2PhysicalPresenceLib
>    ResetSystemLib
> 
>  [Guids]
> --
> 2.16.1.73.g5832b7e9f2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Qemu-devel] [edk2] [PATCH 4/7] ovmf: link with Tcg2Pei module

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This module will initialize TPM device, measure reported FVs and BIOS
> version.
> 
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 7 +++++++
>  OvmfPkg/OvmfPkgX64.fdf | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index b5cbe8430f..34a7c2778e 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -279,6 +279,8 @@
>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
>  !if $(TPM2_ENABLE)
> +  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> +  HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
>    Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>  !endif
> @@ -647,6 +649,11 @@
>  
>  !if $(TPM2_ENABLE) == TRUE
>    SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> +    <LibraryClasses>
> +      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> +  }
>  !endif
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index dc35d0a1f7..9558142a42 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -170,6 +170,7 @@ INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>  !endif
>  !if $(TPM2_ENABLE) == TRUE
>  INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
>  
>  ################################################################################
> 

Would it be possible to drop SHA1 (include SHA256 only) by setting
PcdTpm2HashMask to value 2? Or SHA1 required for some other reason? (If
so please mention it in the commit message.)

Thanks
Laszlo

Re: [Qemu-devel] [edk2] [PATCH 5/7] ovmf: link with Tcg2Dxe module

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This module measures and log the boot environment. It also produces
> the Tcg2 protocol, which allows for example to read the log from OS:
> 
> [    0.000000] efi: EFI v2.70 by EDK II
> [    0.000000] efi:  SMBIOS=0x3fa1f000  ACPI=0x3fbb6000  ACPI 2.0=0x3fbb6014  MEMATTR=0x3e7d4318  TPMEventLog=0x3db21018
> 
> $ python chipsec_util.py tpm parse_log binary_bios_measurements
> 
> [CHIPSEC] Version 1.3.5.dev2
> [CHIPSEC] API mode: using OS native API (not using CHIPSEC kernel module)
> [CHIPSEC] Executing command 'tpm' with args ['parse_log', '/tmp/binary_bios_measurements']
> 
> PCR: 0	type: EV_S_CRTM_VERSION               	size: 0x2	digest: 1489f923c4dca729178b3e3233458550d8dddf29
> 	+ version:
> PCR: 0	type: EV_EFI_PLATFORM_FIRMWARE_BLOB   	size: 0x10	digest: fd39ced7c0d2a61f6830c78c7625f94826b05bcc
> 	+ base: 0x820000	length: 0xe0000
> PCR: 0	type: EV_EFI_PLATFORM_FIRMWARE_BLOB   	size: 0x10	digest: 39ebc6783b72bc1e73c7d5bcfeb5f54a3f105d4c
> 	+ base: 0x900000	length: 0xa00000
> PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x35	digest: 57cd4dc19442475aa82743484f3b1caa88e142b8
> PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x24	digest: 9b1387306ebb7ff8e795e7be77563666bbf4516e
> PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x26	digest: 9afa86c507419b8570c62167cb9486d9fc809758
> PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x24	digest: 5bf8faa078d40ffbd03317c93398b01229a0e1e0
> PCR: 7	type: EV_EFI_VARIABLE_DRIVER_CONFIG   	size: 0x26	digest: 734424c9fe8fc71716c42096f4b74c88733b175e
> PCR: 7	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
> PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x3e	digest: 252f8ebb85340290b64f4b06a001742be8e5cab6
> PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x6e	digest: 22a4f6ee9af6dba01d3528deb64b74b582fc182b
> PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x80	digest: b7811d5bf30a7efd4e385c6179fe10d9290bb9e8
> PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x84	digest: 425e502c24fc924e231e0a62327b6b7d1f704573
> PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x9a	digest: 0b5d2c98ac5de6148a4a1490ff9d5df69039f04e
> PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0xbd	digest: 20bd5f402271d57a88ea314fe35c1705956b1f74
> PCR: 1	type: EV_EFI_VARIABLE_BOOT            	size: 0x88	digest: df5d6605cb8f4366d745a8464cfb26c1efdc305c
> PCR: 4	type: EV_EFI_ACTION                   	size: 0x28	digest: cd0fdb4531a6ec41be2753ba042637d6e5f7f256
> PCR: 0	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
> PCR: 1	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
> PCR: 2	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
> PCR: 3	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
> PCR: 4	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
> PCR: 5	type: EV_SEPARATOR                    	size: 0x4	digest: 9069ca78e7450a285173431b3e52c5c25299e473
> 
> $ tpm2_pcrlist
> sha1 :
>   0  : 35bd1786b6909daad610d7598b1d620352d33b8a
>   1  : ec0511e860206e0af13c31da2f9e943fb6ca353d
>   2  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>   3  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>   4  : 45a323382bd933f08e7f0e256bc8249e4095b1ec
>   5  : d16d7e629fd8d08ca256f9ad3a3a1587c9e6cc1b
>   6  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>   7  : 518bd167271fbb64589c61e43d8c0165861431d8
>   8  : 0000000000000000000000000000000000000000
>   9  : 0000000000000000000000000000000000000000
>   10 : 0000000000000000000000000000000000000000
>   11 : 0000000000000000000000000000000000000000
>   12 : 0000000000000000000000000000000000000000
>   13 : 0000000000000000000000000000000000000000
>   14 : 0000000000000000000000000000000000000000
>   15 : 0000000000000000000000000000000000000000
>   16 : 0000000000000000000000000000000000000000
>   17 : ffffffffffffffffffffffffffffffffffffffff
>   18 : ffffffffffffffffffffffffffffffffffffffff
>   19 : ffffffffffffffffffffffffffffffffffffffff
>   20 : ffffffffffffffffffffffffffffffffffffffff
>   21 : ffffffffffffffffffffffffffffffffffffffff
>   22 : ffffffffffffffffffffffffffffffffffffffff
>   23 : 0000000000000000000000000000000000000000
> sha256 :
>   0  : 9ae903dbae3357ac00d223660bac19ea5c021499a56201104332ab966631ce2c
>   1  : acc611d90245cf04e77b0ca94901f90e7fa54770f0426f53c3049b532243d1b8
>   2  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
>   3  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
>   4  : 7a94ffe8a7729a566d3d3c577fcb4b6b1e671f31540375f80eae6382ab785e35
>   5  : a5ceb755d043f32431d63e39f5161464620a3437280494b5850dc1b47cc074e0
>   6  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
>   7  : 65caf8dd1e0ea7a6347b635d2b379c93b9a1351edc2afc3ecda700e534eb3068
>   8  : 0000000000000000000000000000000000000000000000000000000000000000
>   9  : 0000000000000000000000000000000000000000000000000000000000000000
>   10 : 0000000000000000000000000000000000000000000000000000000000000000
>   11 : 0000000000000000000000000000000000000000000000000000000000000000
>   12 : 0000000000000000000000000000000000000000000000000000000000000000
>   13 : 0000000000000000000000000000000000000000000000000000000000000000
>   14 : 0000000000000000000000000000000000000000000000000000000000000000
>   15 : 0000000000000000000000000000000000000000000000000000000000000000
>   16 : 0000000000000000000000000000000000000000000000000000000000000000
>   17 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>   18 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>   19 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>   20 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>   21 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>   22 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>   23 : 0000000000000000000000000000000000000000000000000000000000000000
> sha384 :
> 
> The PhysicalPresenceLib is required, it sets some variables, but the
> firmware doesn't act on it yet.
> 
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 15 +++++++++++++++
>  OvmfPkg/OvmfPkgX64.fdf |  4 ++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 34a7c2778e..9bd0709f98 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -213,6 +213,8 @@
>  !if $(TPM2_ENABLE) == TRUE
>    Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> +  Tcg2PhysicalPresenceLib|SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
> +  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
>  !endif
>  
>  [LibraryClasses.common]

I'd prefer to review this part in v2, once the @@ hunk headers are set
up at your end ("xfuncname").

> @@ -364,6 +366,11 @@
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +!if $(TPM2_ENABLE)
> +  HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> +  Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf

Can we drop TPM12? "Tcg2Dxe.inf" does not seem to depend on this lib class.

> +  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> +!endif
>  
>  [LibraryClasses.common.UEFI_APPLICATION]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -654,6 +661,14 @@
>        NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>    }
> +
> +  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
> +    <LibraryClasses>
> +      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf

Can you please explain why Tpm2DeviceLib has to be resolved differently
for DXE_DRIVER modules in general (see above) and for "Tcg2Dxe.inf"
specifically?

Hmmm... Looks like "Tpm2DeviceLibTcg2.inf" implements the APIs via the
TPM2 protocol. Whereas "Tcg2Dxe.inf" itself provides that protocol, so
obviously it cannot rely on the protocol; it has to access the device,
which is done with the help of "Tpm2DeviceLibRouterDxe.inf" and the
"Tpm2InstanceLibDTpm.inf" that is plugged in via NULL resolution below.
Is that about correct?

If so, can you please document it in the commit message?

> +      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf

Again -- is SHA1 required?

> +      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> +  }
>  !endif
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 9558142a42..b8dd7ecae4 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -397,6 +397,10 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>  INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>  !endif
>  
> +!if $(TPM2_ENABLE) == TRUE
> +INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> +!endif
> +
>  ################################################################################
>  
>  [FV.FVMAIN_COMPACT]
> 

Thanks!
Laszlo

Re: [Qemu-devel] [edk2] [PATCH 6/7] ovmf: link with Tcg2ConfigDxe module

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The module allows to tweak and interact with the TPM. Note that many
> actions are broken due to implementation of qemu TPM (providing it's
> own ACPI table), and the lack of PPI implementation.
> 
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 2 ++
>  OvmfPkg/OvmfPkgX64.fdf | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 9bd0709f98..2281bd5ff8 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -669,6 +669,8 @@
>        NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>    }
> +
> +  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>  !endif
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index b8dd7ecae4..985404850f 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -399,6 +399,7 @@ INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>  
>  !if $(TPM2_ENABLE) == TRUE
>  INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> +INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>  !endif
>  
>  ################################################################################
> 

Please drop this patch.

In my earlier investigation I wrote, Tcg2ConfigDxe "[p]rovides a Setup
TUI interface to configure the TPM. IIUC, it can also save the
configured TPM type for subsequent boots (see Tcg2ConfigPei.inf above)".

The INF file itself says "This module is only for reference only, each
platform should have its own setup page."

And Jiewen wrote earlier, "Tcg2ConfigPei/Dxe are platform sample driver.
A platform may have its own version based upon platform requirement. For
example, if a platform supports fTPM, it may use another Tcg2Config driver."

Given that OVMF lacks PEI-phase variable access, and that I consequently
suggested cloning, and seriously trimming, Tcg2ConfigPei, it makes no
sense to include an HII dialog that sets a variable for PEI phase
consumption. Also, as you say, many of the exposed operations are broken
due to lack of PPI support. So let's just postpone the inclusion of this
driver, for now.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 7/7] ovmf: add DxeTpm2MeasureBootLib

[Laszlo Ersek]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The library registers a security management handler, to measure images
> that are not measure in PEI phase.
>
> This seems to work for example with the qemu PXE rom:
>
> Loading driver at 0x0003E6C2000 EntryPoint=0x0003E6C9076 8086100e.efi
>
> And the following binary_bios_measurements log entry seems to be
> added:
>
> PCR: 2	type: EV_EFI_BOOT_SERVICES_DRIVER     	size: 0x4e	digest: 70a22475e9f18806d2ed9193b48d80d26779d9a4
>
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 2281bd5ff8..92ed9f3b0c 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -677,7 +677,10 @@
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>      <LibraryClasses>
>        NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -	}
> +!if $(TPM2_ENABLE) == TRUE
> +      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> +!endif
> +  }
>  !else
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>  !endif
>

This looks OK to me.

First, can you please clean up the SecurityStubDxe stanza as follows (as
a separate patch):

> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 96fc7b82e708..f4288b625cba 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -634,14 +634,12 @@ [Components]
>
>    MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>
> -!if $(SECURE_BOOT_ENABLE) == TRUE
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>      <LibraryClasses>
> +!if $(SECURE_BOOT_ENABLE) == TRUE
>        NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -       }
> -!else
> -  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>  !endif
> +  }
>
>    MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
>    PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf

The idea is that "SecurityStubDxe.inf" should be included
unconditionally; only its plug-in libs should be conditional on various
build flags. While the current (pre-patch) code does that -- in effect
-- for SECURE_BOOT_ENABLE already, your patch (as-is) can only add
TPM2_ENABLE *within* SECURE_BOOT_ENABLE.

I don't think that's for the best -- first we should make
DxeImageVerificationLib the *only* bit that's conditional on
SECURE_BOOT_ENABLE, and then we can add DxeTpm2MeasureBootLib
independently. (If neither build option is specified, we'll have a
<LibraryClasses> list that's empty, but that's perfectly fine.)

Thanks!
Laszlo

Re: [Qemu-devel] [edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

[Marc-André Lureau]

Hi

On Fri, Feb 23, 2018 at 6:31 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> This module initializes TPM device type based on variable and
>> detection.
>
> (1) I suggest we say the following here:
>
> "The Tcg2ConfigPei module informs the firmware globally about the TPM
> device type, by setting the PcdTpmInstanceGuid PCD to the appropriate
> GUID value. The original module under SecurityPkg can perform device
> detection, or read a cached value from a non-volatile UEFI variable.
> OvmfPkg's clone of the module always performs the hardware detection."
>

ok

> Becase...
>
>> The module requires VariablePei, which is built with
>> MEM_VARSTORE_EMU_ENABLE=FALSE.
>
> (2) ... as I hinted in my response to your blurb, and also as suggested
> by "Tcg2ConfigPei.inf", we should clone Tcg2ConfigPei for OVMF, and
> *trim it* quite a bit.
>
> - The new location should be "OvmfPkg/Tcg/Tcg2Config/".
>
> - We need not copy the ".uni" file (also drop MODULE_UNI_FILE from the
> INF file)
>
> - Re-generate FILE_GUID in the INF file with "uuidgen"
>
> - Remove all PEI-phase variable access; always perform the hw detection.
>
> - I would even suggest removing support for TPM1.2. Just check whether
> TPM2 is available or not.
>

ok

>
> (3) Ultimately, this is what the module should do:
>
> - Check the QEMU hardware for TPM2 availability only
>
> - If found, set the dynamic PCD "PcdTpmInstanceGuid" to
>   &gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
>   the firmware about the TPM type.
>
> - Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
>   PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
>   In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
>   and the consumption of the "TPM type" PCD.
>
> - If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
>   (Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
>   no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)

ok

>
> (4) Regarding the TPM detection itself. It looks like DetectTpmDevice()
> [SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c] calls a number of TPM1.2
> functions. If the earliest one fails, it assumes "no TPM" at all, but if
> only a later call fails, it deduces, from the 1.2 failure, that TPM2 exists.
>
> I think we can do better than this, in our Tcg2ConfigPei clone:
>
> - We should call Tpm2RequestUseTpm() directly, from
> "SecurityPkg/Include/Library/Tpm2DeviceLib.h".
>
> - And, Tpm2Startup(), from
> "SecurityPkg/Include/Library/Tpm2CommandLib.h", will be called by Tcg2Pei.

ok

>
> (5) Finally, there's no need to set "PcdTpmInitializationPolicy" to
> anything. I don't see it consumed by any module that we should include
> in OVMF. (More on this below.)
>

ok

>
> (6) Now, I realize Tcg2Pei *apparently* depends on
> gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the
> PEI phase) as well. That's a bug in the INF file (the [depex] section).
> If you grep the Tcg2Pei module source for the GUID, the [depex] section
> is the only hit. Can you please submit a separate patch that removes it
> from the depex?

I don't get how you came to that conclusion, both
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c and
SecurityPkg/Tcg/Tcg2Config/TpmDetection.c match. Apparently, the
variable is used in s3 mode, in DetectTpmDevice().

I'll drop it from OvmfPkg version for now.

>
>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  OvmfPkg/OvmfPkgX64.dsc | 20 ++++++++++++++++++++
>>  OvmfPkg/OvmfPkgX64.fdf |  3 +++
>>  2 files changed, 23 insertions(+)
>
> Is there any particular reason to exclude the Ia32 and Ia32X64 builds?
>
> If not, then please modify all three sets of dsc/fdf files identically.

I'd rather keep this as a TODO item for now, since we are not close to
a final version, and it's annoying to have to fix each files etc..

>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 32c57b04e1..b5cbe8430f 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -40,6 +40,7 @@
>
> (7) Please implement the following git settings in your edk2 clone:
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05
>
> (in particular "xfuncname")
>
> and
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09
>
> This will help reviewers see what section of the DSC / FDF / INI / DEC
> files are modified by a patch hunk.
>

done

>>    DEFINE SMM_REQUIRE             = FALSE
>>    DEFINE TLS_ENABLE              = FALSE
>>    DEFINE MEM_VARSTORE_EMU_ENABLE = TRUE
>> +  DEFINE TPM2_ENABLE             = FALSE
>>
>>    #
>>    # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
>> @@ -209,6 +210,11 @@
>>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>>
>> +!if $(TPM2_ENABLE) == TRUE
>> +  Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
>> +  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> +!endif
>> +
>
> (8) For the patch, as posted, resolving Tpm2CommandLib looks unneeded,
> because "SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" doesn't seem to
> depend on that lib class.
>
> However, for the OvmfPkg clone of Tcg2ConfigPei that I'm suggesting
> here, *only* the Tpm2CommandLib resolution will be necessary.
>

iirc, I tried removing it and it failed to link. Anyway, next version
will use tpm2 only.

>>  [LibraryClasses.common]
>>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>>
>> @@ -272,6 +278,10 @@
>>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
>>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
>> +!if $(TPM2_ENABLE)
>> +  Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>> +  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>> +!endif
>
> (9) Again, only the Tpm2DeviceLib resolution should be necessary (for
> our clone).
>
> (10) Furthermore, is there any particular reason you add this resolution
> only for PEIMs? Are you going to add different resolutions later, for
> different module types?
>

I followed SecurityPkg dsc, they use Tpm2DeviceLibRouterDxe.inf and
Tpm2DeviceLibTcg2.inf for some reason I don't quite understand yet.

>>
>>  [LibraryClasses.common.DXE_CORE]
>>    HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>> @@ -558,6 +568,12 @@
>>
>>    gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
>>
>> +!if $(TPM2_ENABLE) == TRUE
>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0x8b, 0x19, 0x4f, 0xe8, 0x46, 0xab, 0x93, 0x1c, 0x53, 0x67, 0x1b, 0x90, 0xcc}
>
> (11) This is wrong, IMO. The value you set here is
> "gEfiTpmDeviceInstanceTpm12Guid", which I can't explain.
>
> In order for the PCD to behave dynamically, we should indeed provide a
> dynamic default. But that default value should be all-bits-zero (put
> differently, "gEfiTpmDeviceInstanceNoneGuid"). The actual value (based
> on hardware detection) should be set by our Tcg2ConfigPei clone (see
> near the top).

ok

>
>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
>> +!endif
>
> (12) There is no need to set PcdTpmInitializationPolicy, it should not
> be used (either set or read) by any module we include in OVMF.
>
> (13) There is also no need to set PcdTpm2InitializationPolicy. While we
> *will* include Tcg2Pei, that module only consumes the PCD, so actual
> dynamic behavior is not needed. Furthermore, the top-level default in
> "SecurityPkg/SecurityPkg.dsc" is already 1, so we can simply inherit
> that. It should cause Tcg2Pei to perform a full init on the TPM2 chip.
>

ok

>> +
>>  ################################################################################
>>  #
>>  # Components Section - list of all EDK II Modules needed by this Platform.
>> @@ -629,6 +645,10 @@
>>
>>    MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>>
>> +!if $(TPM2_ENABLE) == TRUE
>> +  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +!endif
>> +
>
> (14) Please move this addition higher up in the DSC file, so that it
> lands at the end of:
>
>   #
>   # PEI Phase modules
>   #
>
> just above
>
>   #
>   # DXE Phase modules
>   #
>

done

>>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>>      <LibraryClasses>
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index bb46a409d9..dc35d0a1f7 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -168,6 +168,9 @@ INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>>  INF  MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
>>  INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>>  !endif
>> +!if $(TPM2_ENABLE) == TRUE
>> +INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +!endif
>>
>>  ################################################################################
>>
>>
>
> (15) This seems correct, yes; with the remark that once you drop the
> dependence on PEI phase variable access, the addition should follow
>
> INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>

done

> directly.
>
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel



-- 
Marc-André Lureau

Re: [Qemu-devel] [edk2] [PATCH 4/7] ovmf: link with Tcg2Pei module

[Marc-André Lureau]

Hi

On Mon, Feb 26, 2018 at 10:38 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> This module will initialize TPM device, measure reported FVs and BIOS
>> version.
>>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  OvmfPkg/OvmfPkgX64.dsc | 7 +++++++
>>  OvmfPkg/OvmfPkgX64.fdf | 1 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index b5cbe8430f..34a7c2778e 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -279,6 +279,8 @@
>>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
>>  !if $(TPM2_ENABLE)
>> +  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>> +  HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
>>    Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>>  !endif
>> @@ -647,6 +649,11 @@
>>
>>  !if $(TPM2_ENABLE) == TRUE
>>    SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>> +    <LibraryClasses>
>> +      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>> +      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>> +  }
>>  !endif
>>
>>  !if $(SECURE_BOOT_ENABLE) == TRUE
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index dc35d0a1f7..9558142a42 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -170,6 +170,7 @@ INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>>  !endif
>>  !if $(TPM2_ENABLE) == TRUE
>>  INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>  !endif
>>
>>  ################################################################################
>>
>
> Would it be possible to drop SHA1 (include SHA256 only) by setting
> PcdTpm2HashMask to value 2? Or SHA1 required for some other reason? (If
> so please mention it in the commit message.)
>

afaik, it's not strictly required, and apparently the support is being
dropped. I'll remove it.

btw, now I understand your comment about read-only variable not being
used by PEI module. I'll add a preliminary patch dropping it from
depex ;)

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [edk2] [PATCH 0/7] RFC: ovmf: preliminary TPM2 support

[Stefan Berger]

On 02/23/2018 10:55 AM, Laszlo Ersek wrote:
> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Hi,
>>
>> The following series adds basic TPM2 support for OVMF-on-QEMU (I
>> haven't tested TPM1, for lack of interest). It links with the modules
>> to initializes the device in PEI phase, and do measurements (both PEI
>> and DXE). The Tcg2Dxe module provides the Tcg2 protocol which allows
>> the guest to access the measurement log and other facilities.
>>
>> DxeTpm2MeasureBootLib seems to do its job at measuring images that are
>> not measured in PEI phase (such as PCI PXE rom)
>>
>> Tcg2ConfigDxe is mostly interesting for debugging for now.
>>
>> A major lack is the support for Physical Present Interface (PPI, more
>> below).
>>
>> Linux guests seem to work fine. But windows guest generally complains
>> about the lack of PPI interface (most HLK tests require it, tpm.msc
>> admin interactions too). I haven't done "real" use-cases tests, as I
>> lack experience with TPM usage. Any help appreciated to test the TPM.
>>
>> Tcg2ConfigPei requires variable access, therefore
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> must be solved
>> first. I used "[edk2] [PATCH v2 0/8] OvmfPkg: add the Variable PEIM,
>> defragment the UEFI memmap" as a base for this series.
>>
>> I build edk2 with:
>>
>> $ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE  -DMEM_VARSTORE_EMU_ENABLE=FALSE
>>
>> I test with qemu & swtpm/libtpms (tpm2 branches, swtpm_setup.sh --tpm2 --tpm-state tpmstatedir)
>>
>> $ swtpm socket --tpmstate tpmstatedir --ctrl type=unixio,path=tpmsock  --tpm2 &
>> $ qemu .. -chardev socket,id=chrtpm,path=tpmsock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-crb,tpmdev=tpm0
> Thanks for this work -- extra thanks for the instructions regarding the
> software TPM backend.

Please use the tpm2-preview.v2 branch of swtpm and the 
tpm2-preview.rev146.v2 branch of libtpms. I had to change the way the 
state is serialized, so unfortunately you will also have to remove the 
tpm2-00.permall files.

    Stefan

Re: [Qemu-devel] [edk2] [PATCH 6/7] ovmf: link with Tcg2ConfigDxe module

[Stefan Berger]

On 02/26/2018 04:58 AM, Laszlo Ersek wrote:
> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> The module allows to tweak and interact with the TPM. Note that many
>> actions are broken due to implementation of qemu TPM (providing it's
>> own ACPI table), and the lack of PPI implementation.
>>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>   OvmfPkg/OvmfPkgX64.dsc | 2 ++
>>   OvmfPkg/OvmfPkgX64.fdf | 1 +
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 9bd0709f98..2281bd5ff8 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -669,6 +669,8 @@
>>         NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>>         NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>>     }
>> +
>> +  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>>   !endif
>>   
>>   !if $(SECURE_BOOT_ENABLE) == TRUE
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index b8dd7ecae4..985404850f 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -399,6 +399,7 @@ INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>>   
>>   !if $(TPM2_ENABLE) == TRUE
>>   INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
>> +INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>>   !endif
>>   
>>   ################################################################################
>>
> Please drop this patch.
>
> In my earlier investigation I wrote, Tcg2ConfigDxe "[p]rovides a Setup
> TUI interface to configure the TPM. IIUC, it can also save the
> configured TPM type for subsequent boots (see Tcg2ConfigPei.inf above)".
>
> The INF file itself says "This module is only for reference only, each
> platform should have its own setup page."
>
> And Jiewen wrote earlier, "Tcg2ConfigPei/Dxe are platform sample driver.
> A platform may have its own version based upon platform requirement. For
> example, if a platform supports fTPM, it may use another Tcg2Config driver."
>
> Given that OVMF lacks PEI-phase variable access, and that I consequently
> suggested cloning, and seriously trimming, Tcg2ConfigPei, it makes no
> sense to include an HII dialog that sets a variable for PEI phase
> consumption. Also, as you say, many of the exposed operations are broken
> due to lack of PPI support. So let's just postpone the inclusion of this
> driver, for now.

Just FYI: The PPI support for the OS requires ACPI and, as it is 
currently implemented, SMF where UEFI variables are manipulated. Some 
menu items in the TPM 2 menu (also TPM 1.2) also require these UEFI 
variables of the PPI interface so that UEFI can react on the menu 
choices upon re.

    Stefan

Re: [Qemu-devel] [edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module

[Laszlo Ersek]

On 03/01/18 15:59, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Feb 23, 2018 at 6:31 PM, Laszlo Ersek <lersek@redhat.com> wrote:

>> (6) Now, I realize Tcg2Pei *apparently* depends on
>> gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the
>> PEI phase) as well. That's a bug in the INF file (the [depex] section).
>> If you grep the Tcg2Pei module source for the GUID, the [depex] section
>> is the only hit. Can you please submit a separate patch that removes it
>> from the depex?
> 
> I don't get how you came to that conclusion, both
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c and
> SecurityPkg/Tcg/Tcg2Config/TpmDetection.c match. Apparently, the
> variable is used in s3 mode, in DetectTpmDevice().

In my point (6) above, I was  talking about Tcg2Pei, not Tcg2ConfigPei.

If you grep "SecurityPkg/Tcg/Tcg2Pei" for
"gEfiPeiReadOnlyVariable2PpiGuid", the only hit is the depex section in
the INF file.

I think that's a bug in "SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf", and the
PPI GUID should removed from there, as a separate patch.

>>> ---
>>>  OvmfPkg/OvmfPkgX64.dsc | 20 ++++++++++++++++++++
>>>  OvmfPkg/OvmfPkgX64.fdf |  3 +++
>>>  2 files changed, 23 insertions(+)
>>
>> Is there any particular reason to exclude the Ia32 and Ia32X64 builds?
>>
>> If not, then please modify all three sets of dsc/fdf files identically.
> 
> I'd rather keep this as a TODO item for now, since we are not close to
> a final version, and it's annoying to have to fix each files etc..

OK.

Thanks!
Laszlo

Re: [Qemu-devel] [edk2] [PATCH 4/7] ovmf: link with Tcg2Pei module

[Laszlo Ersek]

On 03/01/18 16:08, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 26, 2018 at 10:38 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> This module will initialize TPM device, measure reported FVs and BIOS
>>> version.
>>>
>>> CC: Laszlo Ersek <lersek@redhat.com>
>>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  OvmfPkg/OvmfPkgX64.dsc | 7 +++++++
>>>  OvmfPkg/OvmfPkgX64.fdf | 1 +
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index b5cbe8430f..34a7c2778e 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -279,6 +279,8 @@
>>>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
>>>  !if $(TPM2_ENABLE)
>>> +  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>>> +  HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
>>>    Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>>>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>>>  !endif
>>> @@ -647,6 +649,11 @@
>>>
>>>  !if $(TPM2_ENABLE) == TRUE
>>>    SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> +  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>>> +    <LibraryClasses>
>>> +      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>>> +      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>>> +  }
>>>  !endif
>>>
>>>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>> index dc35d0a1f7..9558142a42 100644
>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>> @@ -170,6 +170,7 @@ INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>>>  !endif
>>>  !if $(TPM2_ENABLE) == TRUE
>>>  INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> +INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>>  !endif
>>>
>>>  ################################################################################
>>>
>>
>> Would it be possible to drop SHA1 (include SHA256 only) by setting
>> PcdTpm2HashMask to value 2? Or SHA1 required for some other reason? (If
>> so please mention it in the commit message.)
>>
> 
> afaik, it's not strictly required, and apparently the support is being
> dropped. I'll remove it.
> 
> btw, now I understand your comment about read-only variable not being
> used by PEI module. I'll add a preliminary patch dropping it from
> depex ;)

OK, thanks :)

Laszlo

Re: [Qemu-devel] [edk2] [PATCH 6/7] ovmf: link with Tcg2ConfigDxe module

[Laszlo Ersek]

On 03/01/18 17:59, Stefan Berger wrote:
> On 02/26/2018 04:58 AM, Laszlo Ersek wrote:
>> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> The module allows to tweak and interact with the TPM. Note that many
>>> actions are broken due to implementation of qemu TPM (providing it's
>>> own ACPI table), and the lack of PPI implementation.
>>>
>>> CC: Laszlo Ersek <lersek@redhat.com>
>>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>   OvmfPkg/OvmfPkgX64.dsc | 2 ++
>>>   OvmfPkg/OvmfPkgX64.fdf | 1 +
>>>   2 files changed, 3 insertions(+)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index 9bd0709f98..2281bd5ff8 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -669,6 +669,8 @@
>>>        
>>> NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>>>        
>>> NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>>>     }
>>> +
>>> +  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>>>   !endif
>>>     !if $(SECURE_BOOT_ENABLE) == TRUE
>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>> index b8dd7ecae4..985404850f 100644
>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>> @@ -399,6 +399,7 @@ INF 
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>>>     !if $(TPM2_ENABLE) == TRUE
>>>   INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
>>> +INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>>>   !endif
>>>    
>>> ################################################################################
>>>
>>>
>> Please drop this patch.
>>
>> In my earlier investigation I wrote, Tcg2ConfigDxe "[p]rovides a Setup
>> TUI interface to configure the TPM. IIUC, it can also save the
>> configured TPM type for subsequent boots (see Tcg2ConfigPei.inf above)".
>>
>> The INF file itself says "This module is only for reference only, each
>> platform should have its own setup page."
>>
>> And Jiewen wrote earlier, "Tcg2ConfigPei/Dxe are platform sample driver.
>> A platform may have its own version based upon platform requirement. For
>> example, if a platform supports fTPM, it may use another Tcg2Config
>> driver."
>>
>> Given that OVMF lacks PEI-phase variable access, and that I consequently
>> suggested cloning, and seriously trimming, Tcg2ConfigPei, it makes no
>> sense to include an HII dialog that sets a variable for PEI phase
>> consumption. Also, as you say, many of the exposed operations are broken
>> due to lack of PPI support. So let's just postpone the inclusion of this
>> driver, for now.
> 
> Just FYI: The PPI support for the OS requires ACPI

OK

> and, as it is
> currently implemented, SMF where UEFI variables are manipulated.

(I assume s/SMF/SMM/)

You are correct to write "as it is currently implemented". My point in
my previous email(s) was that QEMU should generate the ACPI payload
needed by the OS, for queueing PPI opcodes (i.e. OVMF should install
QEMU's AML, *not* the sample AML code in SecurityPkg). In turn the AML
from QEMU should queue the PPI opcodes in the custom register block of
the TPM device (which is anyway the only NVRAM-emulation possibility
under SeaBIOS).

Given that the PPI opcodes will henceforth not be stored in a UEFI
variable under OVMF either, the SMM requirement in the AML falls away
completely.

Retrieving the PPI opcodes for processing from the custom MMIO register
block of the TPM device, after reboot, as opposed to reading them out of
a UEFI variable, will take custom code in OVMF. We'll get there.

> Some
> menu items in the TPM 2 menu (also TPM 1.2) also require these UEFI
> variables of the PPI interface so that UEFI can react on the menu
> choices upon re.

(I assume s/re/reboot/)

In "SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr" (which is the "Visual
Form Representation" of the dialog we're talking about), I see three
variable references. The structures for those are defined in
"SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigNvData.h":

- TCG2_CONFIGURATION_INFO
- TCG2_CONFIGURATION
- TCG2_VERSION

I think the last two are irrelevant under OVMF / QEMU (fixed version 2
TPM, and ACPI comes from QEMU -- consistent with my other comments for
the PEI phase modules).

TCG2_CONFIGURATION_INFO looks more complex, so perhaps we'll have to
scavenge its handling for OVMF. However, it seems that
TCG2_CONFIGURATION_INFO is not needed in the PEI phase.

... Understand this right: I'd be bursting from joy if OVMF had
PEI-phase r/o variable access. I got that feature working for QEMU. But
all my attempts to upstream the work failed (apparently because I'm not
willing to develop a parallel "fake" for Xen -- on Xen, NVRAM/pflash
doesn't even exist, so even if the PEI variable service existed on Xen,
it would have zero chance to return valid data.)

Laszlo

Re: [Qemu-devel] [edk2] [PATCH 6/7] ovmf: link with Tcg2ConfigDxe module

[Stefan Berger]

On 03/02/2018 06:12 AM, Laszlo Ersek wrote:
> On 03/01/18 17:59, Stefan Berger wrote:
>> On 02/26/2018 04:58 AM, Laszlo Ersek wrote:
>>> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> The module allows to tweak and interact with the TPM. Note that many
>>>> actions are broken due to implementation of qemu TPM (providing it's
>>>> own ACPI table), and the lack of PPI implementation.
>>>>
>>>> CC: Laszlo Ersek <lersek@redhat.com>
>>>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>>    OvmfPkg/OvmfPkgX64.dsc | 2 ++
>>>>    OvmfPkg/OvmfPkgX64.fdf | 1 +
>>>>    2 files changed, 3 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>> index 9bd0709f98..2281bd5ff8 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>> @@ -669,6 +669,8 @@
>>>>         
>>>> NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>>>>         
>>>> NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>>>>      }
>>>> +
>>>> +  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>>>>    !endif
>>>>      !if $(SECURE_BOOT_ENABLE) == TRUE
>>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>>> index b8dd7ecae4..985404850f 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>>> @@ -399,6 +399,7 @@ INF
>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>>>>      !if $(TPM2_ENABLE) == TRUE
>>>>    INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
>>>> +INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>>>>    !endif
>>>>     
>>>> ################################################################################
>>>>
>>>>
>>> Please drop this patch.
>>>
>>> In my earlier investigation I wrote, Tcg2ConfigDxe "[p]rovides a Setup
>>> TUI interface to configure the TPM. IIUC, it can also save the
>>> configured TPM type for subsequent boots (see Tcg2ConfigPei.inf above)".
>>>
>>> The INF file itself says "This module is only for reference only, each
>>> platform should have its own setup page."
>>>
>>> And Jiewen wrote earlier, "Tcg2ConfigPei/Dxe are platform sample driver.
>>> A platform may have its own version based upon platform requirement. For
>>> example, if a platform supports fTPM, it may use another Tcg2Config
>>> driver."
>>>
>>> Given that OVMF lacks PEI-phase variable access, and that I consequently
>>> suggested cloning, and seriously trimming, Tcg2ConfigPei, it makes no
>>> sense to include an HII dialog that sets a variable for PEI phase
>>> consumption. Also, as you say, many of the exposed operations are broken
>>> due to lack of PPI support. So let's just postpone the inclusion of this
>>> driver, for now.
>> Just FYI: The PPI support for the OS requires ACPI
> OK
>
>> and, as it is
>> currently implemented, SMF where UEFI variables are manipulated.
> (I assume s/SMF/SMM/)

correct.
>
> You are correct to write "as it is currently implemented". My point in
> my previous email(s) was that QEMU should generate the ACPI payload
> needed by the OS, for queueing PPI opcodes (i.e. OVMF should install
> QEMU's AML, *not* the sample AML code in SecurityPkg). In turn the AML
> from QEMU should queue the PPI opcodes in the custom register block of
> the TPM device (which is anyway the only NVRAM-emulation possibility
> under SeaBIOS).

We can emulate other NVRAM as well. It is a possibility and provides the 
flexibility of setting flags per implemented PPI code indicating whether 
the firmware implements the codes and sysfs entries in Linux can then 
show what is actually supported. If you write code '23' into sysfs and 
ACPI has no clue whether the firmware implements '23', then you may 
reboot for nothing and start wondering what is going on because the 
effect isn't there. Also I don't think we'll implement the same set of 
commands in both firmwares that we would want to hard-code the checking 
for implemented code in ACPI produced by the firmware.

>
> Given that the PPI opcodes will henceforth not be stored in a UEFI
> variable under OVMF either, the SMM requirement in the AML falls away
> completely.

I would not modify that code but keep what is there right now and write 
some glue code around it: Upon reboot detect what PPI code has been set, 
if any, and write it into the UEFI variable. Then existing UEFI code 
reads the variable (stemming either from OS or a menu operation) and 
acts upon it. That keeps much of the existing code unmodified, which I 
find appealing. Also the code for the menu wouldn't have to be modified.

>
> Retrieving the PPI opcodes for processing from the custom MMIO register
> block of the TPM device, after reboot, as opposed to reading them out of
> a UEFI variable, will take custom code in OVMF. We'll get there.

That custom MMIO register block could, in the worst case, be assigned a 
different purpose in a future spec...

   Stefan
>
>> Some
>> menu items in the TPM 2 menu (also TPM 1.2) also require these UEFI
>> variables of the PPI interface so that UEFI can react on the menu
>> choices upon re.
> (I assume s/re/reboot/)
>
> In "SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr" (which is the "Visual
> Form Representation" of the dialog we're talking about), I see three
> variable references. The structures for those are defined in
> "SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigNvData.h":
>
> - TCG2_CONFIGURATION_INFO
> - TCG2_CONFIGURATION
> - TCG2_VERSION
>
> I think the last two are irrelevant under OVMF / QEMU (fixed version 2
> TPM, and ACPI comes from QEMU -- consistent with my other comments for
> the PEI phase modules).
>
> TCG2_CONFIGURATION_INFO looks more complex, so perhaps we'll have to
> scavenge its handling for OVMF. However, it seems that
> TCG2_CONFIGURATION_INFO is not needed in the PEI phase.
>
> ... Understand this right: I'd be bursting from joy if OVMF had
> PEI-phase r/o variable access. I got that feature working for QEMU. But
> all my attempts to upstream the work failed (apparently because I'm not
> willing to develop a parallel "fake" for Xen -- on Xen, NVRAM/pflash
> doesn't even exist, so even if the PEI variable service existed on Xen,
> it would have zero chance to return valid data.)
>
> Laszlo
>

Re: [Qemu-devel] [edk2] [PATCH 1/7] SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib dependency

[Laszlo Ersek]

On 02/24/18 01:09, Yao, Jiewen wrote:
> Reviewed-by: Jiewen.yao@intel.com

Given that Jiewen co-maintains SecurityPkg as of commit 3db2823f1e27
("Maintainers.txt: Add Jiewen to be co-maintainer of SecurityPkg.",
2018-03-02), I pushed this patch in isolation: commit a39e72267034.

However, Marc-André, something seems to be wrong with the way you
formatted the patch. The patch email is base64 encoded (which I guess
shouldn't be a problem per se), and I had to pass "--ignore-whitespace"
to git-am. It means that the *context* lines in your patch didn't have
the correct CRLF terminators. That seems very strange because the files
being modified are fully CRLF (I checked).

The git settings that I personally recommend to edk2 contributors are
written up here:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

Also, if you could push every version of the patch set to a separate
branch in your public edk2 clone (e.g. on github, or elsewhere), that
would be great -- please just mark the repo URL and the branch name in
the cover letter of that version, when you post it.

Thanks!
Laszlo


>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> marcandre.lureau@redhat.com
>> Sent: Friday, February 23, 2018 9:23 PM
>> To: edk2-devel@lists.01.org
>> Cc: qemu-devel@nongnu.org; javierm@redhat.com; pjones@redhat.com; Yao,
>> Jiewen <jiewen.yao@intel.com>; lersek@redhat.com
>> Subject: [edk2] [PATCH 1/7] SecurityPkg/Tcg2Pei: drop Tcg2PhysicalPresenceLib
>> dependency
>>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Apparently, unnecessary. Avoids extra build dependency and churn.
>>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c   | 2 --
>>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 -
>>  2 files changed, 3 deletions(-)
>>
>> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
>> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
>> index a7ae3354b5..3758fc6a41 100644
>> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
>> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
>> @@ -18,7 +18,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>> KIND, EITHER EXPRESS OR IMPLIED.
>>  #include <IndustryStandard/UefiTcgPlatform.h>
>>  #include <Ppi/FirmwareVolumeInfo.h>
>>  #include <Ppi/FirmwareVolumeInfo2.h>
>> -#include <Ppi/LockPhysicalPresence.h>
>>  #include <Ppi/TpmInitialized.h>
>>  #include <Ppi/FirmwareVolume.h>
>>  #include <Ppi/EndOfPeiPhase.h>
>> @@ -44,7 +43,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>> KIND, EITHER EXPRESS OR IMPLIED.
>>  #include <Library/MemoryAllocationLib.h>
>>  #include <Library/ReportStatusCodeLib.h>
>>  #include <Library/ResetSystemLib.h>
>> -#include <Library/Tcg2PhysicalPresenceLib.h>
>>
>>  #define PERF_ID_TCG2_PEI  0x3080
>>
>> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>> index f7b85444d9..bc910c3baf 100644
>> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>> @@ -58,7 +58,6 @@
>>    PerformanceLib
>>    MemoryAllocationLib
>>    ReportStatusCodeLib
>> -  Tcg2PhysicalPresenceLib
>>    ResetSystemLib
>>
>>  [Guids]
>> --
>> 2.16.1.73.g5832b7e9f2
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [Qemu-devel] [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Marc-André Lureau]

Hi

On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish <afish@apple.com> wrote:
>
>
>> On Feb 23, 2018, at 5:23 AM, marcandre.lureau@redhat.com wrote:
>>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Without this hack, GetNextHob() loops infinitely with the next patch.
>> I don't understand the reason.
>>
>> The loop is triggered by the GetFirstGuidHob (&gTpmErrorHobGuid) call.
>>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> MdePkg/Library/PeiHobLib/HobLib.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c b/MdePkg/Library/PeiHobLib/HobLib.c
>> index 5c0eeb992f..ed3c5fbd6d 100644
>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>> @@ -89,6 +89,10 @@ GetNextHob (
>>     if (Hob.Header->HobType == Type) {
>>       return Hob.Raw;
>>     }
>> +    if (GET_HOB_LENGTH (HobStart) == 0) {
>
> As Laszlo points out this error condition is likely memory corruption. Thus it would be better to check for all know illegal values?
>
> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>

Thanks, I have adjusted the check.

With manual calls and printf (I don't know  a better way to debug ovmf
;), I try to locate the issue. It's somehow related to
RegisterForShadow(). The "corruption" seems to happen during the
second call. After the
PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
function, it fails (with the same arguments). Right after it succeeds
again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
suppose there is some kind of wrapping code, but I fail to find where.
Any idea?

thanks for your help

-- 
Marc-André Lureau

Re: [Qemu-devel] [edk2] [PATCH 5/7] ovmf: link with Tcg2Dxe module

[Marc-André Lureau]

Hi

On Mon, Feb 26, 2018 at 10:50 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> This module measures and log the boot environment. It also produces
>> the Tcg2 protocol, which allows for example to read the log from OS:
>>
>> [    0.000000] efi: EFI v2.70 by EDK II
>> [    0.000000] efi:  SMBIOS=0x3fa1f000  ACPI=0x3fbb6000  ACPI 2.0=0x3fbb6014  MEMATTR=0x3e7d4318  TPMEventLog=0x3db21018
>>
>> $ python chipsec_util.py tpm parse_log binary_bios_measurements
>>
>> [CHIPSEC] Version 1.3.5.dev2
>> [CHIPSEC] API mode: using OS native API (not using CHIPSEC kernel module)
>> [CHIPSEC] Executing command 'tpm' with args ['parse_log', '/tmp/binary_bios_measurements']
>>
>> PCR: 0        type: EV_S_CRTM_VERSION                 size: 0x2       digest: 1489f923c4dca729178b3e3233458550d8dddf29
>>       + version:
>> PCR: 0        type: EV_EFI_PLATFORM_FIRMWARE_BLOB     size: 0x10      digest: fd39ced7c0d2a61f6830c78c7625f94826b05bcc
>>       + base: 0x820000        length: 0xe0000
>> PCR: 0        type: EV_EFI_PLATFORM_FIRMWARE_BLOB     size: 0x10      digest: 39ebc6783b72bc1e73c7d5bcfeb5f54a3f105d4c
>>       + base: 0x900000        length: 0xa00000
>> PCR: 7        type: EV_EFI_VARIABLE_DRIVER_CONFIG     size: 0x35      digest: 57cd4dc19442475aa82743484f3b1caa88e142b8
>> PCR: 7        type: EV_EFI_VARIABLE_DRIVER_CONFIG     size: 0x24      digest: 9b1387306ebb7ff8e795e7be77563666bbf4516e
>> PCR: 7        type: EV_EFI_VARIABLE_DRIVER_CONFIG     size: 0x26      digest: 9afa86c507419b8570c62167cb9486d9fc809758
>> PCR: 7        type: EV_EFI_VARIABLE_DRIVER_CONFIG     size: 0x24      digest: 5bf8faa078d40ffbd03317c93398b01229a0e1e0
>> PCR: 7        type: EV_EFI_VARIABLE_DRIVER_CONFIG     size: 0x26      digest: 734424c9fe8fc71716c42096f4b74c88733b175e
>> PCR: 7        type: EV_SEPARATOR                      size: 0x4       digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 1        type: EV_EFI_VARIABLE_BOOT              size: 0x3e      digest: 252f8ebb85340290b64f4b06a001742be8e5cab6
>> PCR: 1        type: EV_EFI_VARIABLE_BOOT              size: 0x6e      digest: 22a4f6ee9af6dba01d3528deb64b74b582fc182b
>> PCR: 1        type: EV_EFI_VARIABLE_BOOT              size: 0x80      digest: b7811d5bf30a7efd4e385c6179fe10d9290bb9e8
>> PCR: 1        type: EV_EFI_VARIABLE_BOOT              size: 0x84      digest: 425e502c24fc924e231e0a62327b6b7d1f704573
>> PCR: 1        type: EV_EFI_VARIABLE_BOOT              size: 0x9a      digest: 0b5d2c98ac5de6148a4a1490ff9d5df69039f04e
>> PCR: 1        type: EV_EFI_VARIABLE_BOOT              size: 0xbd      digest: 20bd5f402271d57a88ea314fe35c1705956b1f74
>> PCR: 1        type: EV_EFI_VARIABLE_BOOT              size: 0x88      digest: df5d6605cb8f4366d745a8464cfb26c1efdc305c
>> PCR: 4        type: EV_EFI_ACTION                     size: 0x28      digest: cd0fdb4531a6ec41be2753ba042637d6e5f7f256
>> PCR: 0        type: EV_SEPARATOR                      size: 0x4       digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 1        type: EV_SEPARATOR                      size: 0x4       digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 2        type: EV_SEPARATOR                      size: 0x4       digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 3        type: EV_SEPARATOR                      size: 0x4       digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 4        type: EV_SEPARATOR                      size: 0x4       digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 5        type: EV_SEPARATOR                      size: 0x4       digest: 9069ca78e7450a285173431b3e52c5c25299e473
>>
>> $ tpm2_pcrlist
>> sha1 :
>>   0  : 35bd1786b6909daad610d7598b1d620352d33b8a
>>   1  : ec0511e860206e0af13c31da2f9e943fb6ca353d
>>   2  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>>   3  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>>   4  : 45a323382bd933f08e7f0e256bc8249e4095b1ec
>>   5  : d16d7e629fd8d08ca256f9ad3a3a1587c9e6cc1b
>>   6  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>>   7  : 518bd167271fbb64589c61e43d8c0165861431d8
>>   8  : 0000000000000000000000000000000000000000
>>   9  : 0000000000000000000000000000000000000000
>>   10 : 0000000000000000000000000000000000000000
>>   11 : 0000000000000000000000000000000000000000
>>   12 : 0000000000000000000000000000000000000000
>>   13 : 0000000000000000000000000000000000000000
>>   14 : 0000000000000000000000000000000000000000
>>   15 : 0000000000000000000000000000000000000000
>>   16 : 0000000000000000000000000000000000000000
>>   17 : ffffffffffffffffffffffffffffffffffffffff
>>   18 : ffffffffffffffffffffffffffffffffffffffff
>>   19 : ffffffffffffffffffffffffffffffffffffffff
>>   20 : ffffffffffffffffffffffffffffffffffffffff
>>   21 : ffffffffffffffffffffffffffffffffffffffff
>>   22 : ffffffffffffffffffffffffffffffffffffffff
>>   23 : 0000000000000000000000000000000000000000
>> sha256 :
>>   0  : 9ae903dbae3357ac00d223660bac19ea5c021499a56201104332ab966631ce2c
>>   1  : acc611d90245cf04e77b0ca94901f90e7fa54770f0426f53c3049b532243d1b8
>>   2  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
>>   3  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
>>   4  : 7a94ffe8a7729a566d3d3c577fcb4b6b1e671f31540375f80eae6382ab785e35
>>   5  : a5ceb755d043f32431d63e39f5161464620a3437280494b5850dc1b47cc074e0
>>   6  : 3d458cfe55cc03ea1f443f1562beec8df51c75e14a9fcf9a7234a13f198e7969
>>   7  : 65caf8dd1e0ea7a6347b635d2b379c93b9a1351edc2afc3ecda700e534eb3068
>>   8  : 0000000000000000000000000000000000000000000000000000000000000000
>>   9  : 0000000000000000000000000000000000000000000000000000000000000000
>>   10 : 0000000000000000000000000000000000000000000000000000000000000000
>>   11 : 0000000000000000000000000000000000000000000000000000000000000000
>>   12 : 0000000000000000000000000000000000000000000000000000000000000000
>>   13 : 0000000000000000000000000000000000000000000000000000000000000000
>>   14 : 0000000000000000000000000000000000000000000000000000000000000000
>>   15 : 0000000000000000000000000000000000000000000000000000000000000000
>>   16 : 0000000000000000000000000000000000000000000000000000000000000000
>>   17 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>>   18 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>>   19 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>>   20 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>>   21 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>>   22 : ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>>   23 : 0000000000000000000000000000000000000000000000000000000000000000
>> sha384 :
>>
>> The PhysicalPresenceLib is required, it sets some variables, but the
>> firmware doesn't act on it yet.
>>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  OvmfPkg/OvmfPkgX64.dsc | 15 +++++++++++++++
>>  OvmfPkg/OvmfPkgX64.fdf |  4 ++++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 34a7c2778e..9bd0709f98 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -213,6 +213,8 @@
>>  !if $(TPM2_ENABLE) == TRUE
>>    Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
>>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>> +  Tcg2PhysicalPresenceLib|SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
>> +  Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
>>  !endif
>>
>>  [LibraryClasses.common]
>
> I'd prefer to review this part in v2, once the @@ hunk headers are set
> up at your end ("xfuncname").
>
>> @@ -364,6 +366,11 @@
>>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>>    MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
>> +!if $(TPM2_ENABLE)
>> +  HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
>> +  Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
>
> Can we drop TPM12? "Tcg2Dxe.inf" does not seem to depend on this lib class.
>

indeed

>> +  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>> +!endif
>>
>>  [LibraryClasses.common.UEFI_APPLICATION]
>>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
>> @@ -654,6 +661,14 @@
>>        NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>>        NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>>    }
>> +
>> +  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
>> +    <LibraryClasses>
>> +      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
>
> Can you please explain why Tpm2DeviceLib has to be resolved differently
> for DXE_DRIVER modules in general (see above) and for "Tcg2Dxe.inf"
> specifically?
>
> Hmmm... Looks like "Tpm2DeviceLibTcg2.inf" implements the APIs via the
> TPM2 protocol. Whereas "Tcg2Dxe.inf" itself provides that protocol, so
> obviously it cannot rely on the protocol; it has to access the device,
> which is done with the help of "Tpm2DeviceLibRouterDxe.inf" and the
> "Tpm2InstanceLibDTpm.inf" that is plugged in via NULL resolution below.
> Is that about correct?
>
> If so, can you please document it in the commit message?

I followed the SecurityPkg.dsc, and tried some variants. This router
pattern can be found in other places, unfortunately, I can't explain
it. I can copy&paste your explanation if that helps.

>
>> +      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
>> +      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>
> Again -- is SHA1 required?

The linux kernel doesn't yet read the EFI_TCG2_EVENT_LOG_FORMAT_TCG_2,
which is required for crypto-agile log. In fact, only upcoming 4.16
adds support EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2...

Any major drawback in keeping sha1 enabled? (same for Pei)

>> +      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>> +  }
>>  !endif
>>
>>  !if $(SECURE_BOOT_ENABLE) == TRUE
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index 9558142a42..b8dd7ecae4 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -397,6 +397,10 @@ INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>>  INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>>  !endif
>>
>> +!if $(TPM2_ENABLE) == TRUE
>> +INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
>> +!endif
>> +
>>  ################################################################################
>>
>>  [FV.FVMAIN_COMPACT]
>>
>
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Laszlo Ersek]

On 03/05/18 15:05, Marc-André Lureau wrote:
> Hi
>
> On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish <afish@apple.com> wrote:
>>
>>
>>> On Feb 23, 2018, at 5:23 AM, marcandre.lureau@redhat.com wrote:
>>>
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Without this hack, GetNextHob() loops infinitely with the next
>>> patch. I don't understand the reason.
>>>
>>> The loop is triggered by the GetFirstGuidHob (&gTpmErrorHobGuid)
>>> call.
>>>
>>> CC: Laszlo Ersek <lersek@redhat.com>
>>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>> MdePkg/Library/PeiHobLib/HobLib.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c b/MdePkg/Library/PeiHobLib/HobLib.c
>>> index 5c0eeb992f..ed3c5fbd6d 100644
>>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>>> @@ -89,6 +89,10 @@ GetNextHob (
>>>     if (Hob.Header->HobType == Type) {
>>>       return Hob.Raw;
>>>     }
>>> +    if (GET_HOB_LENGTH (HobStart) == 0) {
>>
>> As Laszlo points out this error condition is likely memory
>> corruption. Thus it would be better to check for all know illegal
>> values?
>>
>> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>>
>
> Thanks, I have adjusted the check.
>
> With manual calls and printf (I don't know  a better way to debug ovmf
> ;),

Well that's how I generally debug it too :)

> I try to locate the issue. It's somehow related to
> RegisterForShadow(). The "corruption" seems to happen during the
> second call. After the
> PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
> calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
> function, it fails (with the same arguments). Right after it succeeds
> again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
> suppose there is some kind of wrapping code, but I fail to find where.
> Any idea?

This sounds helpful. I don't know what the problem is, but I can
elaborate on your details a bit; perhaps someone else will have more
ideas.

Apparently there is a PEI service called RegisterForShadow().
("Apparently", because I've never seen, let alone written, a PEIM
calling this service.) The service is specified in the PI spec, which is
quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:

> /**
>   Register a PEIM so that it will be shadowed and called again.
>
>   This service registers a file handle so that after memory is
>   available, the PEIM will be re-loaded into permanent memory and
>   re-initialized. The PEIM registered this way will always be
>   initialized twice. The first time, this function call will
>   return EFI_SUCCESS. The second time, this function call will
>   return EFI_ALREADY_STARTED. Depending on the order in which
>   PEIMs are dispatched, the PEIM making this call may be
>   initialized after permanent memory is installed, even the first
>   time.
>
>   @param  FileHandle            PEIM's file handle. Must be the currently
>                                 executing PEIM.
>
>   @retval EFI_SUCCESS           The PEIM was successfully registered for
>                                 shadowing.
>   @retval EFI_ALREADY_STARTED   The PEIM was previously
>                                 registered for shadowing.
>   @retval EFI_NOT_FOUND         The FileHandle does not refer to a
>                                 valid file handle.
>
> **/
> typedef
> EFI_STATUS
> (EFIAPI *EFI_PEI_REGISTER_FOR_SHADOW)(
>   IN  EFI_PEI_FILE_HANDLE FileHandle
>   );

PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
like RAM) for stack & heap; whatever HOBs they produce are stored in
"temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
(basically it programs the memory controller and publishes the RAM
ranges). In turn the PEI core "migrates" PEIMs from temporary to
permanent RAM, including the HOB list.

Before the temporary RAM migration (when still executing in place from
flash), PEIMs cannot have writeable global variables. For example,
dynamic PCDs are also maintained in a HOB (the PCD HOB).

A PEIM normally cannot (and shouldn't) tell whether it is dispatched
before or after permanent RAM is published. If needed, a PEIM can
advertise that it depends on permanent RAM (for example because it needs
a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
its DEPEX.

Finally, it seems like a PEIM can also express, "I'm fine with being
dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
tell me whichever it is". Apparently, if the PEIM is dispatched from
flash (before permanent RAM is available), its call to
RegisterForShadow() returns EFI_SUCCESS (and then its entry point
function will be invoked for a 2nd time, after the temp RAM migration).
And when a PEIM is dispatched from RAM (either for the very first time,
or for the second time, after being dispatched from flash first), the
same call returns EFI_ALREADY_STARTED.

Honestly, I'm unsure what this is good for (both in general, and
specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
doing the measurements (which makes sense); I just wonder what exactly
it does so that it cannot simply specify
"gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.

I do see that the (!mImageInMemory) branch contains the TPM
initialization code. However, as the PI spec itself says, it is not
guaranteed that a PEIM will be dispatched from XIP (i.e., before
permanent RAM) *at all*. So it's not clear to me how "business
functionality" can depend on XIP.


Now, OVMF is a bit different wrt. "memory controller programming" -- it
runs on virtual platforms where programming the memory controllers is
unnecessary. What happens is, instead, that only the SEC phase runs from
flash (XIP), and it decompresses even the PEI firmware volume to normal
RAM. The phase where PEIMs "run from flash" (XIP) still exists, of
course, except they actually run from RAM -- so, for example, they have
writeable global variables right off the bat. Perhaps this interferes
somehow with the RegisterForShadow() service and/or how PEIMs expect
that service to work.

Regarding the PEIM entry point -- the PEIMs' "own" entry point functions
are always wrapped.

- The outermost (common) entry point function is called
  _ModuleEntryPoint(). It is declared in
  "MdePkg/Include/Library/PeimEntryPoint.h". This is what the PEI core
  calls when dispatching a PEIM.

- Individual PEIMs add the PeimEntryPoint lib class to their INF files,
  under [LibraryClasses]. The implementation is in
  "MdePkg/Library/PeimEntryPoint". In particular, the function calls
  ProcessLibraryConstructorList().

- The build tools generate the source code for
  ProcessLibraryConstructorList(); based on the (recursively determined)
  library instance dependency tree. (Search the Build subdirectory for
  "AutoGen.c" files.) So, before the PEIM's actual entry point is
  called, the lib instances' CONSTRUCTOR functions are called, in the
  right order, so that the PEIM is entered with all libraries "ready to
  use".

I guess that, when Tcg2Pei is dispatched for the 2nd time, from
permanent RAM, the first (successful) GetFirstGuidHob() call that you
see occurs like this, from generated code, as part of library
construction. Some library instance's constructor function calls
GetFirstGuidHob(), successfully.

The corruption could somehow be related to the HOB list's migration from
temp RAM to permanent RAM. The OVMF debug log should contain something
like this:

> Temp Stack : BaseAddress=0x818000 Length=0x8000
> Temp Heap  : BaseAddress=0x810000 Length=0x8000
> Total temporary memory:    65536 bytes.
>   temporary memory stack ever used:       28656 bytes.
>   temporary memory heap used for HobList: 6056 bytes.
>   temporary memory heap occupied by memory pages: 0 bytes.

Implying that, before the temp RAM migration, the HOB list consumed ~6KB
in total of the 32KB temp RAM heap. It seems unlikely that we run out of
temp RAM heap.

Hopefully this helps you proceed with the debugging... Corrections are
welcome too, obviously!

Thanks,
Laszlo

Re: [Qemu-devel] [edk2] [PATCH 5/7] ovmf: link with Tcg2Dxe module

[Laszlo Ersek]

On 03/05/18 16:45, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 26, 2018 at 10:50 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:

>>> +  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
>>> +    <LibraryClasses>
>>> +      Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
>>
>> Can you please explain why Tpm2DeviceLib has to be resolved differently
>> for DXE_DRIVER modules in general (see above) and for "Tcg2Dxe.inf"
>> specifically?
>>
>> Hmmm... Looks like "Tpm2DeviceLibTcg2.inf" implements the APIs via the
>> TPM2 protocol. Whereas "Tcg2Dxe.inf" itself provides that protocol, so
>> obviously it cannot rely on the protocol; it has to access the device,
>> which is done with the help of "Tpm2DeviceLibRouterDxe.inf" and the
>> "Tpm2InstanceLibDTpm.inf" that is plugged in via NULL resolution below.
>> Is that about correct?
>>
>> If so, can you please document it in the commit message?
> 
> I followed the SecurityPkg.dsc, and tried some variants. This router
> pattern can be found in other places, unfortunately, I can't explain
> it. I can copy&paste your explanation if that helps.

Yes, I think we should capture it in the commit message.

I'd also like to make another attempt at explaining it to you (and me as
well :) ).


* We have a library class called Tpm2DeviceLib -- this is basically the
set of APIs declared in "SecurityPkg/Include/Library/Tpm2DeviceLib.h".
Its leading comment says "This library abstract how to access TPM2
hardware device".

There are two *sets* of APIs in "Tpm2DeviceLib.h":

(a) functions that deal with the TPM2 device:
    - Tpm2RequestUseTpm(),
    - Tpm2SubmitCommand()

    This set of APIs is supposed to be used by clients that *consume*
    the TPM2 device abstraction.

(b) the function Tpm2RegisterTpm2DeviceLib(), which is supposed to be
    used by *providers* of various TPM2 device abstractions.


* Then, we have two implementations (instances) of the Tpm2DeviceLib class:
(1) SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
(2) SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf

(1) The first library instance ("Tpm2DeviceLibTcg2.inf") implements the
APIs listed under (a), and it does not implement (b) -- see
EFI_UNSUPPORTED. In other words, this lib instance is strictly meant for
drivers that *consume* the TPM2 device abstraction. And, the (a) group
of APIs is implemented by forwarding the requests to the TCG2 protocol.

The idea here is that all the drivers that consume the TPM2 abstraction
do not have to be statically linked with a large TPM2 device library
instance; instead they are only linked (statically) with this "thin"
library instance, and all the actual work is delegated to whichever
driver that provides the singleton TCG2 protocol.

(2) The second library instance ("Tpm2DeviceLibRouterDxe.inf") is meant
for the driver that offers (produces) the TCG2 protocol. This lib
instance implements both (a) and (b) API groups.


* Here's how things fit together:

(i) The "SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf"
library instance (which has no lib class) is linked into "Tcg2Dxe.inf"
via NULL class resolution. This simply means that before the
"Tcg2Dxe.inf" entry point function is entered, the constructor function
of "Tpm2InstanceLibDTpm.inf" will be called.

(ii) This Tpm2InstanceLibDTpmConstructor() function calls API (b), and
registers its own actual TPM2 command implementation with the
"Tpm2DeviceLibRouter" library instance (also linked into the Tcg2Dxe
driver). This provides the back-end for the API set (a).

       TCG2 protocol provider (Tcg2Dxe.inf driver) launches
                    |
                    v
  NULL class: Tpm2InstanceLibDTpm instance construction
                    |
                    v
  Tpm2DeviceLib class: Tpm2DeviceLibRouter instance
         backend registration for API set (a)

(iii) The Tcg2Dxe driver exposes the TCG2 protocol.

(iv) A TPM2 consumer calls API set (a) via lib instance (1). Such calls
land in Tcg2Dxe, via the protocol.

(v) Tcg2Dxe serves the protocol request by forwarding it to API set (a)
from lib instance (2).

(vi) Those functions call the "backend" functions registered by
Tpm2DeviceLibDTpm in step (ii).

     TPM 2 consumer driver
              |
              v
Tpm2DeviceLib class: Tpm2DeviceLibTcg2 instance
              |
              v
       TCG2 protocol interface
              |
              v
TCG2 protocol provider: Tcg2Dxe.inf driver
              |
              v
Tpm2DeviceLib class: Tpm2DeviceLibRouter instance
              |
              v
   NULL class: Tpm2InstanceLibDTpm instance
      (via earlier registration)
              |
              v
     TPM2 chip (actual hardware)


* So that is the "router" pattern in edk2. Namely,

- Consumers of an abstraction use a thin library instance.

- The thin library instance calls a firmware-global (singleton) service,
  i.e. a PPI (in the PEI phase) or protocol (in the DXE phase).

- The PEIM providing the PPI, or the DXE driver providing the protocol,
  don't themselves implement the actual service either. Instead they
  offer a "registration" service too, and they only connect the incoming
  "consumer" calls to the earlier registered back-end(s).

- The "registration service", for back-ends to use, may take various
  forms.

  It can be exposed globally to the rest of the firmware, as
  another member function of the PPI / protocol structure. Then backends
  can be provided by separate PEIMs / DXE drivers.

  Or else, the registration service can be exposed as just another
  library API. In this case, the backends are provided as NULL class
  library instances, and a platform  DSC file links them into the PEIM /
  DXE driver via NULL class resolutions. The backend lib instances call
  the registration service in their own respective constructor
  functions.

>>
>>> +      NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
>>> +      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>>
>> Again -- is SHA1 required?
> 
> The linux kernel doesn't yet read the EFI_TCG2_EVENT_LOG_FORMAT_TCG_2,
> which is required for crypto-agile log. In fact, only upcoming 4.16
> adds support EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2...
> 
> Any major drawback in keeping sha1 enabled? (same for Pei)

Likely not a major drawback.

I just remember that, for a while now, it's been "time to walk, but not
run, to the fire exits. You don't see smoke, but the fire alarms have
gone off" --
<https://www.schneier.com/blog/archives/2005/02/cryptanalysis_o.html>.
Note the year. :)

So I prefer avoiding SHA1 in new things, for general hygiene. If we
can't avoid SHA1, I'm OK with pulling it in, though.

Thanks!
Laszlo

Re: [Qemu-devel] [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Andrew Fish]

> On Mar 5, 2018, at 10:22 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 03/05/18 15:05, Marc-André Lureau wrote:
>> Hi
>> 
>> On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish <afish@apple.com> wrote:
>>> 
>>> 
>>>> On Feb 23, 2018, at 5:23 AM, marcandre.lureau@redhat.com wrote:
>>>> 
>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> 
>>>> Without this hack, GetNextHob() loops infinitely with the next
>>>> patch. I don't understand the reason.
>>>> 
>>>> The loop is triggered by the GetFirstGuidHob (&gTpmErrorHobGuid)
>>>> call.
>>>> 
>>>> CC: Laszlo Ersek <lersek@redhat.com>
>>>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>> MdePkg/Library/PeiHobLib/HobLib.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c b/MdePkg/Library/PeiHobLib/HobLib.c
>>>> index 5c0eeb992f..ed3c5fbd6d 100644
>>>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>>>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>>>> @@ -89,6 +89,10 @@ GetNextHob (
>>>>    if (Hob.Header->HobType == Type) {
>>>>      return Hob.Raw;
>>>>    }
>>>> +    if (GET_HOB_LENGTH (HobStart) == 0) {
>>> 
>>> As Laszlo points out this error condition is likely memory
>>> corruption. Thus it would be better to check for all know illegal
>>> values?
>>> 
>>> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>>> 
>> 
>> Thanks, I have adjusted the check.
>> 
>> With manual calls and printf (I don't know  a better way to debug ovmf
>> ;),
> 
> Well that's how I generally debug it too :)
> 
>> I try to locate the issue. It's somehow related to
>> RegisterForShadow(). The "corruption" seems to happen during the
>> second call. After the
>> PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
>> calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
>> function, it fails (with the same arguments). Right after it succeeds
>> again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
>> suppose there is some kind of wrapping code, but I fail to find where.
>> Any idea?
> 
> This sounds helpful. I don't know what the problem is, but I can
> elaborate on your details a bit; perhaps someone else will have more
> ideas.
> 
> Apparently there is a PEI service called RegisterForShadow().
> ("Apparently", because I've never seen, let alone written, a PEIM
> calling this service.) The service is specified in the PI spec, which is
> quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:
> 

In the "olden days" the PEI Core did not shadow PEIMs. There were a few PEIMs that had some hacky code to shadow themselves into memory. We thought of making it a library, but there was not a clean way to detect if the PEIM was shadowed. So we ended up adding the PEI Service. You could also use RegisterForShadow with a DEPEX of gEfiPeiMemoryDiscoveredPpiGuid to cause your PEIM to get shadowed even if the PEI Core did not support automatically shadowing PEIMs after memory showed up. 

Anyway the RegisterForShadow can cause the entry point for the PEIM to get called again, and if there is a bug handling that I imagine bad things can happen. 

There are plenty examples of RegisterForShadow usage in the edk2. Maybe looking at how other PEIMs are using it would be helpful. 

(master)>git grep ".RegisterForShadow"
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c:852:    Status = (**PeiServices).RegisterForShadow(FileHandle);
SecurityPkg/Tcg/TcgPei/TcgPei.c:776:    Status = (**PeiServices).RegisterForShadow(FileHandle);
SecurityPkg/Tcg/TrEEPei/TrEEPei.c:616:    Status = (**PeiServices).RegisterForShadow(FileHandle);
(master)>git grep "PeiServicesRegisterForShadow"
EmulatorPkg/Library/SecPeiServicesLib/PeiServicesLib.c:423:PeiServicesRegisterForShadow (
FatPkg/FatPei/FatLiteApi.c:258:  Status = PeiServicesRegisterForShadow (FileHandle);
IntelFrameworkModulePkg/Bus/Isa/IsaFloppyPei/FloppyPeim.c:1725:  Status = PeiServicesRegisterForShadow (FileHandle);
MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.c:1199:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c:44:  Status = PeiServicesRegisterForShadow (FileHandle);
MdeModulePkg/Bus/Pci/SdMmcPciHcPei/SdMmcPciHcPei.c:103:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/UfsPciHcPei/UfsPciHcPei.c:92:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.c:121:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.c:1462:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcBlockIoPei.c:693:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Sd/SdBlockIoPei/SdBlockIoPei.c:540:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c:1147:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Usb/UsbBotPei/UsbBotPeim.c:96:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Usb/UsbBusPei/UsbPeim.c:140:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:87:    Status = PeiServicesRegisterForShadow (FileHandle);
MdeModulePkg/Universal/Disk/CdExpressPei/PeiCdExpress.c:44:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
MdePkg/Include/Library/PeiServicesLib.h:464:PeiServicesRegisterForShadow (
MdePkg/Library/PeiServicesLib/PeiServicesLib.c:474:PeiServicesRegisterForShadow (
QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.c:595:    PeiServicesRegisterForShadow (FileHandle);
QuarkSocPkg/QuarkSouthCluster/Usb/Ohci/Pei/OhcPeim.c:1302:  if (!EFI_ERROR (PeiServicesRegisterForShadow (FileHandle))) {
UefiCpuPkg/CpuIoPei/CpuIoPei.c:898:  Status = PeiServicesRegisterForShadow (FileHandle);


Thanks,

Andrew Fish


>> /**
>>  Register a PEIM so that it will be shadowed and called again.
>> 
>>  This service registers a file handle so that after memory is
>>  available, the PEIM will be re-loaded into permanent memory and
>>  re-initialized. The PEIM registered this way will always be
>>  initialized twice. The first time, this function call will
>>  return EFI_SUCCESS. The second time, this function call will
>>  return EFI_ALREADY_STARTED. Depending on the order in which
>>  PEIMs are dispatched, the PEIM making this call may be
>>  initialized after permanent memory is installed, even the first
>>  time.
>> 
>>  @param  FileHandle            PEIM's file handle. Must be the currently
>>                                executing PEIM.
>> 
>>  @retval EFI_SUCCESS           The PEIM was successfully registered for
>>                                shadowing.
>>  @retval EFI_ALREADY_STARTED   The PEIM was previously
>>                                registered for shadowing.
>>  @retval EFI_NOT_FOUND         The FileHandle does not refer to a
>>                                valid file handle.
>> 
>> **/
>> typedef
>> EFI_STATUS
>> (EFIAPI *EFI_PEI_REGISTER_FOR_SHADOW)(
>>  IN  EFI_PEI_FILE_HANDLE FileHandle
>>  );
> 
> PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
> RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
> like RAM) for stack & heap; whatever HOBs they produce are stored in
> "temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
> (basically it programs the memory controller and publishes the RAM
> ranges). In turn the PEI core "migrates" PEIMs from temporary to
> permanent RAM, including the HOB list.
> 
> Before the temporary RAM migration (when still executing in place from
> flash), PEIMs cannot have writeable global variables. For example,
> dynamic PCDs are also maintained in a HOB (the PCD HOB).
> 
> A PEIM normally cannot (and shouldn't) tell whether it is dispatched
> before or after permanent RAM is published. If needed, a PEIM can
> advertise that it depends on permanent RAM (for example because it needs
> a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
> its DEPEX.
> 
> Finally, it seems like a PEIM can also express, "I'm fine with being
> dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
> tell me whichever it is". Apparently, if the PEIM is dispatched from
> flash (before permanent RAM is available), its call to
> RegisterForShadow() returns EFI_SUCCESS (and then its entry point
> function will be invoked for a 2nd time, after the temp RAM migration).
> And when a PEIM is dispatched from RAM (either for the very first time,
> or for the second time, after being dispatched from flash first), the
> same call returns EFI_ALREADY_STARTED.
> 
> Honestly, I'm unsure what this is good for (both in general, and
> specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
> doing the measurements (which makes sense); I just wonder what exactly
> it does so that it cannot simply specify
> "gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.
> 
> I do see that the (!mImageInMemory) branch contains the TPM
> initialization code. However, as the PI spec itself says, it is not
> guaranteed that a PEIM will be dispatched from XIP (i.e., before
> permanent RAM) *at all*. So it's not clear to me how "business
> functionality" can depend on XIP.
> 
> 
> Now, OVMF is a bit different wrt. "memory controller programming" -- it
> runs on virtual platforms where programming the memory controllers is
> unnecessary. What happens is, instead, that only the SEC phase runs from
> flash (XIP), and it decompresses even the PEI firmware volume to normal
> RAM. The phase where PEIMs "run from flash" (XIP) still exists, of
> course, except they actually run from RAM -- so, for example, they have
> writeable global variables right off the bat. Perhaps this interferes
> somehow with the RegisterForShadow() service and/or how PEIMs expect
> that service to work.
> 
> Regarding the PEIM entry point -- the PEIMs' "own" entry point functions
> are always wrapped.
> 
> - The outermost (common) entry point function is called
>  _ModuleEntryPoint(). It is declared in
>  "MdePkg/Include/Library/PeimEntryPoint.h". This is what the PEI core
>  calls when dispatching a PEIM.
> 
> - Individual PEIMs add the PeimEntryPoint lib class to their INF files,
>  under [LibraryClasses]. The implementation is in
>  "MdePkg/Library/PeimEntryPoint". In particular, the function calls
>  ProcessLibraryConstructorList().
> 
> - The build tools generate the source code for
>  ProcessLibraryConstructorList(); based on the (recursively determined)
>  library instance dependency tree. (Search the Build subdirectory for
>  "AutoGen.c" files.) So, before the PEIM's actual entry point is
>  called, the lib instances' CONSTRUCTOR functions are called, in the
>  right order, so that the PEIM is entered with all libraries "ready to
>  use".
> 
> I guess that, when Tcg2Pei is dispatched for the 2nd time, from
> permanent RAM, the first (successful) GetFirstGuidHob() call that you
> see occurs like this, from generated code, as part of library
> construction. Some library instance's constructor function calls
> GetFirstGuidHob(), successfully.
> 
> The corruption could somehow be related to the HOB list's migration from
> temp RAM to permanent RAM. The OVMF debug log should contain something
> like this:
> 
>> Temp Stack : BaseAddress=0x818000 Length=0x8000
>> Temp Heap  : BaseAddress=0x810000 Length=0x8000
>> Total temporary memory:    65536 bytes.
>>  temporary memory stack ever used:       28656 bytes.
>>  temporary memory heap used for HobList: 6056 bytes.
>>  temporary memory heap occupied by memory pages: 0 bytes.
> 
> Implying that, before the temp RAM migration, the HOB list consumed ~6KB
> in total of the 32KB temp RAM heap. It seems unlikely that we run out of
> temp RAM heap.
> 
> Hopefully this helps you proceed with the debugging... Corrections are
> welcome too, obviously!
> 
> Thanks,
> Laszlo

Re: [Qemu-devel] [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Brian J. Johnson]

On 03/05/2018 12:22 PM, Laszlo Ersek wrote:
> PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
> RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
> like RAM) for stack & heap; whatever HOBs they produce are stored in
> "temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
> (basically it programs the memory controller and publishes the RAM
> ranges). In turn the PEI core "migrates" PEIMs from temporary to
> permanent RAM, including the HOB list.
> 
> Before the temporary RAM migration (when still executing in place from
> flash), PEIMs cannot have writeable global variables. For example,
> dynamic PCDs are also maintained in a HOB (the PCD HOB).
> 
> A PEIM normally cannot (and shouldn't) tell whether it is dispatched
> before or after permanent RAM is published. If needed, a PEIM can
> advertise that it depends on permanent RAM (for example because it needs
> a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
> its DEPEX.
> 
> Finally, it seems like a PEIM can also express, "I'm fine with being
> dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
> tell me whichever it is". Apparently, if the PEIM is dispatched from
> flash (before permanent RAM is available), its call to
> RegisterForShadow() returns EFI_SUCCESS (and then its entry point
> function will be invoked for a 2nd time, after the temp RAM migration).
> And when a PEIM is dispatched from RAM (either for the very first time,
> or for the second time, after being dispatched from flash first), the
> same call returns EFI_ALREADY_STARTED.
> 
> Honestly, I'm unsure what this is good for (both in general, and
> specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
> doing the measurements (which makes sense); I just wonder what exactly
> it does so that it cannot simply specify
> "gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.

I haven't looked at this particular PEIM.  But one case where 
registering for shadowing is useful is for improving performance when 
running from permanent RAM, where writable global variables are 
available.  For instance, when running from flash, a ReportStatusCode 
PEIM may need to go through a slow process to locate an output hardware 
device on every PPI call.  This may involve traversing the HOB list, 
consulting other PPIs, even probing hardware addresses.  But once it's 
shadowed to RAM, it can locate the device once, then cache its address 
in a global.  Not to mention that the code itself is far, far faster 
when run from RAM vs. flash.  (That's probably a key difference between 
a real machine and a VM.)

Also, I've personally written a PEIM which saves a bunch of internal 
state in a HOB, since that's the main writable storage when running from 
flash.  That state includes pointers to other data (in flash.)  Once the 
data is all shadowed to RAM, it updates the HOB to point to the data in 
RAM.  That way it's a lot faster to access the data.

I also have other PEIMs which are constrained (via DEPEX) to run *only* 
from RAM, since they have larger memory requirements than can be 
satisfied from temporary cache-as-RAM.  That's certainly a valid 
technique as well.

RegisterForShadow() is a useful tool for making the most of the 
restricted PEI environment.  And having it standardized like this is, as 
Andrew said, a lot better than the hacks people had to use beforehand.

Thanks,
-- 
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

brian.johnson@hpe.com

Re: [Qemu-devel] [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Gao, Liming]

Laszlo:
  I also suggest to check the generated ProcessLibraryConstructorList () function. It is in the driver build output AutoGen.c code. You can check what library function be called in this function. Then, further add debug message in the library function. I suspect some function does the wrong operation and corrupt the memory. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Tuesday, March 6, 2018 2:23 AM
> To: Marc-André Lureau <marcandre.lureau@gmail.com>; Andrew Fish <afish@apple.com>
> Cc: edk2-devel@lists.01.org; Peter Jones <pjones@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; QEMU
> <qemu-devel@nongnu.org>; Javier Martinez Canillas <javierm@redhat.com>
> Subject: Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop
> 
> On 03/05/18 15:05, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish <afish@apple.com> wrote:
> >>
> >>
> >>> On Feb 23, 2018, at 5:23 AM, marcandre.lureau@redhat.com wrote:
> >>>
> >>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>
> >>> Without this hack, GetNextHob() loops infinitely with the next
> >>> patch. I don't understand the reason.
> >>>
> >>> The loop is triggered by the GetFirstGuidHob (&gTpmErrorHobGuid)
> >>> call.
> >>>
> >>> CC: Laszlo Ersek <lersek@redhat.com>
> >>> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>> ---
> >>> MdePkg/Library/PeiHobLib/HobLib.c | 4 ++++
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c b/MdePkg/Library/PeiHobLib/HobLib.c
> >>> index 5c0eeb992f..ed3c5fbd6d 100644
> >>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
> >>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
> >>> @@ -89,6 +89,10 @@ GetNextHob (
> >>>     if (Hob.Header->HobType == Type) {
> >>>       return Hob.Raw;
> >>>     }
> >>> +    if (GET_HOB_LENGTH (HobStart) == 0) {
> >>
> >> As Laszlo points out this error condition is likely memory
> >> corruption. Thus it would be better to check for all know illegal
> >> values?
> >>
> >> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
> >>
> >
> > Thanks, I have adjusted the check.
> >
> > With manual calls and printf (I don't know  a better way to debug ovmf
> > ;),
> 
> Well that's how I generally debug it too :)
> 
> > I try to locate the issue. It's somehow related to
> > RegisterForShadow(). The "corruption" seems to happen during the
> > second call. After the
> > PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
> > calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
> > function, it fails (with the same arguments). Right after it succeeds
> > again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
> > suppose there is some kind of wrapping code, but I fail to find where.
> > Any idea?
> 
> This sounds helpful. I don't know what the problem is, but I can
> elaborate on your details a bit; perhaps someone else will have more
> ideas.
> 
> Apparently there is a PEI service called RegisterForShadow().
> ("Apparently", because I've never seen, let alone written, a PEIM
> calling this service.) The service is specified in the PI spec, which is
> quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:
> 
> > /**
> >   Register a PEIM so that it will be shadowed and called again.
> >
> >   This service registers a file handle so that after memory is
> >   available, the PEIM will be re-loaded into permanent memory and
> >   re-initialized. The PEIM registered this way will always be
> >   initialized twice. The first time, this function call will
> >   return EFI_SUCCESS. The second time, this function call will
> >   return EFI_ALREADY_STARTED. Depending on the order in which
> >   PEIMs are dispatched, the PEIM making this call may be
> >   initialized after permanent memory is installed, even the first
> >   time.
> >
> >   @param  FileHandle            PEIM's file handle. Must be the currently
> >                                 executing PEIM.
> >
> >   @retval EFI_SUCCESS           The PEIM was successfully registered for
> >                                 shadowing.
> >   @retval EFI_ALREADY_STARTED   The PEIM was previously
> >                                 registered for shadowing.
> >   @retval EFI_NOT_FOUND         The FileHandle does not refer to a
> >                                 valid file handle.
> >
> > **/
> > typedef
> > EFI_STATUS
> > (EFIAPI *EFI_PEI_REGISTER_FOR_SHADOW)(
> >   IN  EFI_PEI_FILE_HANDLE FileHandle
> >   );
> 
> PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
> RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
> like RAM) for stack & heap; whatever HOBs they produce are stored in
> "temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
> (basically it programs the memory controller and publishes the RAM
> ranges). In turn the PEI core "migrates" PEIMs from temporary to
> permanent RAM, including the HOB list.
> 
> Before the temporary RAM migration (when still executing in place from
> flash), PEIMs cannot have writeable global variables. For example,
> dynamic PCDs are also maintained in a HOB (the PCD HOB).
> 
> A PEIM normally cannot (and shouldn't) tell whether it is dispatched
> before or after permanent RAM is published. If needed, a PEIM can
> advertise that it depends on permanent RAM (for example because it needs
> a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
> its DEPEX.
> 
> Finally, it seems like a PEIM can also express, "I'm fine with being
> dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
> tell me whichever it is". Apparently, if the PEIM is dispatched from
> flash (before permanent RAM is available), its call to
> RegisterForShadow() returns EFI_SUCCESS (and then its entry point
> function will be invoked for a 2nd time, after the temp RAM migration).
> And when a PEIM is dispatched from RAM (either for the very first time,
> or for the second time, after being dispatched from flash first), the
> same call returns EFI_ALREADY_STARTED.
> 
> Honestly, I'm unsure what this is good for (both in general, and
> specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
> doing the measurements (which makes sense); I just wonder what exactly
> it does so that it cannot simply specify
> "gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.
> 
> I do see that the (!mImageInMemory) branch contains the TPM
> initialization code. However, as the PI spec itself says, it is not
> guaranteed that a PEIM will be dispatched from XIP (i.e., before
> permanent RAM) *at all*. So it's not clear to me how "business
> functionality" can depend on XIP.
> 
> 
> Now, OVMF is a bit different wrt. "memory controller programming" -- it
> runs on virtual platforms where programming the memory controllers is
> unnecessary. What happens is, instead, that only the SEC phase runs from
> flash (XIP), and it decompresses even the PEI firmware volume to normal
> RAM. The phase where PEIMs "run from flash" (XIP) still exists, of
> course, except they actually run from RAM -- so, for example, they have
> writeable global variables right off the bat. Perhaps this interferes
> somehow with the RegisterForShadow() service and/or how PEIMs expect
> that service to work.
> 
> Regarding the PEIM entry point -- the PEIMs' "own" entry point functions
> are always wrapped.
> 
> - The outermost (common) entry point function is called
>   _ModuleEntryPoint(). It is declared in
>   "MdePkg/Include/Library/PeimEntryPoint.h". This is what the PEI core
>   calls when dispatching a PEIM.
> 
> - Individual PEIMs add the PeimEntryPoint lib class to their INF files,
>   under [LibraryClasses]. The implementation is in
>   "MdePkg/Library/PeimEntryPoint". In particular, the function calls
>   ProcessLibraryConstructorList().
> 
> - The build tools generate the source code for
>   ProcessLibraryConstructorList(); based on the (recursively determined)
>   library instance dependency tree. (Search the Build subdirectory for
>   "AutoGen.c" files.) So, before the PEIM's actual entry point is
>   called, the lib instances' CONSTRUCTOR functions are called, in the
>   right order, so that the PEIM is entered with all libraries "ready to
>   use".
> 
> I guess that, when Tcg2Pei is dispatched for the 2nd time, from
> permanent RAM, the first (successful) GetFirstGuidHob() call that you
> see occurs like this, from generated code, as part of library
> construction. Some library instance's constructor function calls
> GetFirstGuidHob(), successfully.
> 
> The corruption could somehow be related to the HOB list's migration from
> temp RAM to permanent RAM. The OVMF debug log should contain something
> like this:
> 
> > Temp Stack : BaseAddress=0x818000 Length=0x8000
> > Temp Heap  : BaseAddress=0x810000 Length=0x8000
> > Total temporary memory:    65536 bytes.
> >   temporary memory stack ever used:       28656 bytes.
> >   temporary memory heap used for HobList: 6056 bytes.
> >   temporary memory heap occupied by memory pages: 0 bytes.
> 
> Implying that, before the temp RAM migration, the HOB list consumed ~6KB
> in total of the 32KB temp RAM heap. It seems unlikely that we run out of
> temp RAM heap.
> 
> Hopefully this helps you proceed with the debugging... Corrections are
> welcome too, obviously!
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Qemu-devel] [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Laszlo Ersek]

On 03/06/18 01:45, Brian J. Johnson wrote:
> On 03/05/2018 12:22 PM, Laszlo Ersek wrote:
>> PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
>> RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
>> like RAM) for stack & heap; whatever HOBs they produce are stored in
>> "temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
>> (basically it programs the memory controller and publishes the RAM
>> ranges). In turn the PEI core "migrates" PEIMs from temporary to
>> permanent RAM, including the HOB list.
>>
>> Before the temporary RAM migration (when still executing in place from
>> flash), PEIMs cannot have writeable global variables. For example,
>> dynamic PCDs are also maintained in a HOB (the PCD HOB).
>>
>> A PEIM normally cannot (and shouldn't) tell whether it is dispatched
>> before or after permanent RAM is published. If needed, a PEIM can
>> advertise that it depends on permanent RAM (for example because it needs
>> a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
>> its DEPEX.
>>
>> Finally, it seems like a PEIM can also express, "I'm fine with being
>> dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
>> tell me whichever it is". Apparently, if the PEIM is dispatched from
>> flash (before permanent RAM is available), its call to
>> RegisterForShadow() returns EFI_SUCCESS (and then its entry point
>> function will be invoked for a 2nd time, after the temp RAM migration).
>> And when a PEIM is dispatched from RAM (either for the very first time,
>> or for the second time, after being dispatched from flash first), the
>> same call returns EFI_ALREADY_STARTED.
>>
>> Honestly, I'm unsure what this is good for (both in general, and
>> specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
>> doing the measurements (which makes sense); I just wonder what exactly
>> it does so that it cannot simply specify
>> "gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.
> 
> I haven't looked at this particular PEIM.  But one case where
> registering for shadowing is useful is for improving performance when
> running from permanent RAM, where writable global variables are
> available.  For instance, when running from flash, a ReportStatusCode
> PEIM may need to go through a slow process to locate an output hardware
> device on every PPI call.  This may involve traversing the HOB list,
> consulting other PPIs, even probing hardware addresses.  But once it's
> shadowed to RAM, it can locate the device once, then cache its address
> in a global.  Not to mention that the code itself is far, far faster
> when run from RAM vs. flash.  (That's probably a key difference between
> a real machine and a VM.)

Ah, this sounds like a great example. Status code reporting is useful /
important for debugging even before permanent RAM is installed, so the
PEIM providing that PPI should not have a depex on
gEfiPeiMemoryDiscoveredPpiGuid. However, once the permanent RAM is
published, it makes sense to speed up the implementation. Thanks for
this example!

> Also, I've personally written a PEIM which saves a bunch of internal
> state in a HOB, since that's the main writable storage when running from
> flash.  That state includes pointers to other data (in flash.)  Once the
> data is all shadowed to RAM, it updates the HOB to point to the data in
> RAM.  That way it's a lot faster to access the data.

Another good example. (I think I've been generally blind to this perf
difference between flash and RAM, specifically concerning execution -- I
must have been spoiled by virt, as you say :) )

> I also have other PEIMs which are constrained (via DEPEX) to run *only*
> from RAM, since they have larger memory requirements than can be
> satisfied from temporary cache-as-RAM.  That's certainly a valid
> technique as well.

Right, that's quite known to OVMF too, due to CpuMpPei being "hungry"
like this.

> RegisterForShadow() is a useful tool for making the most of the
> restricted PEI environment.  And having it standardized like this is, as
> Andrew said, a lot better than the hacks people had to use beforehand.

I agree. I'm happy to have learned about this service!

Thanks all,
Laszlo