public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
@ 2021-08-06 15:33 Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 1/7] SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms Stefan Berger
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao; +Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger

This series imports code from the edk2-platforms project related to
changing the password of the TPM2 platform hierarchy and uses it to
disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
aspects of the following bugs:

https://bugzilla.tianocore.org/show_bug.cgi?id=3510
https://bugzilla.tianocore.org/show_bug.cgi?id=3499

There's no doubt that my struggles with the build system and handling
of dependencies are visible in this series. Quite a few aspects of
getting things right are more or less guesswork and I am often not sure
what the correct way of doing things are. If 'you' wanted to fix
things up and repost it, please go ahead...

Stefan

Stefan Berger (7):
  SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
    edk2-platforms
  SecruityPkg/TPM: Disable dependency on MinPlatformPkg
  SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
  SecurityPkg/TPM: Disable a Pcd
  SecurityPkg/TPM: Add a NULL implementation of
    PeiDxeTpmPlatformHierarchyLib
  OVMF: Reference new classes in the build system for compilation
  OVMF: Disable the TPM2 platform hierarchy

 OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
 .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
 .../PlatformBootManagerLib.inf                |   1 +
 .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
 .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
 OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
 OvmfPkg/OvmfPkgX64.dsc                        |   3 +
 .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
 .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
 .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
 .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
 .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
 13 files changed, 432 insertions(+)
 create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.c
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf

-- 
2.31.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 1/7] SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
@ 2021-08-06 15:33 ` Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 2/7] SecruityPkg/TPM: Disable dependency on MinPlatformPkg Stefan Berger
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger,
	Stefan Berger

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
 .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
 .../PeiDxeTpmPlatformHierarchyLib.inf         |  45 +++
 3 files changed, 338 insertions(+)
 create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf

diff --git a/SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h b/SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
new file mode 100644
index 0000000000..a872fa09dc
--- /dev/null
+++ b/SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
@@ -0,0 +1,27 @@
+/** @file
+    TPM Platform Hierarchy configuration library.
+
+    This library provides functions for customizing the TPM's Platform Hierarchy
+    Authorization Value (platformAuth) and Platform Hierarchy Authorization
+    Policy (platformPolicy) can be defined through this function.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) Microsoft Corporation.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef _TPM_PLATFORM_HIERARCHY_LIB_H_
+#define _TPM_PLATFORM_HIERARCHY_LIB_H_
+
+/**
+   This service will perform the TPM Platform Hierarchy configuration at the SmmReadyToLock event.
+
+**/
+VOID
+EFIAPI
+ConfigureTpmPlatformHierarchy (
+  VOID
+  );
+
+#endif
diff --git a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
new file mode 100644
index 0000000000..9812ab99ab
--- /dev/null
+++ b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
@@ -0,0 +1,266 @@
+/** @file
+    TPM Platform Hierarchy configuration library.
+
+    This library provides functions for customizing the TPM's Platform Hierarchy
+    Authorization Value (platformAuth) and Platform Hierarchy Authorization
+    Policy (platformPolicy) can be defined through this function.
+
+    Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+    Copyright (c) Microsoft Corporation.<BR>
+    SPDX-License-Identifier: BSD-2-Clause-Patent
+
+    @par Specification Reference:
+    https://trustedcomputinggroup.org/resource/tcg-tpm-v2-0-provisioning-guidance/
+**/
+
+#include <Uefi.h>
+
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Library/RngLib.h>
+#include <Library/Tpm2CommandLib.h>
+#include <Library/Tpm2DeviceLib.h>
+
+//
+// The authorization value may be no larger than the digest produced by the hash
+//   algorithm used for context integrity.
+//
+#define      MAX_NEW_AUTHORIZATION_SIZE SHA512_DIGEST_SIZE
+
+UINT16       mAuthSize;
+
+/**
+  Generate high-quality entropy source through RDRAND.
+
+  @param[in]   Length        Size of the buffer, in bytes, to fill with.
+  @param[out]  Entropy       Pointer to the buffer to store the entropy data.
+
+  @retval EFI_SUCCESS        Entropy generation succeeded.
+  @retval EFI_NOT_READY      Failed to request random data.
+
+**/
+EFI_STATUS
+EFIAPI
+RdRandGenerateEntropy (
+  IN UINTN         Length,
+  OUT UINT8        *Entropy
+  )
+{
+  EFI_STATUS  Status;
+  UINTN       BlockCount;
+  UINT64      Seed[2];
+  UINT8       *Ptr;
+
+  Status = EFI_NOT_READY;
+  BlockCount = Length / 64;
+  Ptr = (UINT8 *)Entropy;
+
+  //
+  // Generate high-quality seed for DRBG Entropy
+  //
+  while (BlockCount > 0) {
+    Status = GetRandomNumber128 (Seed);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    CopyMem (Ptr, Seed, 64);
+
+    BlockCount--;
+    Ptr = Ptr + 64;
+  }
+
+  //
+  // Populate the remained data as request.
+  //
+  Status = GetRandomNumber128 (Seed);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  CopyMem (Ptr, Seed, (Length % 64));
+
+  return Status;
+}
+
+/**
+  This function returns the maximum size of TPM2B_AUTH; this structure is used for an authorization value
+  and limits an authValue to being no larger than the largest digest produced by a TPM.
+
+  @param[out] AuthSize                 Tpm2 Auth size
+
+  @retval EFI_SUCCESS                  Auth size returned.
+  @retval EFI_DEVICE_ERROR             Can not return platform auth due to device error.
+
+**/
+EFI_STATUS
+EFIAPI
+GetAuthSize (
+  OUT UINT16            *AuthSize
+  )
+{
+  EFI_STATUS            Status;
+  TPML_PCR_SELECTION    Pcrs;
+  UINTN                 Index;
+  UINT16                DigestSize;
+
+  Status = EFI_SUCCESS;
+
+  while (mAuthSize == 0) {
+
+    mAuthSize = SHA1_DIGEST_SIZE;
+    ZeroMem (&Pcrs, sizeof (TPML_PCR_SELECTION));
+    Status = Tpm2GetCapabilityPcrs (&Pcrs);
+
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "Tpm2GetCapabilityPcrs fail!\n"));
+      break;
+    }
+
+    DEBUG ((DEBUG_ERROR, "Tpm2GetCapabilityPcrs - %08x\n", Pcrs.count));
+
+    for (Index = 0; Index < Pcrs.count; Index++) {
+      DEBUG ((DEBUG_ERROR, "alg - %x\n", Pcrs.pcrSelections[Index].hash));
+
+      switch (Pcrs.pcrSelections[Index].hash) {
+      case TPM_ALG_SHA1:
+        DigestSize = SHA1_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SHA256:
+        DigestSize = SHA256_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SHA384:
+        DigestSize = SHA384_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SHA512:
+        DigestSize = SHA512_DIGEST_SIZE;
+        break;
+      case TPM_ALG_SM3_256:
+        DigestSize = SM3_256_DIGEST_SIZE;
+        break;
+      default:
+        DigestSize = SHA1_DIGEST_SIZE;
+        break;
+      }
+
+      if (DigestSize > mAuthSize) {
+        mAuthSize = DigestSize;
+      }
+    }
+    break;
+  }
+
+  *AuthSize = mAuthSize;
+  return Status;
+}
+
+/**
+  Set PlatformAuth to random value.
+**/
+VOID
+RandomizePlatformAuth (
+  VOID
+  )
+{
+  EFI_STATUS                        Status;
+  UINT16                            AuthSize;
+  UINT8                             *Rand;
+  UINTN                             RandSize;
+  TPM2B_AUTH                        NewPlatformAuth;
+
+  //
+  // Send Tpm2HierarchyChange Auth with random value to avoid PlatformAuth being null
+  //
+
+  GetAuthSize (&AuthSize);
+
+  ZeroMem (NewPlatformAuth.buffer, AuthSize);
+  NewPlatformAuth.size = AuthSize;
+
+  //
+  // Allocate one buffer to store random data.
+  //
+  RandSize = MAX_NEW_AUTHORIZATION_SIZE;
+  Rand = AllocatePool (RandSize);
+
+  RdRandGenerateEntropy (RandSize, Rand);
+  CopyMem (NewPlatformAuth.buffer, Rand, AuthSize);
+
+  FreePool (Rand);
+
+  //
+  // Send Tpm2HierarchyChangeAuth command with the new Auth value
+  //
+  Status = Tpm2HierarchyChangeAuth (TPM_RH_PLATFORM, NULL, &NewPlatformAuth);
+  DEBUG ((DEBUG_INFO, "Tpm2HierarchyChangeAuth Result: - %r\n", Status));
+  ZeroMem (NewPlatformAuth.buffer, AuthSize);
+  ZeroMem (Rand, RandSize);
+}
+
+/**
+  Disable the TPM platform hierarchy.
+
+  @retval   EFI_SUCCESS       The TPM was disabled successfully.
+  @retval   Others            An error occurred attempting to disable the TPM platform hierarchy.
+
+**/
+EFI_STATUS
+DisableTpmPlatformHierarchy (
+  VOID
+  )
+{
+  EFI_STATUS  Status;
+
+  // Make sure that we have use of the TPM.
+  Status = Tpm2RequestUseTpm ();
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a:%a() - Tpm2RequestUseTpm Failed! %r\n", gEfiCallerBaseName, __FUNCTION__, Status));
+    ASSERT_EFI_ERROR (Status);
+    return Status;
+  }
+
+  // Let's do what we can to shut down the hierarchies.
+
+  // Disable the PH NV.
+  // IMPORTANT NOTE: We *should* be able to disable the PH NV here, but TPM parts have
+  //                 been known to store the EK cert in the PH NV. If we disable it, the
+  //                 EK cert will be unreadable.
+
+  // Disable the PH.
+  Status =  Tpm2HierarchyControl (
+              TPM_RH_PLATFORM,     // AuthHandle
+              NULL,                // AuthSession
+              TPM_RH_PLATFORM,     // Hierarchy
+              NO                   // State
+              );
+  DEBUG ((DEBUG_VERBOSE, "%a:%a() -  Disable PH = %r\n", gEfiCallerBaseName, __FUNCTION__, Status));
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a:%a() -  Disable PH Failed! %r\n", gEfiCallerBaseName, __FUNCTION__, Status));
+    ASSERT_EFI_ERROR (Status);
+  }
+
+  return Status;
+}
+
+/**
+   This service defines the configuration of the Platform Hierarchy Authorization Value (platformAuth)
+   and Platform Hierarchy Authorization Policy (platformPolicy)
+
+**/
+VOID
+EFIAPI
+ConfigureTpmPlatformHierarchy (
+  )
+{
+  if (PcdGetBool (PcdRandomizePlatformHierarchy)) {
+    //
+    // Send Tpm2HierarchyChange Auth with random value to avoid PlatformAuth being null
+    //
+    RandomizePlatformAuth ();
+  } else {
+    //
+    // Disable the hierarchy entirely (do not randomize it)
+    //
+    DisableTpmPlatformHierarchy ();
+  }
+}
diff --git a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
new file mode 100644
index 0000000000..b7a7fb0a08
--- /dev/null
+++ b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
@@ -0,0 +1,45 @@
+### @file
+#
+#   TPM Platform Hierarchy configuration library.
+#
+#   This library provides functions for customizing the TPM's Platform Hierarchy
+#   Authorization Value (platformAuth) and Platform Hierarchy Authorization
+#   Policy (platformPolicy) can be defined through this function.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) Microsoft Corporation.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+###
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = PeiDxeTpmPlatformHierarchyLib
+  FILE_GUID                      = 7794F92C-4E8E-4E57-9E4A-49A0764C7D73
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = TpmPlatformHierarchyLib|PEIM DXE_DRIVER
+
+[LibraryClasses]
+  BaseLib
+  BaseMemoryLib
+  DebugLib
+  MemoryAllocationLib
+  PcdLib
+  RngLib
+  Tpm2CommandLib
+  Tpm2DeviceLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  SecurityPkg/SecurityPkg.dec
+  CryptoPkg/CryptoPkg.dec
+  MinPlatformPkg/MinPlatformPkg.dec
+
+[Sources]
+  PeiDxeTpmPlatformHierarchyLib.c
+
+[Pcd]
+  gMinPlatformPkgTokenSpaceGuid.PcdRandomizePlatformHierarchy
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 2/7] SecruityPkg/TPM: Disable dependency on MinPlatformPkg
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 1/7] SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms Stefan Berger
@ 2021-08-06 15:33 ` Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy) Stefan Berger
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger,
	Stefan Berger

Disable the dependency on the MinPlatformPkg to avoid this type of build
errors:

/home/stefanb/dev/edk2/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf(39): error 000E: File/directory not found in workspace
	/home/stefanb/dev/edk2/MinPlatformPkg/MinPlatformPkg.dec

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 .../PeiDxeTpmPlatformHierarchyLib.inf                          | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
index b7a7fb0a08..1f23032e46 100644
--- a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
+++ b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
@@ -36,7 +36,8 @@
   MdeModulePkg/MdeModulePkg.dec
   SecurityPkg/SecurityPkg.dec
   CryptoPkg/CryptoPkg.dec
-  MinPlatformPkg/MinPlatformPkg.dec
+
+#  MinPlatformPkg/MinPlatformPkg.dec
 
 [Sources]
   PeiDxeTpmPlatformHierarchyLib.c
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 1/7] SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 2/7] SecruityPkg/TPM: Disable dependency on MinPlatformPkg Stefan Berger
@ 2021-08-06 15:33 ` Stefan Berger
  2021-08-07  5:38   ` Yao, Jiewen
  2021-08-06 15:33 ` [RFC PATCH 4/7] SecurityPkg/TPM: Disable a Pcd Stefan Berger
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger,
	Stefan Berger

To avoid this type of build errors, disable
'PcdGetBool (PcdRandomizePlatformHierarchy)'.

Building ... /home/stefanb/dev/edk2/SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.inf [X64]
In file included from /home/stefanb/dev/edk2/Build/OvmfX64/DEBUG_GCC5/X64/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib/DEBUG/AutoGen.h:17,
                 from <command-line>:
/home/stefanb/dev/edk2/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c: In function ‘ConfigureTpmPlatformHierarchy’:
/home/stefanb/dev/edk2/MdePkg/Include/Library/PcdLib.h:424:45: error: ‘_PCD_GET_MODE_BOOL_PcdRandomizePlatformHierarchy’ undeclared (first use in this function)
  424 | #define PcdGetBool(TokenName)               _PCD_GET_MODE_BOOL_##TokenName
      |                                             ^~~~~~~~~~~~~~~~~~~

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 .../PeiDxeTpmPlatformHierarchyLib.c                             | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
index 9812ab99ab..bea10d37a4 100644
--- a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
+++ b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
@@ -252,7 +252,7 @@ EFIAPI
 ConfigureTpmPlatformHierarchy (
   )
 {
-  if (PcdGetBool (PcdRandomizePlatformHierarchy)) {
+  if (1 /*PcdGetBool (PcdRandomizePlatformHierarchy)*/) {
     //
     // Send Tpm2HierarchyChange Auth with random value to avoid PlatformAuth being null
     //
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 4/7] SecurityPkg/TPM: Disable a Pcd
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
                   ` (2 preceding siblings ...)
  2021-08-06 15:33 ` [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy) Stefan Berger
@ 2021-08-06 15:33 ` Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 5/7] SecurityPkg/TPM: Add a NULL implementation of PeiDxeTpmPlatformHierarchyLib Stefan Berger
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger,
	Stefan Berger

Fix the following build issue.

/home/stefanb/dev/edk2/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf(46): error 4000: Value of Guid [gMinPlatformPkgTokenSpaceGuid] is not found under [Guids] section in
	/home/stefanb/dev/edk2/MdePkg/MdePkg.dec
	/home/stefanb/dev/edk2/MdeModulePkg/MdeModulePkg.dec
	/home/stefanb/dev/edk2/SecurityPkg/SecurityPkg.dec
	/home/stefanb/dev/edk2/CryptoPkg/CryptoPkg.dec

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 .../PeiDxeTpmPlatformHierarchyLib.inf                         | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
index 1f23032e46..f1effd3ffb 100644
--- a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
+++ b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
@@ -42,5 +42,5 @@
 [Sources]
   PeiDxeTpmPlatformHierarchyLib.c
 
-[Pcd]
-  gMinPlatformPkgTokenSpaceGuid.PcdRandomizePlatformHierarchy
+#[Pcd]
+#  gMinPlatformPkgTokenSpaceGuid.PcdRandomizePlatformHierarchy
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 5/7] SecurityPkg/TPM: Add a NULL implementation of PeiDxeTpmPlatformHierarchyLib
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
                   ` (3 preceding siblings ...)
  2021-08-06 15:33 ` [RFC PATCH 4/7] SecurityPkg/TPM: Disable a Pcd Stefan Berger
@ 2021-08-06 15:33 ` Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 6/7] OVMF: Reference new classes in the build system for compilation Stefan Berger
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger,
	Stefan Berger

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 .../PeiDxeTpmPlatformHierarchyLib.c           | 23 +++++++++++
 .../PeiDxeTpmPlatformHierarchyLib.inf         | 39 +++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.c
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf

diff --git a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.c b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.c
new file mode 100644
index 0000000000..e871ada230
--- /dev/null
+++ b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.c
@@ -0,0 +1,23 @@
+/** @file
+    Null TPM Platform Hierarchy configuration library.
+
+    This library provides stub functions for customizing the TPM's Platform Hierarchy
+    Authorization Value (platformAuth) and Platform Hierarchy Authorization
+    Policy (platformPolicy) can be defined through this function.
+
+    Copyright (c) 2021, IBM Corporation.
+    Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+    Copyright (c) Microsoft Corporation.<BR>
+    SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+
+VOID
+EFIAPI
+ConfigureTpmPlatformHierarchy (
+  )
+{
+  /* no nothing */
+}
diff --git a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
new file mode 100644
index 0000000000..678f38410a
--- /dev/null
+++ b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
@@ -0,0 +1,39 @@
+### @file
+#
+#   TPM Platform Hierarchy configuration library.
+#
+#   This library provides functions for customizing the TPM's Platform Hierarchy
+#   Authorization Value (platformAuth) and Platform Hierarchy Authorization
+#   Policy (platformPolicy) can be defined through this function.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) Microsoft Corporation.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+###
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = PeiDxeTpmPlatformHierarchyLibNull
+  FILE_GUID                      = 7794F92C-4E8E-4E57-9E4A-49A0764C7D73
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = TpmPlatformHierarchyLib|PEIM DXE_DRIVER
+
+[LibraryClasses]
+  BaseLib
+#  BaseMemoryLib
+#  DebugLib
+#  MemoryAllocationLib
+#  PcdLib
+#  RngLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  SecurityPkg/SecurityPkg.dec
+  CryptoPkg/CryptoPkg.dec
+
+[Sources]
+  PeiDxeTpmPlatformHierarchyLib.c
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 6/7] OVMF: Reference new classes in the build system for compilation
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
                   ` (4 preceding siblings ...)
  2021-08-06 15:33 ` [RFC PATCH 5/7] SecurityPkg/TPM: Add a NULL implementation of PeiDxeTpmPlatformHierarchyLib Stefan Berger
@ 2021-08-06 15:33 ` Stefan Berger
  2021-08-06 15:33 ` [RFC PATCH 7/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
  2021-08-07  6:00 ` [RFC PATCH 0/7] " Yao, Jiewen
  7 siblings, 0 replies; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger,
	Stefan Berger

Compile the added code now.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc                                   | 3 +++
 .../Library/PlatformBootManagerLib/PlatformBootManagerLib.inf  | 1 +
 OvmfPkg/OvmfPkgIa32.dsc                                        | 3 +++
 OvmfPkg/OvmfPkgIa32X64.dsc                                     | 3 +++
 OvmfPkg/OvmfPkgX64.dsc                                         | 3 +++
 5 files changed, 13 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index e6cd10b759..6b582626ff 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -209,9 +209,11 @@
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
   Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
   TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
 !else
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
 !endif
 
 [LibraryClasses.common]
@@ -836,6 +838,7 @@
   SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
     <LibraryClasses>
       Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
+      TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
       NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
       HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index e470b9a6a3..e7d1917022 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -56,6 +56,7 @@
   PlatformBmPrintScLib
   Tcg2PhysicalPresenceLib
   XenPlatformLib
+  TpmPlatformHierarchyLib
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index d1d92c97ba..374a1ea652 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -235,9 +235,11 @@
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
   Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
   TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
 !else
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
 !endif
 
 [LibraryClasses.common]
@@ -711,6 +713,7 @@
   SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
     <LibraryClasses>
       HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
+      TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a467ab7090..7b7dffcd94 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -239,9 +239,11 @@
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
   Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
   TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
 !else
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
 !endif
 
 [LibraryClasses.common]
@@ -1034,6 +1036,7 @@
   SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
     <LibraryClasses>
       Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
+      TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
       NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
       HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index e56b83d95e..34c6e833e4 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -239,9 +239,11 @@
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
   Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
   TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
 !else
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+  TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
 !endif
 
 [LibraryClasses.common]
@@ -723,6 +725,7 @@
   SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
     <LibraryClasses>
       HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
+      TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 7/7] OVMF: Disable the TPM2 platform hierarchy
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
                   ` (5 preceding siblings ...)
  2021-08-06 15:33 ` [RFC PATCH 6/7] OVMF: Reference new classes in the build system for compilation Stefan Berger
@ 2021-08-06 15:33 ` Stefan Berger
  2021-08-07  6:00 ` [RFC PATCH 0/7] " Yao, Jiewen
  7 siblings, 0 replies; 14+ messages in thread
From: Stefan Berger @ 2021-08-06 15:33 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: marcandre.lureau, lersek, dick_wilkins, Stefan Berger,
	Stefan Berger

Use the newly added functions to disable the TPM2 platform hierarchy.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c      | 6 ++++++
 OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c | 6 ++++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c  | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index b0e9742937..5bf145ba25 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -11,6 +11,7 @@
 #include <Protocol/FirmwareVolume2.h>
 #include <Library/PlatformBmPrintScLib.h>
 #include <Library/Tcg2PhysicalPresenceLib.h>
+#include <Library/TpmPlatformHierarchyLib.h>
 #include <Library/XenPlatformLib.h>
 
 
@@ -1516,6 +1517,11 @@ PlatformBootManagerAfterConsole (
   //
   Tcg2PhysicalPresenceLibProcessRequest (NULL);
 
+  //
+  // Disable the TPM 2 platform hierarchy
+  //
+  ConfigureTpmPlatformHierarchy ();
+
   //
   // Process QEMU's -kernel command line option
   //
diff --git a/OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c
index eaade4adea..09418dc4ff 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c
@@ -12,6 +12,7 @@
 #include <Protocol/FirmwareVolume2.h>
 #include <Library/PlatformBmPrintScLib.h>
 #include <Library/Tcg2PhysicalPresenceLib.h>
+#include <Library/TpmPlatformHierarchyLib.h>
 
 #include <Protocol/BlockIo.h>
 
@@ -1450,6 +1451,11 @@ PlatformBootManagerAfterConsole (
   //
   Tcg2PhysicalPresenceLibProcessRequest (NULL);
 
+  //
+  // Disable the TPM 2 platform hierarchy
+  //
+  ConfigureTpmPlatformHierarchy ();
+
   //
   // Perform some platform specific connect sequence
   //
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
index 7cceeea487..508e2b6403 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
@@ -12,6 +12,7 @@
 #include <Protocol/FirmwareVolume2.h>
 #include <Library/PlatformBmPrintScLib.h>
 #include <Library/Tcg2PhysicalPresenceLib.h>
+#include <Library/TpmPlatformHierarchyLib.h>
 
 
 //
@@ -1315,6 +1316,11 @@ PlatformBootManagerAfterConsole (
   //
   Tcg2PhysicalPresenceLibProcessRequest (NULL);
 
+  //
+  // Disable the TPM 2 platform hierachy
+  //
+  ConfigureTpmPlatformHierarchy ();
+
   //
   // Process QEMU's -kernel command line option
   //
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
  2021-08-06 15:33 ` [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy) Stefan Berger
@ 2021-08-07  5:38   ` Yao, Jiewen
  0 siblings, 0 replies; 14+ messages in thread
From: Yao, Jiewen @ 2021-08-07  5:38 UTC (permalink / raw)
  To: Stefan Berger, devel@edk2.groups.io
  Cc: marcandre.lureau@redhat.com, lersek@redhat.com,
	dick_wilkins@phoenix.com, Stefan Berger

Stefan
This patch is NOT acceptable.

> +  if (1 /*PcdGetBool (PcdRandomizePlatformHierarchy)*/) {

Nacked-by: Jiewen Yao <Jiewen.yao@intel.com>

Thank you
Yao Jiewen


> -----Original Message-----
> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Sent: Friday, August 6, 2021 11:33 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>;
> Stefan Berger <stefanb@linux.ibm.com>
> Subject: [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool
> (PcdRandomizePlatformHierarchy)
> 
> To avoid this type of build errors, disable
> 'PcdGetBool (PcdRandomizePlatformHierarchy)'.
> 
> Building ...
> /home/stefanb/dev/edk2/SecurityPkg/Library/SecureBootVariableProvisionLib/
> SecureBootVariableProvisionLib.inf [X64]
> In file included from
> /home/stefanb/dev/edk2/Build/OvmfX64/DEBUG_GCC5/X64/SecurityPkg/Librar
> y/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib/DEBUG/Aut
> oGen.h:17,
>                  from <command-line>:
> /home/stefanb/dev/edk2/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/
> PeiDxeTpmPlatformHierarchyLib.c: In function
> ‘ConfigureTpmPlatformHierarchy’:
> /home/stefanb/dev/edk2/MdePkg/Include/Library/PcdLib.h:424:45: error:
> ‘_PCD_GET_MODE_BOOL_PcdRandomizePlatformHierarchy’ undeclared (first
> use in this function)
>   424 | #define PcdGetBool(TokenName)
> _PCD_GET_MODE_BOOL_##TokenName
>       |                                             ^~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  .../PeiDxeTpmPlatformHierarchyLib.c                             | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git
> a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHier
> archyLib.c
> b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHier
> archyLib.c
> index 9812ab99ab..bea10d37a4 100644
> ---
> a/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHier
> archyLib.c
> +++
> b/SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHier
> archyLib.c
> @@ -252,7 +252,7 @@ EFIAPI
>  ConfigureTpmPlatformHierarchy (
> 
>    )
> 
>  {
> 
> -  if (PcdGetBool (PcdRandomizePlatformHierarchy)) {
> 
> +  if (1 /*PcdGetBool (PcdRandomizePlatformHierarchy)*/) {
> 
>      //
> 
>      // Send Tpm2HierarchyChange Auth with random value to avoid PlatformAuth
> being null
> 
>      //
> 
> --
> 2.31.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
  2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
                   ` (6 preceding siblings ...)
  2021-08-06 15:33 ` [RFC PATCH 7/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
@ 2021-08-07  6:00 ` Yao, Jiewen
  2021-08-07 10:22   ` [edk2-devel] " Marvin Häuser
  2021-08-07 15:03   ` Stefan Berger
  7 siblings, 2 replies; 14+ messages in thread
From: Yao, Jiewen @ 2021-08-07  6:00 UTC (permalink / raw)
  To: Stefan Berger, devel@edk2.groups.io
  Cc: marcandre.lureau@redhat.com, lersek@redhat.com,
	dick_wilkins@phoenix.com

Hi Stefan
It seems this patch series is not a production fix. It is more like a prototype, my personally feeling.

A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the code. Please remove the unnecessary code directly without // or /**/ in C, and # in INF.

For patch 1, if you want to move the code to SecurityPkg, that is fine. Please move the whole driver their and you should not remove and code by comment. Please fix the issue to make it pass build, instead of commenting the code like work-around.
Otherwise, you may copy the module to OvmfPkg. Then you can modify it as you need.

Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1, then add fix in 2, 3, 4.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Sent: Friday, August 6, 2021 11:33 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> 
> This series imports code from the edk2-platforms project related to
> changing the password of the TPM2 platform hierarchy and uses it to
> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
> aspects of the following bugs:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
> 
> There's no doubt that my struggles with the build system and handling
> of dependencies are visible in this series. Quite a few aspects of
> getting things right are more or less guesswork and I am often not sure
> what the correct way of doing things are. If 'you' wanted to fix
> things up and repost it, please go ahead...
> 
> Stefan
> 
> Stefan Berger (7):
>   SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
>     edk2-platforms
>   SecruityPkg/TPM: Disable dependency on MinPlatformPkg
>   SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
>   SecurityPkg/TPM: Disable a Pcd
>   SecurityPkg/TPM: Add a NULL implementation of
>     PeiDxeTpmPlatformHierarchyLib
>   OVMF: Reference new classes in the build system for compilation
>   OVMF: Disable the TPM2 platform hierarchy
> 
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
>  .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
>  .../PlatformBootManagerLib.inf                |   1 +
>  .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
>  .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
>  OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   3 +
>  .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
>  .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
>  .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
>  .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
>  .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
>  13 files changed, 432 insertions(+)
>  create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> chyLib.c
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> chyLib.inf
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> erarchyLib.c
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> erarchyLib.inf
> 
> --
> 2.31.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [edk2-devel] [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
  2021-08-07  6:00 ` [RFC PATCH 0/7] " Yao, Jiewen
@ 2021-08-07 10:22   ` Marvin Häuser
  2021-08-08  0:31     ` Yao, Jiewen
  2021-08-07 15:03   ` Stefan Berger
  1 sibling, 1 reply; 14+ messages in thread
From: Marvin Häuser @ 2021-08-07 10:22 UTC (permalink / raw)
  To: devel, jiewen.yao, Stefan Berger
  Cc: marcandre.lureau@redhat.com, lersek@redhat.com,
	dick_wilkins@phoenix.com

Hey Jiewen,
Hey Stefan,

On 07.08.21 08:00, Yao, Jiewen wrote:
> Hi Stefan
> It seems this patch series is not a production fix. It is more like a prototype, my personally feeling.
>
> A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the code. Please remove the unnecessary code directly without // or /**/ in C, and # in INF.
>
> For patch 1, if you want to move the code to SecurityPkg, that is fine. Please move the whole driver their and you should not remove and code by comment. Please fix the issue to make it pass build, instead of commenting the code like work-around.
> Otherwise, you may copy the module to OvmfPkg. Then you can modify it as you need.

Maybe there should be stricter policies about what code goes where, and 
duplication should be outright banned? My PE/COFF loader proposal merged 
at least 4 copies of the Authenticode hashing and 3 copies of a record 
construction algorithm together. There have been other code 
centralisations, such as certificate iteration. If code exists twice, it 
needs to me maintained twice, and in reality this does not happen.

Regarding what goes where, e.g. FatPkg is in edk2, but the new EXT4 
driver was recommended to be located in edk2-platforms. I know there are 
a bunch of TPM-related things (e.g. Image measuring) in SecurityPkg, and 
as someone with no expertise around TPM or the edk2-platforms design, I 
would never even have thought to look in edk2-platforms for such code. 
And especially for library code edk2-platforms seems to be an 
unfortunate location, as edk2 packages can never depend on them.

> Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1, then add fix in 2, 3, 4.

I think for this initial inspection this was actually a good thing. The 
separation and the patches after 1 signal to me that there have been no 
functional changes to the library at all. Probably the best idea is to 
just move it to SecurityPkg entirely, including the PCDs, and update 
MinPlatformPkg to consume it from there instead?

Best regards,
Marvin

>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Sent: Friday, August 6, 2021 11:33 PM
>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
>> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
>>
>> This series imports code from the edk2-platforms project related to
>> changing the password of the TPM2 platform hierarchy and uses it to
>> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
>> aspects of the following bugs:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
>>
>> There's no doubt that my struggles with the build system and handling
>> of dependencies are visible in this series. Quite a few aspects of
>> getting things right are more or less guesswork and I am often not sure
>> what the correct way of doing things are. If 'you' wanted to fix
>> things up and repost it, please go ahead...
>>
>> Stefan
>>
>> Stefan Berger (7):
>>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
>>      edk2-platforms
>>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
>>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
>>    SecurityPkg/TPM: Disable a Pcd
>>    SecurityPkg/TPM: Add a NULL implementation of
>>      PeiDxeTpmPlatformHierarchyLib
>>    OVMF: Reference new classes in the build system for compilation
>>    OVMF: Disable the TPM2 platform hierarchy
>>
>>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
>>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
>>   .../PlatformBootManagerLib.inf                |   1 +
>>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
>>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
>>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
>>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
>>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
>>   13 files changed, 432 insertions(+)
>>   create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.inf
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.inf
>>
>> --
>> 2.31.1
>
>
> 
>
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
  2021-08-07  6:00 ` [RFC PATCH 0/7] " Yao, Jiewen
  2021-08-07 10:22   ` [edk2-devel] " Marvin Häuser
@ 2021-08-07 15:03   ` Stefan Berger
  2021-08-08  0:54     ` Yao, Jiewen
  1 sibling, 1 reply; 14+ messages in thread
From: Stefan Berger @ 2021-08-07 15:03 UTC (permalink / raw)
  To: Yao, Jiewen, Stefan Berger, devel@edk2.groups.io
  Cc: marcandre.lureau@redhat.com, lersek@redhat.com,
	dick_wilkins@phoenix.com


On 8/7/21 2:00 AM, Yao, Jiewen wrote:
> Hi Stefan
> It seems this patch series is not a production fix. It is more like a prototype, my personally feeling.]

Yes, it's an RFC patch and with the struggles as pointed out below. I 
don't know how this project would go about importing code from 
'edk2-platforms' for example, what deviations from the original code in 
edk2-platforms you are willing to accept and what not, such as removing 
dependencies from the original code and commenting out code that doesn't 
work anymore due the removal. What I imported looked like it had a 
dependency on 'MinPlatformPkg/MinPlatformPkg.dec'. Do we need to import 
this package first or rather not?


>
> A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the code. Please remove the unnecessary code directly without // or /**/ in C, and # in INF.
>
> For patch 1, if you want to move the code to SecurityPkg, that is fine. Please move the whole driver their and you should not remove and code by comment. Please fix the issue to make it pass build, instead of commenting the code like work-around.


What is the 'whole driver'? Can you be a bit more specific what all 
files are comprising the 'whole driver.'


> Otherwise, you may copy the module to OvmfPkg. Then you can modify it as you need.

What is the 'module' versus the 'whole driver?'

And in the end, how would 'you' go about this in this case?


>
> Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1, then add fix in 2, 3, 4.
>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Sent: Friday, August 6, 2021 11:33 PM
>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
>> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
>>
>> This series imports code from the edk2-platforms project related to
>> changing the password of the TPM2 platform hierarchy and uses it to
>> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
>> aspects of the following bugs:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
>>
>> There's no doubt that my struggles with the build system and handling
>> of dependencies are visible in this series. Quite a few aspects of
>> getting things right are more or less guesswork and I am often not sure
>> what the correct way of doing things are. If 'you' wanted to fix
>> things up and repost it, please go ahead...
>>
>> Stefan
>>
>> Stefan Berger (7):
>>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
>>      edk2-platforms
>>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
>>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
>>    SecurityPkg/TPM: Disable a Pcd
>>    SecurityPkg/TPM: Add a NULL implementation of
>>      PeiDxeTpmPlatformHierarchyLib
>>    OVMF: Reference new classes in the build system for compilation
>>    OVMF: Disable the TPM2 platform hierarchy
>>
>>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
>>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
>>   .../PlatformBootManagerLib.inf                |   1 +
>>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
>>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
>>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
>>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
>>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
>>   13 files changed, 432 insertions(+)
>>   create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.inf
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.inf
>>
>> --
>> 2.31.1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [edk2-devel] [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
  2021-08-07 10:22   ` [edk2-devel] " Marvin Häuser
@ 2021-08-08  0:31     ` Yao, Jiewen
  0 siblings, 0 replies; 14+ messages in thread
From: Yao, Jiewen @ 2021-08-08  0:31 UTC (permalink / raw)
  To: devel@edk2.groups.io, mhaeuser@posteo.de, Stefan Berger
  Cc: marcandre.lureau@redhat.com, lersek@redhat.com,
	dick_wilkins@phoenix.com

Hi Marvin
There are many factors impacting where to put the feature code - EDKII or EDKII-platform/Feature.
I can list some of them:
If it is a basic feature v.s. an advanced feature.
If it is a mature production feature v.s. immature feature.
If it is a generic core feature v.s. a platform specific feature.
......


The position can be changed. For example, we can put a feature to EDKII-platform at first. Later, if everyone wants this feature in a production, we can move it to EDKII.

For Ext4, I think it had better be in EDKII-platform at first, because it seems to be an advanced feature. I am not sure how many production need it. And I am not sure the maturity level.

For TPM Hierarchy, we put it to EDKII-platform at first, because we think the platform hierarchy management is platform specific behavior. Each platform may choose their own way to do that. But if all people think the example in EDKII-platform can be used in their production. I think it is OK to move to SecurityPkg.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
> Häuser
> Sent: Saturday, August 7, 2021 6:22 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Stefan Berger
> <stefanb@linux.vnet.ibm.com>
> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> dick_wilkins@phoenix.com
> Subject: Re: [edk2-devel] [RFC PATCH 0/7] OVMF: Disable the TPM2 platform
> hierarchy
> 
> Hey Jiewen,
> Hey Stefan,
> 
> On 07.08.21 08:00, Yao, Jiewen wrote:
> > Hi Stefan
> > It seems this patch series is not a production fix. It is more like a prototype, my
> personally feeling.
> >
> > A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the
> code. Please remove the unnecessary code directly without // or /**/ in C, and #
> in INF.
> >
> > For patch 1, if you want to move the code to SecurityPkg, that is fine. Please
> move the whole driver their and you should not remove and code by comment.
> Please fix the issue to make it pass build, instead of commenting the code like
> work-around.
> > Otherwise, you may copy the module to OvmfPkg. Then you can modify it as
> you need.
> 
> Maybe there should be stricter policies about what code goes where, and
> duplication should be outright banned? My PE/COFF loader proposal merged
> at least 4 copies of the Authenticode hashing and 3 copies of a record
> construction algorithm together. There have been other code
> centralisations, such as certificate iteration. If code exists twice, it
> needs to me maintained twice, and in reality this does not happen.
> 
> Regarding what goes where, e.g. FatPkg is in edk2, but the new EXT4
> driver was recommended to be located in edk2-platforms. I know there are
> a bunch of TPM-related things (e.g. Image measuring) in SecurityPkg, and
> as someone with no expertise around TPM or the edk2-platforms design, I
> would never even have thought to look in edk2-platforms for such code.
> And especially for library code edk2-platforms seems to be an
> unfortunate location, as edk2 packages can never depend on them.
> 
> > Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1,
> then add fix in 2, 3, 4.
> 
> I think for this initial inspection this was actually a good thing. The
> separation and the patches after 1 signal to me that there have been no
> functional changes to the library at all. Probably the best idea is to
> just move it to SecurityPkg entirely, including the PCDs, and update
> MinPlatformPkg to consume it from there instead?
> 
> Best regards,
> Marvin
> 
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Sent: Friday, August 6, 2021 11:33 PM
> >> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> >> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> >>
> >> This series imports code from the edk2-platforms project related to
> >> changing the password of the TPM2 platform hierarchy and uses it to
> >> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
> >> aspects of the following bugs:
> >>
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
> >>
> >> There's no doubt that my struggles with the build system and handling
> >> of dependencies are visible in this series. Quite a few aspects of
> >> getting things right are more or less guesswork and I am often not sure
> >> what the correct way of doing things are. If 'you' wanted to fix
> >> things up and repost it, please go ahead...
> >>
> >> Stefan
> >>
> >> Stefan Berger (7):
> >>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
> >>      edk2-platforms
> >>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
> >>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
> >>    SecurityPkg/TPM: Disable a Pcd
> >>    SecurityPkg/TPM: Add a NULL implementation of
> >>      PeiDxeTpmPlatformHierarchyLib
> >>    OVMF: Reference new classes in the build system for compilation
> >>    OVMF: Disable the TPM2 platform hierarchy
> >>
> >>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
> >>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
> >>   .../PlatformBootManagerLib.inf                |   1 +
> >>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
> >>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
> >>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
> >>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
> >>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
> >>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
> >>   13 files changed, 432 insertions(+)
> >>   create mode 100644
> SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.inf
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.inf
> >>
> >> --
> >> 2.31.1
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
  2021-08-07 15:03   ` Stefan Berger
@ 2021-08-08  0:54     ` Yao, Jiewen
  0 siblings, 0 replies; 14+ messages in thread
From: Yao, Jiewen @ 2021-08-08  0:54 UTC (permalink / raw)
  To: Stefan Berger, Stefan Berger, devel@edk2.groups.io
  Cc: marcandre.lureau@redhat.com, lersek@redhat.com,
	dick_wilkins@phoenix.com

Thanks for the clarification.
Comments below:


> -----Original Message-----
> From: Stefan Berger <stefanb@linux.ibm.com>
> Sent: Saturday, August 7, 2021 11:04 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Stefan Berger
> <stefanb@linux.vnet.ibm.com>; devel@edk2.groups.io
> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> dick_wilkins@phoenix.com
> Subject: Re: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> 
> 
> On 8/7/21 2:00 AM, Yao, Jiewen wrote:
> > Hi Stefan
> > It seems this patch series is not a production fix. It is more like a prototype, my
> personally feeling.]
> 
> Yes, it's an RFC patch and with the struggles as pointed out below. I
> don't know how this project would go about importing code from
> 'edk2-platforms' for example, what deviations from the original code in
> edk2-platforms you are willing to accept and what not, such as removing
> dependencies from the original code and commenting out code that doesn't
> work anymore due the removal. What I imported looked like it had a
> dependency on 'MinPlatformPkg/MinPlatformPkg.dec'. Do we need to import
> this package first or rather not?

[Jiewen]
If you want to move a feature from EDKII-platform to EDKII, you must remove the dependency on EDKII-platform.
And there are lots of ways on how to remove dependency.

For example, you can define the PCD in EDKII directly. As such, you don’t need depend upon MinPlatformPkg.



> >
> > A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the
> code. Please remove the unnecessary code directly without // or /**/ in C, and #
> in INF.
> >
> > For patch 1, if you want to move the code to SecurityPkg, that is fine. Please
> move the whole driver their and you should not remove and code by comment.
> Please fix the issue to make it pass build, instead of commenting the code like
> work-around.
> 
> 
> What is the 'whole driver'? Can you be a bit more specific what all
> files are comprising the 'whole driver.'
> 
> 
> > Otherwise, you may copy the module to OvmfPkg. Then you can modify it as
> you need.
> 
> What is the 'module' versus the 'whole driver?'

[Jiewen] Sorry, I don’t mean to confuse you.
Usually, Driver means a UEFI/PI driver. Module means a driver or a library.
But a code may be built as a driver or a library, some people treat them as same thing. 
So they are same thing in this context.

> 
> And in the end, how would 'you' go about this in this case?

[Jiewen] I have two options in my mind.
Option 1: We move the Tpm2PlatformHierachy support from EDKII-platform to EDKII/SecurityPkg.
The move action can take 2 steps:
A) To 'create' the same Tpm2PlatformHierachy modules in SecurityPkg.
B) To 'remove' the Tpm2PlatformHierachy modules in EDKII-platform.

The modules refer to Tcg2PlatformPei/ Tcg2PlatformDxe / PeiDxeTpmPlatformHierarchyLib in
https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/MinPlatformPkg/Tcg

'Create' means you don’t need use 'move/modify' approach to introduce a broken patch with MinPlatformPkg then fix it in the second patch. You just need create one workable patch.

However, since this impacts the EDKII core component, we need more time to review.


Option 2: We create the Tpm2PlatformHiearchy support in OvmfPkg directly.
This only takes 1 step:
A) To 'create' the Tpm2PlatformHierachy modules in OvmfPkg.
You cannot touch Tpm2PlatformHierachy modules in EDKII-platform.

This only impacts OvmfPkg. So you can make necessary change for virtual platform without considering the requirement in physical platform.

Thank you
Yao Jiewen

> 
> 
> >
> > Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1,
> then add fix in 2, 3, 4.
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Sent: Friday, August 6, 2021 11:33 PM
> >> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> >> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> >>
> >> This series imports code from the edk2-platforms project related to
> >> changing the password of the TPM2 platform hierarchy and uses it to
> >> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
> >> aspects of the following bugs:
> >>
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
> >>
> >> There's no doubt that my struggles with the build system and handling
> >> of dependencies are visible in this series. Quite a few aspects of
> >> getting things right are more or less guesswork and I am often not sure
> >> what the correct way of doing things are. If 'you' wanted to fix
> >> things up and repost it, please go ahead...
> >>
> >> Stefan
> >>
> >> Stefan Berger (7):
> >>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
> >>      edk2-platforms
> >>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
> >>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
> >>    SecurityPkg/TPM: Disable a Pcd
> >>    SecurityPkg/TPM: Add a NULL implementation of
> >>      PeiDxeTpmPlatformHierarchyLib
> >>    OVMF: Reference new classes in the build system for compilation
> >>    OVMF: Disable the TPM2 platform hierarchy
> >>
> >>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
> >>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
> >>   .../PlatformBootManagerLib.inf                |   1 +
> >>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
> >>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
> >>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
> >>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
> >>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
> >>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
> >>   13 files changed, 432 insertions(+)
> >>   create mode 100644
> SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.inf
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.inf
> >>
> >> --
> >> 2.31.1

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-08-08  0:54 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 1/7] SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 2/7] SecruityPkg/TPM: Disable dependency on MinPlatformPkg Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy) Stefan Berger
2021-08-07  5:38   ` Yao, Jiewen
2021-08-06 15:33 ` [RFC PATCH 4/7] SecurityPkg/TPM: Disable a Pcd Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 5/7] SecurityPkg/TPM: Add a NULL implementation of PeiDxeTpmPlatformHierarchyLib Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 6/7] OVMF: Reference new classes in the build system for compilation Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 7/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
2021-08-07  6:00 ` [RFC PATCH 0/7] " Yao, Jiewen
2021-08-07 10:22   ` [edk2-devel] " Marvin Häuser
2021-08-08  0:31     ` Yao, Jiewen
2021-08-07 15:03   ` Stefan Berger
2021-08-08  0:54     ` Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox