Re: [edk2-devel] [edk2-platforms][Patch V2] MinPlatformPkg: Library for customizing TPM platform auth

2019-11-11 Thread Kubacki, Michael A 
Can you please make these changes in V3?

MinPlatformPkg\MinPlatformPkg.dsc
  * It seems like a library class override section for Tcg2PlatformDxe is not 
necessary. Can the
  TpmPlatformAuthLib library class simply be assigned in the [LibraryClasses] 
section?

MinPlatformPkg\Tcg\Tcg2PlatformDxe\Tcg2PlatformDxe.c
  * I suggest using "SmmReadyToLock" instead of "ReadyToLock" throughout the 
file.
  * Can you please check again if all library classes included are required for 
this implementation?
  For example, I don't see usage of MemoryAllocationLib in the file.

MinPlatformPkg\Include\Library\TpmPlatformAuthLib\TpmPlatformAuthLib.h
  * Please add a file description to the copyright header.

  * ReadyToBoot is not the actual event the function is being invoked upon.
  So TpmPlatformAuthReadyToBootHandler ( ) is not accurate. The function is 
also not
  directly a notification handler (the function signature does not reflect that 
of a handler)
  so keeping "Handler" in the name is somewhat misleading.

  Some suggestions are:
1. TpmPlatformAuthAtSmmReadyToLock ( ) -> Keeps the event point in the name
2. ConfigureTpmPlatformAuthAtSmmReadyToLock ( ) -> More descriptive; keeps 
event point in the name
3. ConfigureTpmPlatformAuth ( ) -> More succinct; allows flexibility for 
invocation

  Unless there's a strong reason to associate the functionality with 
SmmReadyToLock, I suggest #3 (or your own name).

MinPlatformPkg\Tcg\Tcg2PlatformPei\Tcg2PlatformPei.c
  * It doesn't seem these changes are directly related to the rest of the 
patch? Could it be a separate patch?

MinPlatformPkg\Tcg\Library\TpmPlatformAuthLib\TpmPlatformAuthlib.inf
  * The constructor doesn't do anything. Is it actually needed?
  * Typo in function name: TpmPlatformAuthLibContructor

MinPlatformPkg\Tcg\Library\TpmPlatformAuthLib\TpmPlatformAuthlib.c
  * The function description for TpmPlatformAuthLibConstructor ( ) references 
parameters that don't actually exist
  in the implementation.

Thanks,
Michael

> -Original Message-
> From: Gonzalez Del Cueto, Rodrigo 
> Sent: Monday, November 11, 2019 1:43 AM
> To: devel@edk2.groups.io
> Cc: Gonzalez Del Cueto, Rodrigo ;
> Kubacki, Michael A ; Chiu, Chasel
> ; Desimone, Nathaniel L
> ; Gao, Liming 
> Subject: [edk2-platforms][Patch V2] MinPlatformPkg: Library for customizing
> TPM platform auth
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2331
> 
> In V2:
>   + Kept callback function and registration in Tcg2PlatformDxe module.
>   + New library defining API function: TpmPlatformAuthReadyToBootHandler
> for configuring the TPM's Platform Hierachy. This is now called
> through Tcg2PlatformDxe's ReadyToLockEventCallBack.
>   + Ported GetAuthSize fix to both Tcg2PlatformPei and MinPlatform's
> TpmPlatformAuthLib instance.
> 
> In order to enable some TPM use cases BIOS should enable to customize the
> configuration of the TPM platform, provisioning of endorsement, platform
> and storage hierarchy.
> 
> Cc: Michael Kubacki 
> Cc: Chasel Chiu 
> Cc: Nate DeSimone 
> Cc: Liming Gao 
> 
> Signed-off-by: Rodrigo Gonzalez del Cueto
> 
> ---
>  .../Include/Library/TpmPlatformAuthLib.h  |  24 ++
>  .../Intel/MinPlatformPkg/MinPlatformPkg.dec   |   2 +
>  .../Intel/MinPlatformPkg/MinPlatformPkg.dsc   |   5 +-
>  .../TpmPlatformAuthLib/TpmPlatformAuthLib.c   | 229
> ++
>  .../TpmPlatformAuthLib/TpmPlatformAuthLib.inf |  49 
>  .../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c | 161 ++--
>  .../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf   |   6 +-
>  .../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c | 100 +---
>  8 files changed, 402 insertions(+), 174 deletions(-)  create mode 100644
> Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h
>  create mode 100644
> Platform/Intel/MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatfo
> rmAuthLib.c
>  create mode 100644
> Platform/Intel/MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatfo
> rmAuthLib.inf
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h
> b/Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h
> new file mode 100644
> index ..f33b67b0
> --- /dev/null
> +++
> b/Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h
> @@ -0,0 +1,24 @@
> +/** @file++Copyright (c) 2019, Intel Corporation. All rights
> reserved.+SPDX-License-Identifier: BSD-2-Clause-
> Patent++**/++#ifndef _TPM_PLATFORM_AUTH_LIB_H_+#define
> _TPM_PLATFORM_AUTH_LIB_H_++#include +#include
> ++/**+   This service will perform the TPM Platform Auth
> configuration at the ReadyToBoot
> event.++**/+VOID+EFIAPI+TpmPlatformAuthReadyToBootHandler (+
> VOID+  );++#endifdiff --git
> a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> index a851021c..fc5979db 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> @@ -62,6

[edk2-devel] [edk2-platforms][Patch V2] MinPlatformPkg: Library for customizing TPM platform auth

2019-11-11 Thread Rodrigo Gonzalez del Cueto 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2331

In V2:
  + Kept callback function and registration in Tcg2PlatformDxe module.
  + New library defining API function: TpmPlatformAuthReadyToBootHandler
for configuring the TPM's Platform Hierachy. This is now called
through Tcg2PlatformDxe's ReadyToLockEventCallBack.
  + Ported GetAuthSize fix to both Tcg2PlatformPei and MinPlatform's
TpmPlatformAuthLib instance.

In order to enable some TPM use cases BIOS should enable to customize
the configuration of the TPM platform, provisioning of endorsement,
platform and storage hierarchy.

Cc: Michael Kubacki 
Cc: Chasel Chiu 
Cc: Nate DeSimone 
Cc: Liming Gao 

Signed-off-by: Rodrigo Gonzalez del Cueto 
---
 .../Include/Library/TpmPlatformAuthLib.h  |  24 ++
 .../Intel/MinPlatformPkg/MinPlatformPkg.dec   |   2 +
 .../Intel/MinPlatformPkg/MinPlatformPkg.dsc   |   5 +-
 .../TpmPlatformAuthLib/TpmPlatformAuthLib.c   | 229 ++
 .../TpmPlatformAuthLib/TpmPlatformAuthLib.inf |  49 
 .../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.c | 161 ++--
 .../Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf   |   6 +-
 .../Tcg/Tcg2PlatformPei/Tcg2PlatformPei.c | 100 +---
 8 files changed, 402 insertions(+), 174 deletions(-)
 create mode 100644 
Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h
 create mode 100644 
Platform/Intel/MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatformAuthLib.c
 create mode 100644 
Platform/Intel/MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatformAuthLib.inf

diff --git a/Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h 
b/Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h
new file mode 100644
index ..f33b67b0
--- /dev/null
+++ b/Platform/Intel/MinPlatformPkg/Include/Library/TpmPlatformAuthLib.h
@@ -0,0 +1,24 @@
+/** @file
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef _TPM_PLATFORM_AUTH_LIB_H_
+#define _TPM_PLATFORM_AUTH_LIB_H_
+
+#include 
+#include 
+
+/**
+   This service will perform the TPM Platform Auth configuration at the 
ReadyToBoot event.
+
+**/
+VOID
+EFIAPI
+TpmPlatformAuthReadyToBootHandler (
+  VOID
+  );
+
+#endif
diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec 
b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
index a851021c..fc5979db 100644
--- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
+++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
@@ -62,6 +62,8 @@ BoardInitLib|Include/Library/BoardInitLib.h
 MultiBoardInitSupportLib|Include/Library/MultiBoardInitSupportLib.h
 SecBoardInitLib|Include/Library/SecBoardInitLib.h
 
+TpmPlatformAuthLib|Include/Library/TpmPlatformAuthLib.h
+
 TestPointLib|Include/Library/TestPointLib.h
 TestPointCheckLib|Include/Library/TestPointCheckLib.h
 
diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc 
b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
index 5f9363ff..fbfd1e5d 100644
--- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
+++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
@@ -185,7 +185,10 @@
 
 !if gMinPlatformPkgTokenSpaceGuid.PcdTpm2Enable == TRUE
   MinPlatformPkg/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf
-  MinPlatformPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf
+  MinPlatformPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf {
+
+
TpmPlatformAuthLib|MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatformAuthLib.inf
+  }
 !endif
 
 [BuildOptions]
diff --git 
a/Platform/Intel/MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatformAuthLib.c
 
b/Platform/Intel/MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatformAuthLib.c
new file mode 100644
index ..8ac780e1
--- /dev/null
+++ 
b/Platform/Intel/MinPlatformPkg/Tcg/Library/TpmPlatformAuthLib/TpmPlatformAuthLib.c
@@ -0,0 +1,229 @@
+/** @file
+TPM Platform Auth configuration library.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+@par Specification Reference:
+
https://trustedcomputinggroup.org/resource/tcg-tpm-v2-0-provisioning-guidance/
+**/
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+//
+// 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
+
+/**
+  Generate high-quality entropy source through RDRAND.
+
+  @param[in]   LengthSize of the buffer, in bytes, to fill with.
+  @param[out]  Entropy   Pointer to the buffer to store the entropy data.
+
+  @retval EFI_SUCCESSEntropy 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