[PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
@ 2020-09-16  0:58 Zurcher, Christopher J
  2020-09-16  0:59 ` [PATCH v3 1/3] " Zurcher, Christopher J
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Zurcher, Christopher J @ 2020-09-16  0:58 UTC (permalink / raw)
  To: devel; +Cc: Laszlo Ersek, Jiewen Yao, Jian J Wang, Xiaoyu Lu

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

V3 changes:
Added list of valid Digest Names to EvpMdInit() header
Added missing copy of CryptEvpMdNull.c in BaseCryptLibNull folder

V2 changes:
Added NullLib implementation
Added Crypto Service implementation
Rebased Hash2DxeCrypto to use EVP interface instead of low-level functions
Removed unnecessary casts
Added "HashAll" utility function
Merged "New" and "Init" functions as well as "Final" and "Free" functions
  Retained "Init/Update/Final" naming instead of "New/Update/Free" as this
  conforms with common usage

Low-level interfaces to message digest (hash) functions have been deprecated
in OpenSSL 3. In order to upgrade to OpenSSL 3, all direct calls to
low-level functions (such as SHA256_Init() in CryptSha256.c) will need to
be replaced by EVP inteface calls.

References:
  https://www.openssl.org/docs/manmaster/man7/evp.html
  https://www.openssl.org/docs/manmaster/man3/SHA256_Init.html

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>

Christopher J Zurcher (3):
  CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  CryptoPkg: Add EVP to Crypto Service driver interface
  SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP
    interface

 CryptoPkg/CryptoPkg.dsc                                 |   3 +
 CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf         |   1 +
 CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf          |   1 +
 CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf      |   1 +
 CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf          |   1 +
 CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
 CryptoPkg/Include/Library/BaseCryptLib.h                | 129 ++++++++
 CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h    |  10 +
 CryptoPkg/Private/Protocol/Crypto.h                     | 131 ++++++++
 SecurityPkg/Hash2DxeCrypto/Driver.h                     |   1 -
 CryptoPkg/Driver/Crypto.c                               | 152 ++++++++-
 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 257 +++++++++++++++
 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 128 ++++++++
 CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128 ++++++++
 CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c  | 144 ++++++++
 SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c             | 345 ++------------------
 16 files changed, 1117 insertions(+), 316 deletions(-)
 create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
 create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
 create mode 100644 CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c

-- 
2.28.0.windows.1


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

* [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16  0:58 [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J
@ 2020-09-16  0:59 ` Zurcher, Christopher J
  2020-09-16 11:06   ` [edk2-devel] " Laszlo Ersek
  2020-09-16  0:59 ` [PATCH v3 2/3] CryptoPkg: Add EVP to Crypto Service driver interface Zurcher, Christopher J
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Zurcher, Christopher J @ 2020-09-16  0:59 UTC (permalink / raw)
  To: devel; +Cc: Laszlo Ersek, Jiewen Yao, Jian J Wang, Xiaoyu Lu

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

The EVP interface should be used in place of discrete digest function
calls.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
 CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf         |   1 +
 CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf          |   1 +
 CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf      |   1 +
 CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf          |   1 +
 CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
 CryptoPkg/Include/Library/BaseCryptLib.h                | 129 ++++++++++
 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 257 ++++++++++++++++++++
 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 128 ++++++++++
 CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128 ++++++++++
 9 files changed, 647 insertions(+)

diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 4aae2aba95..3968f29412 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -50,6 +50,7 @@
   Pk/CryptAuthenticode.c
   Pk/CryptTs.c
   Pem/CryptPem.c
+  Evp/CryptEvpMd.c
 
   SysCall/CrtWrapper.c
   SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index dc28e3a11d..d0b91716d0 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -57,6 +57,7 @@
   Pk/CryptTsNull.c
   Pem/CryptPemNull.c
   Rand/CryptRandNull.c
+  Evp/CryptEvpMd.c
 
   SysCall/CrtWrapper.c
   SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 5005beed02..9f3accd35b 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -56,6 +56,7 @@
   Pk/CryptAuthenticodeNull.c
   Pk/CryptTsNull.c
   Pem/CryptPem.c
+  Evp/CryptEvpMd.c
 
   SysCall/CrtWrapper.c
   SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 91ec3e03bf..420623cdc6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -54,6 +54,7 @@
   Pk/CryptAuthenticodeNull.c
   Pk/CryptTsNull.c
   Pem/CryptPem.c
+  Evp/CryptEvpMd.c
 
   SysCall/CrtWrapper.c
   SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
index 689af4fedd..542ac2e2e1 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
+++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
@@ -50,6 +50,7 @@
   Pk/CryptTsNull.c
   Pem/CryptPemNull.c
   Rand/CryptRandNull.c
+  Evp/CryptEvpMdNull.c
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
index ae9bde9e37..5e1b408b54 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1012,6 +1012,135 @@ HmacSha256Final (
   OUT     UINT8  *HmacValue
   );
 
+//=====================================================================================
+//    EVP (Envelope) Primitive
+//=====================================================================================
+
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  If DigestName is NULL, then return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
+                              Valid digest names are:
+                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
+                              SM3
+
+  @return  Pointer to the EVP_MD_CTX context that has been allocated and initialized.
+           If DigestName is invalid, returns NULL.
+           If the allocations fails, returns NULL.
+           If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+  IN  CONST CHAR8   *DigestName
+  );
+
+/**
+  Makes a copy of an existing EVP_MD context.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If NewEvpMdContext is NULL, then return FALSE.
+
+  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
+  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
+
+  @retval TRUE   EVP_MD context copy succeeded.
+  @retval FALSE  EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+  IN  CONST VOID    *EvpMdContext,
+  OUT VOID          *NewEvpMdContext
+  );
+
+/**
+  Digests the input data and updates EVP_MD context.
+
+  This function performs EVP digest on a data buffer of the specified size.
+  It can be called multiple times to compute the digest of long or discontinuous data streams.
+  EVP_MD context should be already correctly initialized by EvpMdInit(), and should not
+  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+
+  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
+  @param[in]       Data               Pointer to the buffer containing the data to be digested.
+  @param[in]       DataSize           Size of Data buffer in bytes.
+
+  @retval TRUE   EVP data digest succeeded.
+  @retval FALSE  EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+  IN OUT  VOID        *EvpMdContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  );
+
+/**
+  Completes computation of the EVP digest value.
+  Releases the specified EVP_MD_CTX context.
+
+  This function completes EVP hash computation and retrieves the digest value into
+  the specified memory. After this function has been called, the EVP context cannot
+  be used again.
+  EVP context should be already correctly initialized by EvpMdInit(), and should
+  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If DigestValue is NULL, free the Context then return FALSE.
+
+  @param[in, out]  EvpMdContext   Pointer to the EVP context.
+  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
+
+  @retval TRUE   EVP digest computation succeeded.
+  @retval FALSE  EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+  IN OUT  VOID   *EvpMdContext,
+  OUT     UINT8  *DigestValue
+  );
+
+/**
+  Computes the message digest of an input data buffer.
+
+  This function performs the message digest of a given data buffer, and places
+  the digest value into the specified memory.
+
+  If DigestName is NULL, return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+  If HashValue is NULL, return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name.
+  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
+  @param[in]    DataSize      Size of Data buffer in bytes.
+  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
+
+  @retval TRUE   Digest computation succeeded.
+  @retval FALSE  Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+  IN  CONST CHAR8   *DigestName,
+  IN  CONST VOID    *Data,
+  IN  UINTN         DataSize,
+  OUT UINT8         *HashValue
+  );
+
 //=====================================================================================
 //    Symmetric Cryptography Primitive
 //=====================================================================================
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
new file mode 100644
index 0000000000..b2770a9186
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
@@ -0,0 +1,257 @@
+/** @file
+  EVP MD Wrapper Implementation for OpenSSL.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+#include <openssl/evp.h>
+
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  If DigestName is NULL, then return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
+                              Valid digest names are:
+                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
+                              SM3
+
+  @return  Pointer to the EVP_MD_CTX context that has been allocated and initialized.
+           If DigestName is invalid, returns NULL.
+           If the allocations fails, returns NULL.
+           If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+  IN  CONST CHAR8   *DigestName
+  )
+{
+  EVP_MD    *Digest;
+  VOID      *EvpMdContext;
+
+  //
+  // Check input parameters.
+  //
+  if (DigestName == NULL) {
+    return NULL;
+  }
+
+  //
+  // Allocate EVP_MD_CTX Context
+  //
+  EvpMdContext = EVP_MD_CTX_new ();
+  if (EvpMdContext == NULL) {
+    return NULL;
+  }
+
+  Digest = EVP_get_digestbyname (DigestName);
+  if (Digest == NULL) {
+    return NULL;
+  }
+
+  //
+  // Initialize Context
+  //
+  if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
+    EVP_MD_CTX_free (EvpMdContext);
+    return NULL;
+  }
+
+  return EvpMdContext;
+}
+
+/**
+  Makes a copy of an existing EVP_MD context.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If NewEvpMdContext is NULL, then return FALSE.
+
+  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
+  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
+
+  @retval TRUE   EVP_MD context copy succeeded.
+  @retval FALSE  EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+  IN  CONST VOID    *EvpMdContext,
+  OUT VOID          *NewEvpMdContext
+  )
+{
+  //
+  // Check input parameters.
+  //
+  if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
+    return FALSE;
+  }
+
+  if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+/**
+  Digests the input data and updates EVP_MD context.
+
+  This function performs EVP digest on a data buffer of the specified size.
+  It can be called multiple times to compute the digest of long or discontinuous data streams.
+  EVP_MD context should be already correctly initialized by EvpMdInit(), and should not
+  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+
+  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
+  @param[in]       Data               Pointer to the buffer containing the data to be digested.
+  @param[in]       DataSize           Size of Data buffer in bytes.
+
+  @retval TRUE   EVP data digest succeeded.
+  @retval FALSE  EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+  IN OUT  VOID        *EvpMdContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  )
+{
+  //
+  // Check input parameters.
+  //
+  if (EvpMdContext == NULL) {
+    return FALSE;
+  }
+
+  //
+  // Check invalid parameters, in case only DataLength was checked in OpenSSL
+  //
+  if (Data == NULL && DataSize != 0) {
+    return FALSE;
+  }
+
+  //
+  // OpenSSL EVP digest update
+  //
+  if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+/**
+  Completes computation of the EVP digest value.
+  Releases the specified EVP_MD_CTX context.
+
+  This function completes EVP hash computation and retrieves the digest value into
+  the specified memory. After this function has been called, the EVP context cannot
+  be used again.
+  EVP context should be already correctly initialized by EvpMdInit(), and should
+  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If DigestValue is NULL, free the Context then return FALSE.
+
+  @param[in, out]  EvpMdContext   Pointer to the EVP context.
+  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
+
+  @retval TRUE   EVP digest computation succeeded.
+  @retval FALSE  EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+  IN OUT  VOID   *EvpMdContext,
+  OUT     UINT8  *DigestValue
+  )
+{
+  UINT32    Length;
+  BOOLEAN   ReturnValue;
+
+  ReturnValue = TRUE;
+
+  //
+  // Check input parameters.
+  //
+  if (EvpMdContext == NULL) {
+    return FALSE;
+  }
+  if (DigestValue == NULL) {
+    EVP_MD_CTX_free (EvpMdContext);
+    return FALSE;
+  }
+
+  //
+  // OpenSSL EVP digest finalization
+  //
+  if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
+    ReturnValue = FALSE;
+  }
+
+  //
+  // Free OpenSSL EVP_MD_CTX Context
+  //
+  EVP_MD_CTX_free (EvpMdContext);
+
+  return ReturnValue;
+}
+
+/**
+  Computes the message digest of an input data buffer.
+
+  This function performs the message digest of a given data buffer, and places
+  the digest value into the specified memory.
+
+  If DigestName is NULL, return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+  If HashValue is NULL, return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name.
+  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
+  @param[in]    DataSize      Size of Data buffer in bytes.
+  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
+
+  @retval TRUE   Digest computation succeeded.
+  @retval FALSE  Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+  IN  CONST CHAR8   *DigestName,
+  IN  CONST VOID    *Data,
+  IN  UINTN         DataSize,
+  OUT UINT8         *HashValue
+  )
+{
+  BOOLEAN   Result;
+  VOID      *EvpMdContext;
+
+  EvpMdContext = EvpMdInit (DigestName);
+  if (EvpMdContext == NULL) {
+    return FALSE;
+  }
+
+  Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
+  if (Result == FALSE) {
+    EvpMdFinal (EvpMdContext, NULL);
+    return FALSE;
+  }
+
+  Result = EvpMdFinal (EvpMdContext, HashValue);
+
+  return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+  EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
+                              Valid digest names are:
+                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
+                              SM3
+
+  @return NULL  This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+  IN  CONST CHAR8   *DigestName
+  )
+{
+  ASSERT (FALSE);
+  return NULL;
+}
+
+/**
+  Makes a copy of an existing EVP_MD context.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
+  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+  IN  CONST VOID    *EvpMdContext,
+  OUT VOID          *NewEvpMdContext
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
+
+/**
+  Digests the input data and updates EVP_MD context.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
+  @param[in]       Data               Pointer to the buffer containing the data to be digested.
+  @param[in]       DataSize           Size of Data buffer in bytes.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+  IN OUT  VOID        *EvpMdContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
+
+/**
+  Completes computation of the EVP digest value.
+  Releases the specified EVP_MD_CTX context.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in, out]  EvpMdContext   Pointer to the EVP context.
+  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+  IN OUT  VOID   *EvpMdContext,
+  OUT     UINT8  *DigestValue
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
+
+/**
+  Computes the message digest of an input data buffer.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in]    DigestName    Pointer to the digest name.
+  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
+  @param[in]    DataSize      Size of Data buffer in bytes.
+  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+  IN  CONST CHAR8   *DigestName,
+  IN  CONST VOID    *Data,
+  IN  UINTN         DataSize,
+  OUT UINT8         *HashValue
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+  EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
+                              Valid digest names are:
+                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
+                              SM3
+
+  @return NULL  This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+  IN  CONST CHAR8   *DigestName
+  )
+{
+  ASSERT (FALSE);
+  return NULL;
+}
+
+/**
+  Makes a copy of an existing EVP_MD context.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
+  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+  IN  CONST VOID    *EvpMdContext,
+  OUT VOID          *NewEvpMdContext
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
+
+/**
+  Digests the input data and updates EVP_MD context.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
+  @param[in]       Data               Pointer to the buffer containing the data to be digested.
+  @param[in]       DataSize           Size of Data buffer in bytes.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+  IN OUT  VOID        *EvpMdContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
+
+/**
+  Completes computation of the EVP digest value.
+  Releases the specified EVP_MD_CTX context.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in, out]  EvpMdContext   Pointer to the EVP context.
+  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+  IN OUT  VOID   *EvpMdContext,
+  OUT     UINT8  *DigestValue
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
+
+/**
+  Computes the message digest of an input data buffer.
+
+  Return FALSE to indicate this interface is not supported.
+
+  @param[in]    DigestName    Pointer to the digest name.
+  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
+  @param[in]    DataSize      Size of Data buffer in bytes.
+  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
+
+  @retval FALSE  This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+  IN  CONST CHAR8   *DigestName,
+  IN  CONST VOID    *Data,
+  IN  UINTN         DataSize,
+  OUT UINT8         *HashValue
+  )
+{
+  ASSERT (FALSE);
+  return FALSE;
+}
-- 
2.28.0.windows.1


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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16  0:59 ` [PATCH v3 1/3] " Zurcher, Christopher J
@ 2020-09-16 11:06   ` Laszlo Ersek
  2020-09-16 11:44     ` Yao, Jiewen
  0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2020-09-16 11:06 UTC (permalink / raw)
  To: devel, christopher.j.zurcher; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu

Hello Christopher,

On 09/16/20 02:59, Zurcher, Christopher J wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545
> 
> The EVP interface should be used in place of discrete digest function
> calls.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> ---
>  CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf         |   1 +
>  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf          |   1 +
>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf      |   1 +
>  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf          |   1 +
>  CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
>  CryptoPkg/Include/Library/BaseCryptLib.h                | 129 ++++++++++
>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 257 ++++++++++++++++++++
>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 128 ++++++++++
>  CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128 ++++++++++
>  9 files changed, 647 insertions(+)

I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
is necessary. (If I understand correctly, that file was missing from
your v2 posting.)

But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
superfluous. This file is never referenced in the INF files.

The point of this file would be to allow *some* of the Base / Pei /
Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
only stub implementations). But this isn't what's happening -- all of
the Base / Pei / Runtime / Smm instances are getting the real deal
("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").

(1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
should be dropped. Only the Null instance of the library needs null
versions of the new functions. The Base / Pei / Runtime / Smm instances
don't.

> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> index 4aae2aba95..3968f29412 100644
> --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -50,6 +50,7 @@
>    Pk/CryptAuthenticode.c
>    Pk/CryptTs.c
>    Pem/CryptPem.c
> +  Evp/CryptEvpMd.c
>  
>    SysCall/CrtWrapper.c
>    SysCall/TimerWrapper.c
> diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> index dc28e3a11d..d0b91716d0 100644
> --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> @@ -57,6 +57,7 @@
>    Pk/CryptTsNull.c
>    Pem/CryptPemNull.c
>    Rand/CryptRandNull.c
> +  Evp/CryptEvpMd.c
>  
>    SysCall/CrtWrapper.c
>    SysCall/ConstantTimeClock.c
> diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> index 5005beed02..9f3accd35b 100644
> --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> @@ -56,6 +56,7 @@
>    Pk/CryptAuthenticodeNull.c
>    Pk/CryptTsNull.c
>    Pem/CryptPem.c
> +  Evp/CryptEvpMd.c
>  
>    SysCall/CrtWrapper.c
>    SysCall/TimerWrapper.c
> diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> index 91ec3e03bf..420623cdc6 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> @@ -54,6 +54,7 @@
>    Pk/CryptAuthenticodeNull.c
>    Pk/CryptTsNull.c
>    Pem/CryptPem.c
> +  Evp/CryptEvpMd.c
>  
>    SysCall/CrtWrapper.c
>    SysCall/ConstantTimeClock.c
> diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> index 689af4fedd..542ac2e2e1 100644
> --- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> +++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> @@ -50,6 +50,7 @@
>    Pk/CryptTsNull.c
>    Pem/CryptPemNull.c
>    Rand/CryptRandNull.c
> +  Evp/CryptEvpMdNull.c
>  
>  [Packages]
>    MdePkg/MdePkg.dec
> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
> index ae9bde9e37..5e1b408b54 100644
> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> @@ -1012,6 +1012,135 @@ HmacSha256Final (
>    OUT     UINT8  *HmacValue
>    );
>  
> +//=====================================================================================
> +//    EVP (Envelope) Primitive
> +//=====================================================================================
> +
> +/**
> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
> +
> +  If DigestName is NULL, then return FALSE.
> +
> +  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
> +                              Valid digest names are:
> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> +                              SM3
> +
> +  @return  Pointer to the EVP_MD_CTX context that has been allocated and initialized.
> +           If DigestName is invalid, returns NULL.
> +           If the allocations fails, returns NULL.
> +           If initialization fails, returns NULL.
> +
> +**/
> +VOID *
> +EFIAPI
> +EvpMdInit (
> +  IN  CONST CHAR8   *DigestName
> +  );
> +
> +/**
> +  Makes a copy of an existing EVP_MD context.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If NewEvpMdContext is NULL, then return FALSE.
> +
> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> +
> +  @retval TRUE   EVP_MD context copy succeeded.
> +  @retval FALSE  EVP_MD context copy failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdDuplicate (
> +  IN  CONST VOID    *EvpMdContext,
> +  OUT VOID          *NewEvpMdContext
> +  );
> +
> +/**
> +  Digests the input data and updates EVP_MD context.
> +
> +  This function performs EVP digest on a data buffer of the specified size.
> +  It can be called multiple times to compute the digest of long or discontinuous data streams.
> +  EVP_MD context should be already correctly initialized by EvpMdInit(), and should not
> +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If Data is NULL and DataSize is not zero, return FALSE.
> +
> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> +  @param[in]       Data               Pointer to the buffer containing the data to be digested.
> +  @param[in]       DataSize           Size of Data buffer in bytes.
> +
> +  @retval TRUE   EVP data digest succeeded.
> +  @retval FALSE  EVP data digest failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdUpdate (
> +  IN OUT  VOID        *EvpMdContext,
> +  IN      CONST VOID  *Data,
> +  IN      UINTN       DataSize
> +  );
> +
> +/**
> +  Completes computation of the EVP digest value.
> +  Releases the specified EVP_MD_CTX context.
> +
> +  This function completes EVP hash computation and retrieves the digest value into
> +  the specified memory. After this function has been called, the EVP context cannot
> +  be used again.
> +  EVP context should be already correctly initialized by EvpMdInit(), and should
> +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is undefined.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If DigestValue is NULL, free the Context then return FALSE.
> +
> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.

(2) Please extend the comment on Digest with the following:

  The caller is responsible for providing enough storage for the digest
  algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
  will suffice for storing the digest regardless of the algorithm chosen
  in EvpMdInit().

(EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
advertise it to consumers in edk2.)

> +
> +  @retval TRUE   EVP digest computation succeeded.
> +  @retval FALSE  EVP digest computation failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdFinal (
> +  IN OUT  VOID   *EvpMdContext,
> +  OUT     UINT8  *DigestValue

(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).

> +  );
> +
> +/**
> +  Computes the message digest of an input data buffer.
> +
> +  This function performs the message digest of a given data buffer, and places
> +  the digest value into the specified memory.
> +
> +  If DigestName is NULL, return FALSE.
> +  If Data is NULL and DataSize is not zero, return FALSE.
> +  If HashValue is NULL, return FALSE.
> +
> +  @param[in]    DigestName    Pointer to the digest name.
> +  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
> +  @param[in]    DataSize      Size of Data buffer in bytes.
> +  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
> +
> +  @retval TRUE   Digest computation succeeded.
> +  @retval FALSE  Digest computation failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdHashAll (
> +  IN  CONST CHAR8   *DigestName,
> +  IN  CONST VOID    *Data,
> +  IN  UINTN         DataSize,
> +  OUT UINT8         *HashValue
> +  );
> +
>  //=====================================================================================
>  //    Symmetric Cryptography Primitive
>  //=====================================================================================
> diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> new file mode 100644
> index 0000000000..b2770a9186
> --- /dev/null
> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> @@ -0,0 +1,257 @@
> +/** @file
> +  EVP MD Wrapper Implementation for OpenSSL.
> +
> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "InternalCryptLib.h"
> +#include <openssl/evp.h>
> +
> +/**
> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
> +
> +  If DigestName is NULL, then return FALSE.
> +
> +  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
> +                              Valid digest names are:
> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> +                              SM3
> +
> +  @return  Pointer to the EVP_MD_CTX context that has been allocated and initialized.
> +           If DigestName is invalid, returns NULL.
> +           If the allocations fails, returns NULL.
> +           If initialization fails, returns NULL.
> +
> +**/
> +VOID *
> +EFIAPI
> +EvpMdInit (
> +  IN  CONST CHAR8   *DigestName
> +  )
> +{
> +  EVP_MD    *Digest;
> +  VOID      *EvpMdContext;
> +
> +  //
> +  // Check input parameters.
> +  //
> +  if (DigestName == NULL) {
> +    return NULL;
> +  }
> +
> +  //
> +  // Allocate EVP_MD_CTX Context
> +  //
> +  EvpMdContext = EVP_MD_CTX_new ();
> +  if (EvpMdContext == NULL) {
> +    return NULL;
> +  }
> +
> +  Digest = EVP_get_digestbyname (DigestName);

I think this may not compile with gcc (and correctly so). The pointer
returned by EVP_get_digestbyname() is const-qualified, but with the
assignment, we're throwing away the const-ness.

(4) Please const-qualify the "Digest" local pointer.

> +  if (Digest == NULL) {
> +    return NULL;
> +  }

(5) This is a memory leak I believe; "EvpMdContext" is leaked.

For keeping the control flow simple, consider moving
EVP_get_digestbyname() above EVP_MD_CTX_new().

> +
> +  //
> +  // Initialize Context
> +  //
> +  if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
> +    EVP_MD_CTX_free (EvpMdContext);
> +    return NULL;
> +  }
> +
> +  return EvpMdContext;
> +}
> +
> +/**
> +  Makes a copy of an existing EVP_MD context.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If NewEvpMdContext is NULL, then return FALSE.
> +
> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> +
> +  @retval TRUE   EVP_MD context copy succeeded.
> +  @retval FALSE  EVP_MD context copy failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdDuplicate (
> +  IN  CONST VOID    *EvpMdContext,
> +  OUT VOID          *NewEvpMdContext
> +  )
> +{
> +  //
> +  // Check input parameters.
> +  //
> +  if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
> +    return FALSE;
> +  }
> +
> +  if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}

(6) Can you please confirm that the caller is supposed to initialize
"NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?

> +
> +/**
> +  Digests the input data and updates EVP_MD context.
> +
> +  This function performs EVP digest on a data buffer of the specified size.
> +  It can be called multiple times to compute the digest of long or discontinuous data streams.
> +  EVP_MD context should be already correctly initialized by EvpMdInit(), and should not
> +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If Data is NULL and DataSize is not zero, return FALSE.
> +
> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> +  @param[in]       Data               Pointer to the buffer containing the data to be digested.
> +  @param[in]       DataSize           Size of Data buffer in bytes.
> +
> +  @retval TRUE   EVP data digest succeeded.
> +  @retval FALSE  EVP data digest failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdUpdate (
> +  IN OUT  VOID        *EvpMdContext,
> +  IN      CONST VOID  *Data,
> +  IN      UINTN       DataSize
> +  )
> +{
> +  //
> +  // Check input parameters.
> +  //
> +  if (EvpMdContext == NULL) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check invalid parameters, in case only DataLength was checked in OpenSSL
> +  //
> +  if (Data == NULL && DataSize != 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // OpenSSL EVP digest update
> +  //
> +  if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +/**
> +  Completes computation of the EVP digest value.
> +  Releases the specified EVP_MD_CTX context.
> +
> +  This function completes EVP hash computation and retrieves the digest value into
> +  the specified memory. After this function has been called, the EVP context cannot
> +  be used again.
> +  EVP context should be already correctly initialized by EvpMdInit(), and should
> +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is undefined.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If DigestValue is NULL, free the Context then return FALSE.
> +
> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
> +
> +  @retval TRUE   EVP digest computation succeeded.
> +  @retval FALSE  EVP digest computation failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdFinal (
> +  IN OUT  VOID   *EvpMdContext,
> +  OUT     UINT8  *DigestValue
> +  )
> +{
> +  UINT32    Length;
> +  BOOLEAN   ReturnValue;
> +
> +  ReturnValue = TRUE;
> +
> +  //
> +  // Check input parameters.
> +  //
> +  if (EvpMdContext == NULL) {
> +    return FALSE;
> +  }
> +  if (DigestValue == NULL) {
> +    EVP_MD_CTX_free (EvpMdContext);
> +    return FALSE;
> +  }
> +
> +  //
> +  // OpenSSL EVP digest finalization
> +  //
> +  if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
> +    ReturnValue = FALSE;
> +  }


(7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
deals fine with the third parameter being NULL, according to the docs
(and the code).


> +
> +  //
> +  // Free OpenSSL EVP_MD_CTX Context
> +  //
> +  EVP_MD_CTX_free (EvpMdContext);
> +
> +  return ReturnValue;
> +}
> +
> +/**
> +  Computes the message digest of an input data buffer.
> +
> +  This function performs the message digest of a given data buffer, and places
> +  the digest value into the specified memory.
> +
> +  If DigestName is NULL, return FALSE.
> +  If Data is NULL and DataSize is not zero, return FALSE.
> +  If HashValue is NULL, return FALSE.
> +
> +  @param[in]    DigestName    Pointer to the digest name.
> +  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
> +  @param[in]    DataSize      Size of Data buffer in bytes.
> +  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
> +
> +  @retval TRUE   Digest computation succeeded.
> +  @retval FALSE  Digest computation failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdHashAll (
> +  IN  CONST CHAR8   *DigestName,
> +  IN  CONST VOID    *Data,
> +  IN  UINTN         DataSize,
> +  OUT UINT8         *HashValue
> +  )
> +{
> +  BOOLEAN   Result;
> +  VOID      *EvpMdContext;
> +
> +  EvpMdContext = EvpMdInit (DigestName);
> +  if (EvpMdContext == NULL) {
> +    return FALSE;
> +  }
> +
> +  Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
> +  if (Result == FALSE) {

(8) Style: please write (!Result).


> +    EvpMdFinal (EvpMdContext, NULL);
> +    return FALSE;
> +  }
> +
> +  Result = EvpMdFinal (EvpMdContext, HashValue);
> +
> +  return Result;
> +}
> diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> new file mode 100644
> index 0000000000..038f63801f
> --- /dev/null
> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> @@ -0,0 +1,128 @@
> +/** @file
> +  EVP MD Wrapper Null Library.
> +
> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "InternalCryptLib.h"
> +
> +/**
> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
> +                              Valid digest names are:
> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> +                              SM3
> +
> +  @return NULL  This interface is not supported.
> +
> +**/
> +VOID *
> +EFIAPI
> +EvpMdInit (
> +  IN  CONST CHAR8   *DigestName
> +  )
> +{
> +  ASSERT (FALSE);
> +  return NULL;
> +}
> +
> +/**
> +  Makes a copy of an existing EVP_MD context.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdDuplicate (
> +  IN  CONST VOID    *EvpMdContext,
> +  OUT VOID          *NewEvpMdContext
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Digests the input data and updates EVP_MD context.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> +  @param[in]       Data               Pointer to the buffer containing the data to be digested.
> +  @param[in]       DataSize           Size of Data buffer in bytes.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdUpdate (
> +  IN OUT  VOID        *EvpMdContext,
> +  IN      CONST VOID  *Data,
> +  IN      UINTN       DataSize
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Completes computation of the EVP digest value.
> +  Releases the specified EVP_MD_CTX context.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdFinal (
> +  IN OUT  VOID   *EvpMdContext,
> +  OUT     UINT8  *DigestValue
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Computes the message digest of an input data buffer.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in]    DigestName    Pointer to the digest name.
> +  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
> +  @param[in]    DataSize      Size of Data buffer in bytes.
> +  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdHashAll (
> +  IN  CONST CHAR8   *DigestName,
> +  IN  CONST VOID    *Data,
> +  IN  UINTN         DataSize,
> +  OUT UINT8         *HashValue
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> new file mode 100644
> index 0000000000..038f63801f
> --- /dev/null
> +++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> @@ -0,0 +1,128 @@
> +/** @file
> +  EVP MD Wrapper Null Library.
> +
> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "InternalCryptLib.h"
> +
> +/**
> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
> +                              Valid digest names are:
> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> +                              SM3
> +
> +  @return NULL  This interface is not supported.
> +
> +**/
> +VOID *
> +EFIAPI
> +EvpMdInit (
> +  IN  CONST CHAR8   *DigestName
> +  )
> +{
> +  ASSERT (FALSE);
> +  return NULL;
> +}
> +
> +/**
> +  Makes a copy of an existing EVP_MD context.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdDuplicate (
> +  IN  CONST VOID    *EvpMdContext,
> +  OUT VOID          *NewEvpMdContext
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Digests the input data and updates EVP_MD context.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> +  @param[in]       Data               Pointer to the buffer containing the data to be digested.
> +  @param[in]       DataSize           Size of Data buffer in bytes.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdUpdate (
> +  IN OUT  VOID        *EvpMdContext,
> +  IN      CONST VOID  *Data,
> +  IN      UINTN       DataSize
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Completes computation of the EVP digest value.
> +  Releases the specified EVP_MD_CTX context.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdFinal (
> +  IN OUT  VOID   *EvpMdContext,
> +  OUT     UINT8  *DigestValue
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Computes the message digest of an input data buffer.
> +
> +  Return FALSE to indicate this interface is not supported.
> +
> +  @param[in]    DigestName    Pointer to the digest name.
> +  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
> +  @param[in]    DataSize      Size of Data buffer in bytes.
> +  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
> +
> +  @retval FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdHashAll (
> +  IN  CONST CHAR8   *DigestName,
> +  IN  CONST VOID    *Data,
> +  IN  UINTN         DataSize,
> +  OUT UINT8         *HashValue
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> 

Thanks,
Laszlo


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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16 11:06   ` [edk2-devel] " Laszlo Ersek
@ 2020-09-16 11:44     ` Yao, Jiewen
  2020-09-16 13:35       ` Laszlo Ersek
  0 siblings, 1 reply; 13+ messages in thread
From: Yao, Jiewen @ 2020-09-16 11:44 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io, Zurcher, Christopher J
  Cc: Wang, Jian J, Lu, XiaoyuX

Hi Laszlo
I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
If we need follow this, then we need add OPTIONAL to almost all pointer, which is unnecessary.

> +BOOLEAN
> +EFIAPI
> +EvpMdFinal (
> +  IN OUT  VOID   *EvpMdContext,
> +  OUT     UINT8  *DigestValue

(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).




> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, September 16, 2020 7:07 PM
> To: devel@edk2.groups.io; Zurcher, Christopher J
> <christopher.j.zurcher@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> 
> Hello Christopher,
> 
> On 09/16/20 02:59, Zurcher, Christopher J wrote:
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545
> >
> > The EVP interface should be used in place of discrete digest function
> > calls.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> > ---
> >  CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf         |   1 +
> >  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf          |   1 +
> >  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf      |   1 +
> >  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf          |   1 +
> >  CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
> >  CryptoPkg/Include/Library/BaseCryptLib.h                | 129 ++++++++++
> >  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 257
> ++++++++++++++++++++
> >  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 128 ++++++++++
> >  CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128 ++++++++++
> >  9 files changed, 647 insertions(+)
> 
> I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
> is necessary. (If I understand correctly, that file was missing from
> your v2 posting.)
> 
> But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
> superfluous. This file is never referenced in the INF files.
> 
> The point of this file would be to allow *some* of the Base / Pei /
> Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
> only stub implementations). But this isn't what's happening -- all of
> the Base / Pei / Runtime / Smm instances are getting the real deal
> ("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").
> 
> (1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
> should be dropped. Only the Null instance of the library needs null
> versions of the new functions. The Base / Pei / Runtime / Smm instances
> don't.
> 
> >
> > diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > index 4aae2aba95..3968f29412 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > @@ -50,6 +50,7 @@
> >    Pk/CryptAuthenticode.c
> >    Pk/CryptTs.c
> >    Pem/CryptPem.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/TimerWrapper.c
> > diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > index dc28e3a11d..d0b91716d0 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> > @@ -57,6 +57,7 @@
> >    Pk/CryptTsNull.c
> >    Pem/CryptPemNull.c
> >    Rand/CryptRandNull.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/ConstantTimeClock.c
> > diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > index 5005beed02..9f3accd35b 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > @@ -56,6 +56,7 @@
> >    Pk/CryptAuthenticodeNull.c
> >    Pk/CryptTsNull.c
> >    Pem/CryptPem.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/TimerWrapper.c
> > diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> > index 91ec3e03bf..420623cdc6 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> > +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> > @@ -54,6 +54,7 @@
> >    Pk/CryptAuthenticodeNull.c
> >    Pk/CryptTsNull.c
> >    Pem/CryptPem.c
> > +  Evp/CryptEvpMd.c
> >
> >    SysCall/CrtWrapper.c
> >    SysCall/ConstantTimeClock.c
> > diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> > index 689af4fedd..542ac2e2e1 100644
> > --- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> > +++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> > @@ -50,6 +50,7 @@
> >    Pk/CryptTsNull.c
> >    Pem/CryptPemNull.c
> >    Rand/CryptRandNull.c
> > +  Evp/CryptEvpMdNull.c
> >
> >  [Packages]
> >    MdePkg/MdePkg.dec
> > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
> b/CryptoPkg/Include/Library/BaseCryptLib.h
> > index ae9bde9e37..5e1b408b54 100644
> > --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> > @@ -1012,6 +1012,135 @@ HmacSha256Final (
> >    OUT     UINT8  *HmacValue
> >    );
> >
> >
> +//==============================================================
> =======================
> > +//    EVP (Envelope) Primitive
> >
> +//==============================================================
> =======================
> > +
> > +/**
> > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> use.
> > +
> > +  If DigestName is NULL, then return FALSE.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> terminated ASCII string.
> > +                              Valid digest names are:
> > +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> > +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> > +                              SM3
> > +
> > +  @return  Pointer to the EVP_MD_CTX context that has been allocated and
> initialized.
> > +           If DigestName is invalid, returns NULL.
> > +           If the allocations fails, returns NULL.
> > +           If initialization fails, returns NULL.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdInit (
> > +  IN  CONST CHAR8   *DigestName
> > +  );
> > +
> > +/**
> > +  Makes a copy of an existing EVP_MD context.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If NewEvpMdContext is NULL, then return FALSE.
> > +
> > +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> > +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> > +
> > +  @retval TRUE   EVP_MD context copy succeeded.
> > +  @retval FALSE  EVP_MD context copy failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdDuplicate (
> > +  IN  CONST VOID    *EvpMdContext,
> > +  OUT VOID          *NewEvpMdContext
> > +  );
> > +
> > +/**
> > +  Digests the input data and updates EVP_MD context.
> > +
> > +  This function performs EVP digest on a data buffer of the specified size.
> > +  It can be called multiple times to compute the digest of long or
> discontinuous data streams.
> > +  EVP_MD context should be already correctly initialized by EvpMdInit(), and
> should not
> > +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If Data is NULL and DataSize is not zero, return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> > +  @param[in]       Data               Pointer to the buffer containing the data to be
> digested.
> > +  @param[in]       DataSize           Size of Data buffer in bytes.
> > +
> > +  @retval TRUE   EVP data digest succeeded.
> > +  @retval FALSE  EVP data digest failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdUpdate (
> > +  IN OUT  VOID        *EvpMdContext,
> > +  IN      CONST VOID  *Data,
> > +  IN      UINTN       DataSize
> > +  );
> > +
> > +/**
> > +  Completes computation of the EVP digest value.
> > +  Releases the specified EVP_MD_CTX context.
> > +
> > +  This function completes EVP hash computation and retrieves the digest
> value into
> > +  the specified memory. After this function has been called, the EVP context
> cannot
> > +  be used again.
> > +  EVP context should be already correctly initialized by EvpMdInit(), and
> should
> > +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
> undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If DigestValue is NULL, free the Context then return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> > +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
> value.
> 
> (2) Please extend the comment on Digest with the following:
> 
>   The caller is responsible for providing enough storage for the digest
>   algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
>   will suffice for storing the digest regardless of the algorithm chosen
>   in EvpMdInit().
> 
> (EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
> advertise it to consumers in edk2.)
> 
> > +
> > +  @retval TRUE   EVP digest computation succeeded.
> > +  @retval FALSE  EVP digest computation failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdFinal (
> > +  IN OUT  VOID   *EvpMdContext,
> > +  OUT     UINT8  *DigestValue
> 
> (3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
> deliberately permitted (for just freeing the context).
> 
> > +  );
> > +
> > +/**
> > +  Computes the message digest of an input data buffer.
> > +
> > +  This function performs the message digest of a given data buffer, and places
> > +  the digest value into the specified memory.
> > +
> > +  If DigestName is NULL, return FALSE.
> > +  If Data is NULL and DataSize is not zero, return FALSE.
> > +  If HashValue is NULL, return FALSE.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +  @param[in]    Data          Pointer to the buffer containing the data to be
> hashed.
> > +  @param[in]    DataSize      Size of Data buffer in bytes.
> > +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> value.
> > +
> > +  @retval TRUE   Digest computation succeeded.
> > +  @retval FALSE  Digest computation failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdHashAll (
> > +  IN  CONST CHAR8   *DigestName,
> > +  IN  CONST VOID    *Data,
> > +  IN  UINTN         DataSize,
> > +  OUT UINT8         *HashValue
> > +  );
> > +
> >
> //===============================================================
> ======================
> >  //    Symmetric Cryptography Primitive
> >
> //===============================================================
> ======================
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> > new file mode 100644
> > index 0000000000..b2770a9186
> > --- /dev/null
> > +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> > @@ -0,0 +1,257 @@
> > +/** @file
> > +  EVP MD Wrapper Implementation for OpenSSL.
> > +
> > +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "InternalCryptLib.h"
> > +#include <openssl/evp.h>
> > +
> > +/**
> > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> use.
> > +
> > +  If DigestName is NULL, then return FALSE.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> terminated ASCII string.
> > +                              Valid digest names are:
> > +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> > +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> > +                              SM3
> > +
> > +  @return  Pointer to the EVP_MD_CTX context that has been allocated and
> initialized.
> > +           If DigestName is invalid, returns NULL.
> > +           If the allocations fails, returns NULL.
> > +           If initialization fails, returns NULL.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdInit (
> > +  IN  CONST CHAR8   *DigestName
> > +  )
> > +{
> > +  EVP_MD    *Digest;
> > +  VOID      *EvpMdContext;
> > +
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (DigestName == NULL) {
> > +    return NULL;
> > +  }
> > +
> > +  //
> > +  // Allocate EVP_MD_CTX Context
> > +  //
> > +  EvpMdContext = EVP_MD_CTX_new ();
> > +  if (EvpMdContext == NULL) {
> > +    return NULL;
> > +  }
> > +
> > +  Digest = EVP_get_digestbyname (DigestName);
> 
> I think this may not compile with gcc (and correctly so). The pointer
> returned by EVP_get_digestbyname() is const-qualified, but with the
> assignment, we're throwing away the const-ness.
> 
> (4) Please const-qualify the "Digest" local pointer.
> 
> > +  if (Digest == NULL) {
> > +    return NULL;
> > +  }
> 
> (5) This is a memory leak I believe; "EvpMdContext" is leaked.
> 
> For keeping the control flow simple, consider moving
> EVP_get_digestbyname() above EVP_MD_CTX_new().
> 
> > +
> > +  //
> > +  // Initialize Context
> > +  //
> > +  if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
> > +    EVP_MD_CTX_free (EvpMdContext);
> > +    return NULL;
> > +  }
> > +
> > +  return EvpMdContext;
> > +}
> > +
> > +/**
> > +  Makes a copy of an existing EVP_MD context.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If NewEvpMdContext is NULL, then return FALSE.
> > +
> > +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> > +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> > +
> > +  @retval TRUE   EVP_MD context copy succeeded.
> > +  @retval FALSE  EVP_MD context copy failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdDuplicate (
> > +  IN  CONST VOID    *EvpMdContext,
> > +  OUT VOID          *NewEvpMdContext
> > +  )
> > +{
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> 
> (6) Can you please confirm that the caller is supposed to initialize
> "NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?
> 
> > +
> > +/**
> > +  Digests the input data and updates EVP_MD context.
> > +
> > +  This function performs EVP digest on a data buffer of the specified size.
> > +  It can be called multiple times to compute the digest of long or
> discontinuous data streams.
> > +  EVP_MD context should be already correctly initialized by EvpMdInit(), and
> should not
> > +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If Data is NULL and DataSize is not zero, return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> > +  @param[in]       Data               Pointer to the buffer containing the data to be
> digested.
> > +  @param[in]       DataSize           Size of Data buffer in bytes.
> > +
> > +  @retval TRUE   EVP data digest succeeded.
> > +  @retval FALSE  EVP data digest failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdUpdate (
> > +  IN OUT  VOID        *EvpMdContext,
> > +  IN      CONST VOID  *Data,
> > +  IN      UINTN       DataSize
> > +  )
> > +{
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // Check invalid parameters, in case only DataLength was checked in
> OpenSSL
> > +  //
> > +  if (Data == NULL && DataSize != 0) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // OpenSSL EVP digest update
> > +  //
> > +  if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Completes computation of the EVP digest value.
> > +  Releases the specified EVP_MD_CTX context.
> > +
> > +  This function completes EVP hash computation and retrieves the digest
> value into
> > +  the specified memory. After this function has been called, the EVP context
> cannot
> > +  be used again.
> > +  EVP context should be already correctly initialized by EvpMdInit(), and
> should
> > +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
> undefined.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If DigestValue is NULL, free the Context then return FALSE.
> > +
> > +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> > +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
> value.
> > +
> > +  @retval TRUE   EVP digest computation succeeded.
> > +  @retval FALSE  EVP digest computation failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdFinal (
> > +  IN OUT  VOID   *EvpMdContext,
> > +  OUT     UINT8  *DigestValue
> > +  )
> > +{
> > +  UINT32    Length;
> > +  BOOLEAN   ReturnValue;
> > +
> > +  ReturnValue = TRUE;
> > +
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL) {
> > +    return FALSE;
> > +  }
> > +  if (DigestValue == NULL) {
> > +    EVP_MD_CTX_free (EvpMdContext);
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // OpenSSL EVP digest finalization
> > +  //
> > +  if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
> > +    ReturnValue = FALSE;
> > +  }
> 
> 
> (7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
> deals fine with the third parameter being NULL, according to the docs
> (and the code).
> 
> 
> > +
> > +  //
> > +  // Free OpenSSL EVP_MD_CTX Context
> > +  //
> > +  EVP_MD_CTX_free (EvpMdContext);
> > +
> > +  return ReturnValue;
> > +}
> > +
> > +/**
> > +  Computes the message digest of an input data buffer.
> > +
> > +  This function performs the message digest of a given data buffer, and places
> > +  the digest value into the specified memory.
> > +
> > +  If DigestName is NULL, return FALSE.
> > +  If Data is NULL and DataSize is not zero, return FALSE.
> > +  If HashValue is NULL, return FALSE.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +  @param[in]    Data          Pointer to the buffer containing the data to be
> hashed.
> > +  @param[in]    DataSize      Size of Data buffer in bytes.
> > +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> value.
> > +
> > +  @retval TRUE   Digest computation succeeded.
> > +  @retval FALSE  Digest computation failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdHashAll (
> > +  IN  CONST CHAR8   *DigestName,
> > +  IN  CONST VOID    *Data,
> > +  IN  UINTN         DataSize,
> > +  OUT UINT8         *HashValue
> > +  )
> > +{
> > +  BOOLEAN   Result;
> > +  VOID      *EvpMdContext;
> > +
> > +  EvpMdContext = EvpMdInit (DigestName);
> > +  if (EvpMdContext == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
> > +  if (Result == FALSE) {
> 
> (8) Style: please write (!Result).
> 
> 
> > +    EvpMdFinal (EvpMdContext, NULL);
> > +    return FALSE;
> > +  }
> > +
> > +  Result = EvpMdFinal (EvpMdContext, HashValue);
> > +
> > +  return Result;
> > +}
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> > new file mode 100644
> > index 0000000000..038f63801f
> > --- /dev/null
> > +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> > @@ -0,0 +1,128 @@
> > +/** @file
> > +  EVP MD Wrapper Null Library.
> > +
> > +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "InternalCryptLib.h"
> > +
> > +/**
> > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> use.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> terminated ASCII string.
> > +                              Valid digest names are:
> > +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> > +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> > +                              SM3
> > +
> > +  @return NULL  This interface is not supported.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdInit (
> > +  IN  CONST CHAR8   *DigestName
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return NULL;
> > +}
> > +
> > +/**
> > +  Makes a copy of an existing EVP_MD context.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> > +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdDuplicate (
> > +  IN  CONST VOID    *EvpMdContext,
> > +  OUT VOID          *NewEvpMdContext
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Digests the input data and updates EVP_MD context.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> > +  @param[in]       Data               Pointer to the buffer containing the data to be
> digested.
> > +  @param[in]       DataSize           Size of Data buffer in bytes.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdUpdate (
> > +  IN OUT  VOID        *EvpMdContext,
> > +  IN      CONST VOID  *Data,
> > +  IN      UINTN       DataSize
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Completes computation of the EVP digest value.
> > +  Releases the specified EVP_MD_CTX context.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> > +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
> value.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdFinal (
> > +  IN OUT  VOID   *EvpMdContext,
> > +  OUT     UINT8  *DigestValue
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Computes the message digest of an input data buffer.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +  @param[in]    Data          Pointer to the buffer containing the data to be
> hashed.
> > +  @param[in]    DataSize      Size of Data buffer in bytes.
> > +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> value.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdHashAll (
> > +  IN  CONST CHAR8   *DigestName,
> > +  IN  CONST VOID    *Data,
> > +  IN  UINTN         DataSize,
> > +  OUT UINT8         *HashValue
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> > new file mode 100644
> > index 0000000000..038f63801f
> > --- /dev/null
> > +++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> > @@ -0,0 +1,128 @@
> > +/** @file
> > +  EVP MD Wrapper Null Library.
> > +
> > +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "InternalCryptLib.h"
> > +
> > +/**
> > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> use.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> terminated ASCII string.
> > +                              Valid digest names are:
> > +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> > +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> > +                              SM3
> > +
> > +  @return NULL  This interface is not supported.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdInit (
> > +  IN  CONST CHAR8   *DigestName
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return NULL;
> > +}
> > +
> > +/**
> > +  Makes a copy of an existing EVP_MD context.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> > +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdDuplicate (
> > +  IN  CONST VOID    *EvpMdContext,
> > +  OUT VOID          *NewEvpMdContext
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Digests the input data and updates EVP_MD context.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> > +  @param[in]       Data               Pointer to the buffer containing the data to be
> digested.
> > +  @param[in]       DataSize           Size of Data buffer in bytes.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdUpdate (
> > +  IN OUT  VOID        *EvpMdContext,
> > +  IN      CONST VOID  *Data,
> > +  IN      UINTN       DataSize
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Completes computation of the EVP digest value.
> > +  Releases the specified EVP_MD_CTX context.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> > +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
> value.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdFinal (
> > +  IN OUT  VOID   *EvpMdContext,
> > +  OUT     UINT8  *DigestValue
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Computes the message digest of an input data buffer.
> > +
> > +  Return FALSE to indicate this interface is not supported.
> > +
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +  @param[in]    Data          Pointer to the buffer containing the data to be
> hashed.
> > +  @param[in]    DataSize      Size of Data buffer in bytes.
> > +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> value.
> > +
> > +  @retval FALSE  This interface is not supported.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdHashAll (
> > +  IN  CONST CHAR8   *DigestName,
> > +  IN  CONST VOID    *Data,
> > +  IN  UINTN         DataSize,
> > +  OUT UINT8         *HashValue
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> >
> 
> Thanks,
> Laszlo


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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16 11:44     ` Yao, Jiewen
@ 2020-09-16 13:35       ` Laszlo Ersek
  2020-09-16 14:17         ` Yao, Jiewen
  0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2020-09-16 13:35 UTC (permalink / raw)
  To: Yao, Jiewen, devel@edk2.groups.io, Zurcher, Christopher J
  Cc: Wang, Jian J, Lu, XiaoyuX

On 09/16/20 13:44, Yao, Jiewen wrote:
> Hi Laszlo
> I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
> If we need follow this, then we need add OPTIONAL to almost all pointer, which is unnecessary.

I'm not suggesting OPTIONAL *only* because NULL is checked. I'm
suggesting OPTIONAL because there is a specific use case related to
that. Please see the comments on the function:

"If DigestValue is NULL, free the Context then return FALSE."

But, anyway, I don't insist. It's not a huge deal. Feel free to ignore
my comment regarding OPTIONAL.

Thanks
Laszlo


> 
>> +BOOLEAN
>> +EFIAPI
>> +EvpMdFinal (
>> +  IN OUT  VOID   *EvpMdContext,
>> +  OUT     UINT8  *DigestValue
> 
> (3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
> deliberately permitted (for just freeing the context).
> 
> 
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, September 16, 2020 7:07 PM
>> To: devel@edk2.groups.io; Zurcher, Christopher J
>> <christopher.j.zurcher@intel.com>
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
>> Lu, XiaoyuX <xiaoyux.lu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
>> (Envelope) Digest interface
>>
>> Hello Christopher,
>>
>> On 09/16/20 02:59, Zurcher, Christopher J wrote:
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545
>>>
>>> The EVP interface should be used in place of discrete digest function
>>> calls.
>>>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
>>> Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
>>> ---
>>>  CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf         |   1 +
>>>  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf          |   1 +
>>>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf      |   1 +
>>>  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf          |   1 +
>>>  CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
>>>  CryptoPkg/Include/Library/BaseCryptLib.h                | 129 ++++++++++
>>>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 257
>> ++++++++++++++++++++
>>>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 128 ++++++++++
>>>  CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128 ++++++++++
>>>  9 files changed, 647 insertions(+)
>>
>> I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
>> is necessary. (If I understand correctly, that file was missing from
>> your v2 posting.)
>>
>> But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
>> superfluous. This file is never referenced in the INF files.
>>
>> The point of this file would be to allow *some* of the Base / Pei /
>> Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
>> only stub implementations). But this isn't what's happening -- all of
>> the Base / Pei / Runtime / Smm instances are getting the real deal
>> ("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").
>>
>> (1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
>> should be dropped. Only the Null instance of the library needs null
>> versions of the new functions. The Base / Pei / Runtime / Smm instances
>> don't.
>>
>>>
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>> b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>>> index 4aae2aba95..3968f29412 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>>> +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>>> @@ -50,6 +50,7 @@
>>>    Pk/CryptAuthenticode.c
>>>    Pk/CryptTs.c
>>>    Pem/CryptPem.c
>>> +  Evp/CryptEvpMd.c
>>>
>>>    SysCall/CrtWrapper.c
>>>    SysCall/TimerWrapper.c
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>> b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>>> index dc28e3a11d..d0b91716d0 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>>> +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>>> @@ -57,6 +57,7 @@
>>>    Pk/CryptTsNull.c
>>>    Pem/CryptPemNull.c
>>>    Rand/CryptRandNull.c
>>> +  Evp/CryptEvpMd.c
>>>
>>>    SysCall/CrtWrapper.c
>>>    SysCall/ConstantTimeClock.c
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>> b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>>> index 5005beed02..9f3accd35b 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>>> +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>>> @@ -56,6 +56,7 @@
>>>    Pk/CryptAuthenticodeNull.c
>>>    Pk/CryptTsNull.c
>>>    Pem/CryptPem.c
>>> +  Evp/CryptEvpMd.c
>>>
>>>    SysCall/CrtWrapper.c
>>>    SysCall/TimerWrapper.c
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
>> b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
>>> index 91ec3e03bf..420623cdc6 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
>>> +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
>>> @@ -54,6 +54,7 @@
>>>    Pk/CryptAuthenticodeNull.c
>>>    Pk/CryptTsNull.c
>>>    Pem/CryptPem.c
>>> +  Evp/CryptEvpMd.c
>>>
>>>    SysCall/CrtWrapper.c
>>>    SysCall/ConstantTimeClock.c
>>> diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
>> b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
>>> index 689af4fedd..542ac2e2e1 100644
>>> --- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
>>> +++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
>>> @@ -50,6 +50,7 @@
>>>    Pk/CryptTsNull.c
>>>    Pem/CryptPemNull.c
>>>    Rand/CryptRandNull.c
>>> +  Evp/CryptEvpMdNull.c
>>>
>>>  [Packages]
>>>    MdePkg/MdePkg.dec
>>> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
>> b/CryptoPkg/Include/Library/BaseCryptLib.h
>>> index ae9bde9e37..5e1b408b54 100644
>>> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
>>> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
>>> @@ -1012,6 +1012,135 @@ HmacSha256Final (
>>>    OUT     UINT8  *HmacValue
>>>    );
>>>
>>>
>> +//==============================================================
>> =======================
>>> +//    EVP (Envelope) Primitive
>>>
>> +//==============================================================
>> =======================
>>> +
>>> +/**
>>> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
>> use.
>>> +
>>> +  If DigestName is NULL, then return FALSE.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
>> terminated ASCII string.
>>> +                              Valid digest names are:
>>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
>>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
>>> +                              SM3
>>> +
>>> +  @return  Pointer to the EVP_MD_CTX context that has been allocated and
>> initialized.
>>> +           If DigestName is invalid, returns NULL.
>>> +           If the allocations fails, returns NULL.
>>> +           If initialization fails, returns NULL.
>>> +
>>> +**/
>>> +VOID *
>>> +EFIAPI
>>> +EvpMdInit (
>>> +  IN  CONST CHAR8   *DigestName
>>> +  );
>>> +
>>> +/**
>>> +  Makes a copy of an existing EVP_MD context.
>>> +
>>> +  If EvpMdContext is NULL, then return FALSE.
>>> +  If NewEvpMdContext is NULL, then return FALSE.
>>> +
>>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
>>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
>>> +
>>> +  @retval TRUE   EVP_MD context copy succeeded.
>>> +  @retval FALSE  EVP_MD context copy failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdDuplicate (
>>> +  IN  CONST VOID    *EvpMdContext,
>>> +  OUT VOID          *NewEvpMdContext
>>> +  );
>>> +
>>> +/**
>>> +  Digests the input data and updates EVP_MD context.
>>> +
>>> +  This function performs EVP digest on a data buffer of the specified size.
>>> +  It can be called multiple times to compute the digest of long or
>> discontinuous data streams.
>>> +  EVP_MD context should be already correctly initialized by EvpMdInit(), and
>> should not
>>> +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
>>> +
>>> +  If EvpMdContext is NULL, then return FALSE.
>>> +  If Data is NULL and DataSize is not zero, return FALSE.
>>> +
>>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
>>> +  @param[in]       Data               Pointer to the buffer containing the data to be
>> digested.
>>> +  @param[in]       DataSize           Size of Data buffer in bytes.
>>> +
>>> +  @retval TRUE   EVP data digest succeeded.
>>> +  @retval FALSE  EVP data digest failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdUpdate (
>>> +  IN OUT  VOID        *EvpMdContext,
>>> +  IN      CONST VOID  *Data,
>>> +  IN      UINTN       DataSize
>>> +  );
>>> +
>>> +/**
>>> +  Completes computation of the EVP digest value.
>>> +  Releases the specified EVP_MD_CTX context.
>>> +
>>> +  This function completes EVP hash computation and retrieves the digest
>> value into
>>> +  the specified memory. After this function has been called, the EVP context
>> cannot
>>> +  be used again.
>>> +  EVP context should be already correctly initialized by EvpMdInit(), and
>> should
>>> +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
>> undefined.
>>> +
>>> +  If EvpMdContext is NULL, then return FALSE.
>>> +  If DigestValue is NULL, free the Context then return FALSE.
>>> +
>>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
>>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
>> value.
>>
>> (2) Please extend the comment on Digest with the following:
>>
>>   The caller is responsible for providing enough storage for the digest
>>   algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
>>   will suffice for storing the digest regardless of the algorithm chosen
>>   in EvpMdInit().
>>
>> (EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
>> advertise it to consumers in edk2.)
>>
>>> +
>>> +  @retval TRUE   EVP digest computation succeeded.
>>> +  @retval FALSE  EVP digest computation failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdFinal (
>>> +  IN OUT  VOID   *EvpMdContext,
>>> +  OUT     UINT8  *DigestValue
>>
>> (3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
>> deliberately permitted (for just freeing the context).
>>
>>> +  );
>>> +
>>> +/**
>>> +  Computes the message digest of an input data buffer.
>>> +
>>> +  This function performs the message digest of a given data buffer, and places
>>> +  the digest value into the specified memory.
>>> +
>>> +  If DigestName is NULL, return FALSE.
>>> +  If Data is NULL and DataSize is not zero, return FALSE.
>>> +  If HashValue is NULL, return FALSE.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name.
>>> +  @param[in]    Data          Pointer to the buffer containing the data to be
>> hashed.
>>> +  @param[in]    DataSize      Size of Data buffer in bytes.
>>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
>> value.
>>> +
>>> +  @retval TRUE   Digest computation succeeded.
>>> +  @retval FALSE  Digest computation failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdHashAll (
>>> +  IN  CONST CHAR8   *DigestName,
>>> +  IN  CONST VOID    *Data,
>>> +  IN  UINTN         DataSize,
>>> +  OUT UINT8         *HashValue
>>> +  );
>>> +
>>>
>> //===============================================================
>> ======================
>>>  //    Symmetric Cryptography Primitive
>>>
>> //===============================================================
>> ======================
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
>> b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
>>> new file mode 100644
>>> index 0000000000..b2770a9186
>>> --- /dev/null
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
>>> @@ -0,0 +1,257 @@
>>> +/** @file
>>> +  EVP MD Wrapper Implementation for OpenSSL.
>>> +
>>> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#include "InternalCryptLib.h"
>>> +#include <openssl/evp.h>
>>> +
>>> +/**
>>> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
>> use.
>>> +
>>> +  If DigestName is NULL, then return FALSE.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
>> terminated ASCII string.
>>> +                              Valid digest names are:
>>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
>>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
>>> +                              SM3
>>> +
>>> +  @return  Pointer to the EVP_MD_CTX context that has been allocated and
>> initialized.
>>> +           If DigestName is invalid, returns NULL.
>>> +           If the allocations fails, returns NULL.
>>> +           If initialization fails, returns NULL.
>>> +
>>> +**/
>>> +VOID *
>>> +EFIAPI
>>> +EvpMdInit (
>>> +  IN  CONST CHAR8   *DigestName
>>> +  )
>>> +{
>>> +  EVP_MD    *Digest;
>>> +  VOID      *EvpMdContext;
>>> +
>>> +  //
>>> +  // Check input parameters.
>>> +  //
>>> +  if (DigestName == NULL) {
>>> +    return NULL;
>>> +  }
>>> +
>>> +  //
>>> +  // Allocate EVP_MD_CTX Context
>>> +  //
>>> +  EvpMdContext = EVP_MD_CTX_new ();
>>> +  if (EvpMdContext == NULL) {
>>> +    return NULL;
>>> +  }
>>> +
>>> +  Digest = EVP_get_digestbyname (DigestName);
>>
>> I think this may not compile with gcc (and correctly so). The pointer
>> returned by EVP_get_digestbyname() is const-qualified, but with the
>> assignment, we're throwing away the const-ness.
>>
>> (4) Please const-qualify the "Digest" local pointer.
>>
>>> +  if (Digest == NULL) {
>>> +    return NULL;
>>> +  }
>>
>> (5) This is a memory leak I believe; "EvpMdContext" is leaked.
>>
>> For keeping the control flow simple, consider moving
>> EVP_get_digestbyname() above EVP_MD_CTX_new().
>>
>>> +
>>> +  //
>>> +  // Initialize Context
>>> +  //
>>> +  if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
>>> +    EVP_MD_CTX_free (EvpMdContext);
>>> +    return NULL;
>>> +  }
>>> +
>>> +  return EvpMdContext;
>>> +}
>>> +
>>> +/**
>>> +  Makes a copy of an existing EVP_MD context.
>>> +
>>> +  If EvpMdContext is NULL, then return FALSE.
>>> +  If NewEvpMdContext is NULL, then return FALSE.
>>> +
>>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
>>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
>>> +
>>> +  @retval TRUE   EVP_MD context copy succeeded.
>>> +  @retval FALSE  EVP_MD context copy failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdDuplicate (
>>> +  IN  CONST VOID    *EvpMdContext,
>>> +  OUT VOID          *NewEvpMdContext
>>> +  )
>>> +{
>>> +  //
>>> +  // Check input parameters.
>>> +  //
>>> +  if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  return TRUE;
>>> +}
>>
>> (6) Can you please confirm that the caller is supposed to initialize
>> "NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?
>>
>>> +
>>> +/**
>>> +  Digests the input data and updates EVP_MD context.
>>> +
>>> +  This function performs EVP digest on a data buffer of the specified size.
>>> +  It can be called multiple times to compute the digest of long or
>> discontinuous data streams.
>>> +  EVP_MD context should be already correctly initialized by EvpMdInit(), and
>> should not
>>> +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
>>> +
>>> +  If EvpMdContext is NULL, then return FALSE.
>>> +  If Data is NULL and DataSize is not zero, return FALSE.
>>> +
>>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
>>> +  @param[in]       Data               Pointer to the buffer containing the data to be
>> digested.
>>> +  @param[in]       DataSize           Size of Data buffer in bytes.
>>> +
>>> +  @retval TRUE   EVP data digest succeeded.
>>> +  @retval FALSE  EVP data digest failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdUpdate (
>>> +  IN OUT  VOID        *EvpMdContext,
>>> +  IN      CONST VOID  *Data,
>>> +  IN      UINTN       DataSize
>>> +  )
>>> +{
>>> +  //
>>> +  // Check input parameters.
>>> +  //
>>> +  if (EvpMdContext == NULL) {
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  //
>>> +  // Check invalid parameters, in case only DataLength was checked in
>> OpenSSL
>>> +  //
>>> +  if (Data == NULL && DataSize != 0) {
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  //
>>> +  // OpenSSL EVP digest update
>>> +  //
>>> +  if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  return TRUE;
>>> +}
>>> +
>>> +/**
>>> +  Completes computation of the EVP digest value.
>>> +  Releases the specified EVP_MD_CTX context.
>>> +
>>> +  This function completes EVP hash computation and retrieves the digest
>> value into
>>> +  the specified memory. After this function has been called, the EVP context
>> cannot
>>> +  be used again.
>>> +  EVP context should be already correctly initialized by EvpMdInit(), and
>> should
>>> +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
>> undefined.
>>> +
>>> +  If EvpMdContext is NULL, then return FALSE.
>>> +  If DigestValue is NULL, free the Context then return FALSE.
>>> +
>>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
>>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
>> value.
>>> +
>>> +  @retval TRUE   EVP digest computation succeeded.
>>> +  @retval FALSE  EVP digest computation failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdFinal (
>>> +  IN OUT  VOID   *EvpMdContext,
>>> +  OUT     UINT8  *DigestValue
>>> +  )
>>> +{
>>> +  UINT32    Length;
>>> +  BOOLEAN   ReturnValue;
>>> +
>>> +  ReturnValue = TRUE;
>>> +
>>> +  //
>>> +  // Check input parameters.
>>> +  //
>>> +  if (EvpMdContext == NULL) {
>>> +    return FALSE;
>>> +  }
>>> +  if (DigestValue == NULL) {
>>> +    EVP_MD_CTX_free (EvpMdContext);
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  //
>>> +  // OpenSSL EVP digest finalization
>>> +  //
>>> +  if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
>>> +    ReturnValue = FALSE;
>>> +  }
>>
>>
>> (7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
>> deals fine with the third parameter being NULL, according to the docs
>> (and the code).
>>
>>
>>> +
>>> +  //
>>> +  // Free OpenSSL EVP_MD_CTX Context
>>> +  //
>>> +  EVP_MD_CTX_free (EvpMdContext);
>>> +
>>> +  return ReturnValue;
>>> +}
>>> +
>>> +/**
>>> +  Computes the message digest of an input data buffer.
>>> +
>>> +  This function performs the message digest of a given data buffer, and places
>>> +  the digest value into the specified memory.
>>> +
>>> +  If DigestName is NULL, return FALSE.
>>> +  If Data is NULL and DataSize is not zero, return FALSE.
>>> +  If HashValue is NULL, return FALSE.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name.
>>> +  @param[in]    Data          Pointer to the buffer containing the data to be
>> hashed.
>>> +  @param[in]    DataSize      Size of Data buffer in bytes.
>>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
>> value.
>>> +
>>> +  @retval TRUE   Digest computation succeeded.
>>> +  @retval FALSE  Digest computation failed.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdHashAll (
>>> +  IN  CONST CHAR8   *DigestName,
>>> +  IN  CONST VOID    *Data,
>>> +  IN  UINTN         DataSize,
>>> +  OUT UINT8         *HashValue
>>> +  )
>>> +{
>>> +  BOOLEAN   Result;
>>> +  VOID      *EvpMdContext;
>>> +
>>> +  EvpMdContext = EvpMdInit (DigestName);
>>> +  if (EvpMdContext == NULL) {
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
>>> +  if (Result == FALSE) {
>>
>> (8) Style: please write (!Result).
>>
>>
>>> +    EvpMdFinal (EvpMdContext, NULL);
>>> +    return FALSE;
>>> +  }
>>> +
>>> +  Result = EvpMdFinal (EvpMdContext, HashValue);
>>> +
>>> +  return Result;
>>> +}
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
>> b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
>>> new file mode 100644
>>> index 0000000000..038f63801f
>>> --- /dev/null
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
>>> @@ -0,0 +1,128 @@
>>> +/** @file
>>> +  EVP MD Wrapper Null Library.
>>> +
>>> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#include "InternalCryptLib.h"
>>> +
>>> +/**
>>> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
>> use.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
>> terminated ASCII string.
>>> +                              Valid digest names are:
>>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
>>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
>>> +                              SM3
>>> +
>>> +  @return NULL  This interface is not supported.
>>> +
>>> +**/
>>> +VOID *
>>> +EFIAPI
>>> +EvpMdInit (
>>> +  IN  CONST CHAR8   *DigestName
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return NULL;
>>> +}
>>> +
>>> +/**
>>> +  Makes a copy of an existing EVP_MD context.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
>>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdDuplicate (
>>> +  IN  CONST VOID    *EvpMdContext,
>>> +  OUT VOID          *NewEvpMdContext
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>> +
>>> +/**
>>> +  Digests the input data and updates EVP_MD context.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
>>> +  @param[in]       Data               Pointer to the buffer containing the data to be
>> digested.
>>> +  @param[in]       DataSize           Size of Data buffer in bytes.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdUpdate (
>>> +  IN OUT  VOID        *EvpMdContext,
>>> +  IN      CONST VOID  *Data,
>>> +  IN      UINTN       DataSize
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>> +
>>> +/**
>>> +  Completes computation of the EVP digest value.
>>> +  Releases the specified EVP_MD_CTX context.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
>>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
>> value.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdFinal (
>>> +  IN OUT  VOID   *EvpMdContext,
>>> +  OUT     UINT8  *DigestValue
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>> +
>>> +/**
>>> +  Computes the message digest of an input data buffer.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name.
>>> +  @param[in]    Data          Pointer to the buffer containing the data to be
>> hashed.
>>> +  @param[in]    DataSize      Size of Data buffer in bytes.
>>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
>> value.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdHashAll (
>>> +  IN  CONST CHAR8   *DigestName,
>>> +  IN  CONST VOID    *Data,
>>> +  IN  UINTN         DataSize,
>>> +  OUT UINT8         *HashValue
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>> diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
>> b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
>>> new file mode 100644
>>> index 0000000000..038f63801f
>>> --- /dev/null
>>> +++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
>>> @@ -0,0 +1,128 @@
>>> +/** @file
>>> +  EVP MD Wrapper Null Library.
>>> +
>>> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#include "InternalCryptLib.h"
>>> +
>>> +/**
>>> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
>> use.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
>> terminated ASCII string.
>>> +                              Valid digest names are:
>>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
>>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
>>> +                              SM3
>>> +
>>> +  @return NULL  This interface is not supported.
>>> +
>>> +**/
>>> +VOID *
>>> +EFIAPI
>>> +EvpMdInit (
>>> +  IN  CONST CHAR8   *DigestName
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return NULL;
>>> +}
>>> +
>>> +/**
>>> +  Makes a copy of an existing EVP_MD context.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
>>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdDuplicate (
>>> +  IN  CONST VOID    *EvpMdContext,
>>> +  OUT VOID          *NewEvpMdContext
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>> +
>>> +/**
>>> +  Digests the input data and updates EVP_MD context.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
>>> +  @param[in]       Data               Pointer to the buffer containing the data to be
>> digested.
>>> +  @param[in]       DataSize           Size of Data buffer in bytes.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdUpdate (
>>> +  IN OUT  VOID        *EvpMdContext,
>>> +  IN      CONST VOID  *Data,
>>> +  IN      UINTN       DataSize
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>> +
>>> +/**
>>> +  Completes computation of the EVP digest value.
>>> +  Releases the specified EVP_MD_CTX context.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
>>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP digest
>> value.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdFinal (
>>> +  IN OUT  VOID   *EvpMdContext,
>>> +  OUT     UINT8  *DigestValue
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>> +
>>> +/**
>>> +  Computes the message digest of an input data buffer.
>>> +
>>> +  Return FALSE to indicate this interface is not supported.
>>> +
>>> +  @param[in]    DigestName    Pointer to the digest name.
>>> +  @param[in]    Data          Pointer to the buffer containing the data to be
>> hashed.
>>> +  @param[in]    DataSize      Size of Data buffer in bytes.
>>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
>> value.
>>> +
>>> +  @retval FALSE  This interface is not supported.
>>> +
>>> +**/
>>> +BOOLEAN
>>> +EFIAPI
>>> +EvpMdHashAll (
>>> +  IN  CONST CHAR8   *DigestName,
>>> +  IN  CONST VOID    *Data,
>>> +  IN  UINTN         DataSize,
>>> +  OUT UINT8         *HashValue
>>> +  )
>>> +{
>>> +  ASSERT (FALSE);
>>> +  return FALSE;
>>> +}
>>>
>>
>> Thanks,
>> Laszlo
> 


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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16 13:35       ` Laszlo Ersek
@ 2020-09-16 14:17         ` Yao, Jiewen
  2020-09-16 16:04           ` Laszlo Ersek
  0 siblings, 1 reply; 13+ messages in thread
From: Yao, Jiewen @ 2020-09-16 14:17 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io, Zurcher, Christopher J
  Cc: Wang, Jian J, Lu, XiaoyuX

Thank you Laszlo.
You forced me to read the code again and more carefully. :-)

I believe I understand why you use NULL to free the context now - to support error path.

First, I did have some new thought for EVP API. Let’s consider 3 cases, 1) Hash all data one time, 2) Hash update data multiple times, 3) Error during update.

A. In current design, the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)

B. We can auto free context in EvpMdUpdate in error path  - the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR

C: We can use New/Free style - similar to HMAC
1) EvpMdHashAll (Digest)
2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest), EvpMdFree
3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree

I checked below APIs:
1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate, EVP_DigestFinal_ex, EVP_MD_CTX_free)
2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret, mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret, mbedtls_sha256_free)
3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey, HmacSha256Update, HmacSha256Final, HmacSha256Free)

All APIs include free function to free the context, I don’t think it is a bad idea to have EvpMdFree() API.
I would like to propose option - C.


Second, I believe EVP is just something in openssl. It does not mean that we *have to* design API to follow openssl.
After rethink the size impact, I do have concern to merge all Hash implementation together in one function.
It might means nothing if the crypto library is based upon openssl.
But if the cryptolib implementation is based upon other crypto, such as Intel IPP (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCommonPkg/Library/IppCryptoLib) or mbedTls (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then we can NOT get size benefit by separating the hash algorithm.


I would like to propose separate EvpMdxxx.
EvpMdNew -> Sha256New, Sha384New
EvpMdInit -> Sha256Init, Sha384Init
EvpMdUpdate -> Sha256Update, Sha384Update
EvpMdFinal -> Sha256Final, Sha384Final
EvpMdFree -> Sha256Free, Sha384Free

We can do similar thing with Hmac, to deprecate Sha256GetContextSize() API, and replace caller with Sha256New() / Sha384Free()

Thank you
Yao Jiewen


> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, September 16, 2020 9:36 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher,
> Christopher J <christopher.j.zurcher@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> 
> On 09/16/20 13:44, Yao, Jiewen wrote:
> > Hi Laszlo
> > I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
> > If we need follow this, then we need add OPTIONAL to almost all pointer,
> which is unnecessary.
> 
> I'm not suggesting OPTIONAL *only* because NULL is checked. I'm
> suggesting OPTIONAL because there is a specific use case related to
> that. Please see the comments on the function:
> 
> "If DigestValue is NULL, free the Context then return FALSE."
> 
> But, anyway, I don't insist. It's not a huge deal. Feel free to ignore
> my comment regarding OPTIONAL.
> 
> Thanks
> Laszlo
> 
> 
> >
> >> +BOOLEAN
> >> +EFIAPI
> >> +EvpMdFinal (
> >> +  IN OUT  VOID   *EvpMdContext,
> >> +  OUT     UINT8  *DigestValue
> >
> > (3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
> > deliberately permitted (for just freeing the context).
> >
> >
> >
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Wednesday, September 16, 2020 7:07 PM
> >> To: devel@edk2.groups.io; Zurcher, Christopher J
> >> <christopher.j.zurcher@intel.com>
> >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>;
> >> Lu, XiaoyuX <xiaoyux.lu@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> >> (Envelope) Digest interface
> >>
> >> Hello Christopher,
> >>
> >> On 09/16/20 02:59, Zurcher, Christopher J wrote:
> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545
> >>>
> >>> The EVP interface should be used in place of discrete digest function
> >>> calls.
> >>>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> >>> Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> >>> ---
> >>>  CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf         |   1 +
> >>>  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf          |   1 +
> >>>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf      |   1 +
> >>>  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf          |   1 +
> >>>  CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
> >>>  CryptoPkg/Include/Library/BaseCryptLib.h                | 129 ++++++++++
> >>>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 257
> >> ++++++++++++++++++++
> >>>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 128
> ++++++++++
> >>>  CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128
> ++++++++++
> >>>  9 files changed, 647 insertions(+)
> >>
> >> I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
> >> is necessary. (If I understand correctly, that file was missing from
> >> your v2 posting.)
> >>
> >> But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
> >> superfluous. This file is never referenced in the INF files.
> >>
> >> The point of this file would be to allow *some* of the Base / Pei /
> >> Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
> >> only stub implementations). But this isn't what's happening -- all of
> >> the Base / Pei / Runtime / Smm instances are getting the real deal
> >> ("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").
> >>
> >> (1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
> >> should be dropped. Only the Null instance of the library needs null
> >> versions of the new functions. The Base / Pei / Runtime / Smm instances
> >> don't.
> >>
> >>>
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >> b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >>> index 4aae2aba95..3968f29412 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> >>> @@ -50,6 +50,7 @@
> >>>    Pk/CryptAuthenticode.c
> >>>    Pk/CryptTs.c
> >>>    Pem/CryptPem.c
> >>> +  Evp/CryptEvpMd.c
> >>>
> >>>    SysCall/CrtWrapper.c
> >>>    SysCall/TimerWrapper.c
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >> b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >>> index dc28e3a11d..d0b91716d0 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> >>> @@ -57,6 +57,7 @@
> >>>    Pk/CryptTsNull.c
> >>>    Pem/CryptPemNull.c
> >>>    Rand/CryptRandNull.c
> >>> +  Evp/CryptEvpMd.c
> >>>
> >>>    SysCall/CrtWrapper.c
> >>>    SysCall/ConstantTimeClock.c
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> >> b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> >>> index 5005beed02..9f3accd35b 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> >>> @@ -56,6 +56,7 @@
> >>>    Pk/CryptAuthenticodeNull.c
> >>>    Pk/CryptTsNull.c
> >>>    Pem/CryptPem.c
> >>> +  Evp/CryptEvpMd.c
> >>>
> >>>    SysCall/CrtWrapper.c
> >>>    SysCall/TimerWrapper.c
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> >> b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> >>> index 91ec3e03bf..420623cdc6 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
> >>> @@ -54,6 +54,7 @@
> >>>    Pk/CryptAuthenticodeNull.c
> >>>    Pk/CryptTsNull.c
> >>>    Pem/CryptPem.c
> >>> +  Evp/CryptEvpMd.c
> >>>
> >>>    SysCall/CrtWrapper.c
> >>>    SysCall/ConstantTimeClock.c
> >>> diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> >> b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> >>> index 689af4fedd..542ac2e2e1 100644
> >>> --- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> >>> +++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> >>> @@ -50,6 +50,7 @@
> >>>    Pk/CryptTsNull.c
> >>>    Pem/CryptPemNull.c
> >>>    Rand/CryptRandNull.c
> >>> +  Evp/CryptEvpMdNull.c
> >>>
> >>>  [Packages]
> >>>    MdePkg/MdePkg.dec
> >>> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
> >> b/CryptoPkg/Include/Library/BaseCryptLib.h
> >>> index ae9bde9e37..5e1b408b54 100644
> >>> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> >>> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> >>> @@ -1012,6 +1012,135 @@ HmacSha256Final (
> >>>    OUT     UINT8  *HmacValue
> >>>    );
> >>>
> >>>
> >>
> +//==============================================================
> >> =======================
> >>> +//    EVP (Envelope) Primitive
> >>>
> >>
> +//==============================================================
> >> =======================
> >>> +
> >>> +/**
> >>> +  Allocates and initializes one EVP_MD_CTX context for subsequent
> EVP_MD
> >> use.
> >>> +
> >>> +  If DigestName is NULL, then return FALSE.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> >> terminated ASCII string.
> >>> +                              Valid digest names are:
> >>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> >>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> >>> +                              SM3
> >>> +
> >>> +  @return  Pointer to the EVP_MD_CTX context that has been allocated and
> >> initialized.
> >>> +           If DigestName is invalid, returns NULL.
> >>> +           If the allocations fails, returns NULL.
> >>> +           If initialization fails, returns NULL.
> >>> +
> >>> +**/
> >>> +VOID *
> >>> +EFIAPI
> >>> +EvpMdInit (
> >>> +  IN  CONST CHAR8   *DigestName
> >>> +  );
> >>> +
> >>> +/**
> >>> +  Makes a copy of an existing EVP_MD context.
> >>> +
> >>> +  If EvpMdContext is NULL, then return FALSE.
> >>> +  If NewEvpMdContext is NULL, then return FALSE.
> >>> +
> >>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> >>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> >>> +
> >>> +  @retval TRUE   EVP_MD context copy succeeded.
> >>> +  @retval FALSE  EVP_MD context copy failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdDuplicate (
> >>> +  IN  CONST VOID    *EvpMdContext,
> >>> +  OUT VOID          *NewEvpMdContext
> >>> +  );
> >>> +
> >>> +/**
> >>> +  Digests the input data and updates EVP_MD context.
> >>> +
> >>> +  This function performs EVP digest on a data buffer of the specified size.
> >>> +  It can be called multiple times to compute the digest of long or
> >> discontinuous data streams.
> >>> +  EVP_MD context should be already correctly initialized by EvpMdInit(),
> and
> >> should not
> >>> +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> >>> +
> >>> +  If EvpMdContext is NULL, then return FALSE.
> >>> +  If Data is NULL and DataSize is not zero, return FALSE.
> >>> +
> >>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> >>> +  @param[in]       Data               Pointer to the buffer containing the data to
> be
> >> digested.
> >>> +  @param[in]       DataSize           Size of Data buffer in bytes.
> >>> +
> >>> +  @retval TRUE   EVP data digest succeeded.
> >>> +  @retval FALSE  EVP data digest failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdUpdate (
> >>> +  IN OUT  VOID        *EvpMdContext,
> >>> +  IN      CONST VOID  *Data,
> >>> +  IN      UINTN       DataSize
> >>> +  );
> >>> +
> >>> +/**
> >>> +  Completes computation of the EVP digest value.
> >>> +  Releases the specified EVP_MD_CTX context.
> >>> +
> >>> +  This function completes EVP hash computation and retrieves the digest
> >> value into
> >>> +  the specified memory. After this function has been called, the EVP context
> >> cannot
> >>> +  be used again.
> >>> +  EVP context should be already correctly initialized by EvpMdInit(), and
> >> should
> >>> +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
> >> undefined.
> >>> +
> >>> +  If EvpMdContext is NULL, then return FALSE.
> >>> +  If DigestValue is NULL, free the Context then return FALSE.
> >>> +
> >>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> >>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP
> digest
> >> value.
> >>
> >> (2) Please extend the comment on Digest with the following:
> >>
> >>   The caller is responsible for providing enough storage for the digest
> >>   algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
> >>   will suffice for storing the digest regardless of the algorithm chosen
> >>   in EvpMdInit().
> >>
> >> (EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
> >> advertise it to consumers in edk2.)
> >>
> >>> +
> >>> +  @retval TRUE   EVP digest computation succeeded.
> >>> +  @retval FALSE  EVP digest computation failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdFinal (
> >>> +  IN OUT  VOID   *EvpMdContext,
> >>> +  OUT     UINT8  *DigestValue
> >>
> >> (3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
> >> deliberately permitted (for just freeing the context).
> >>
> >>> +  );
> >>> +
> >>> +/**
> >>> +  Computes the message digest of an input data buffer.
> >>> +
> >>> +  This function performs the message digest of a given data buffer, and
> places
> >>> +  the digest value into the specified memory.
> >>> +
> >>> +  If DigestName is NULL, return FALSE.
> >>> +  If Data is NULL and DataSize is not zero, return FALSE.
> >>> +  If HashValue is NULL, return FALSE.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name.
> >>> +  @param[in]    Data          Pointer to the buffer containing the data to be
> >> hashed.
> >>> +  @param[in]    DataSize      Size of Data buffer in bytes.
> >>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> >> value.
> >>> +
> >>> +  @retval TRUE   Digest computation succeeded.
> >>> +  @retval FALSE  Digest computation failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdHashAll (
> >>> +  IN  CONST CHAR8   *DigestName,
> >>> +  IN  CONST VOID    *Data,
> >>> +  IN  UINTN         DataSize,
> >>> +  OUT UINT8         *HashValue
> >>> +  );
> >>> +
> >>>
> >>
> //===============================================================
> >> ======================
> >>>  //    Symmetric Cryptography Primitive
> >>>
> >>
> //===============================================================
> >> ======================
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> >> b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> >>> new file mode 100644
> >>> index 0000000000..b2770a9186
> >>> --- /dev/null
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> >>> @@ -0,0 +1,257 @@
> >>> +/** @file
> >>> +  EVP MD Wrapper Implementation for OpenSSL.
> >>> +
> >>> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> >>> +SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> +
> >>> +**/
> >>> +
> >>> +#include "InternalCryptLib.h"
> >>> +#include <openssl/evp.h>
> >>> +
> >>> +/**
> >>> +  Allocates and initializes one EVP_MD_CTX context for subsequent
> EVP_MD
> >> use.
> >>> +
> >>> +  If DigestName is NULL, then return FALSE.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> >> terminated ASCII string.
> >>> +                              Valid digest names are:
> >>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> >>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> >>> +                              SM3
> >>> +
> >>> +  @return  Pointer to the EVP_MD_CTX context that has been allocated and
> >> initialized.
> >>> +           If DigestName is invalid, returns NULL.
> >>> +           If the allocations fails, returns NULL.
> >>> +           If initialization fails, returns NULL.
> >>> +
> >>> +**/
> >>> +VOID *
> >>> +EFIAPI
> >>> +EvpMdInit (
> >>> +  IN  CONST CHAR8   *DigestName
> >>> +  )
> >>> +{
> >>> +  EVP_MD    *Digest;
> >>> +  VOID      *EvpMdContext;
> >>> +
> >>> +  //
> >>> +  // Check input parameters.
> >>> +  //
> >>> +  if (DigestName == NULL) {
> >>> +    return NULL;
> >>> +  }
> >>> +
> >>> +  //
> >>> +  // Allocate EVP_MD_CTX Context
> >>> +  //
> >>> +  EvpMdContext = EVP_MD_CTX_new ();
> >>> +  if (EvpMdContext == NULL) {
> >>> +    return NULL;
> >>> +  }
> >>> +
> >>> +  Digest = EVP_get_digestbyname (DigestName);
> >>
> >> I think this may not compile with gcc (and correctly so). The pointer
> >> returned by EVP_get_digestbyname() is const-qualified, but with the
> >> assignment, we're throwing away the const-ness.
> >>
> >> (4) Please const-qualify the "Digest" local pointer.
> >>
> >>> +  if (Digest == NULL) {
> >>> +    return NULL;
> >>> +  }
> >>
> >> (5) This is a memory leak I believe; "EvpMdContext" is leaked.
> >>
> >> For keeping the control flow simple, consider moving
> >> EVP_get_digestbyname() above EVP_MD_CTX_new().
> >>
> >>> +
> >>> +  //
> >>> +  // Initialize Context
> >>> +  //
> >>> +  if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
> >>> +    EVP_MD_CTX_free (EvpMdContext);
> >>> +    return NULL;
> >>> +  }
> >>> +
> >>> +  return EvpMdContext;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Makes a copy of an existing EVP_MD context.
> >>> +
> >>> +  If EvpMdContext is NULL, then return FALSE.
> >>> +  If NewEvpMdContext is NULL, then return FALSE.
> >>> +
> >>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> >>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> >>> +
> >>> +  @retval TRUE   EVP_MD context copy succeeded.
> >>> +  @retval FALSE  EVP_MD context copy failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdDuplicate (
> >>> +  IN  CONST VOID    *EvpMdContext,
> >>> +  OUT VOID          *NewEvpMdContext
> >>> +  )
> >>> +{
> >>> +  //
> >>> +  // Check input parameters.
> >>> +  //
> >>> +  if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  return TRUE;
> >>> +}
> >>
> >> (6) Can you please confirm that the caller is supposed to initialize
> >> "NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?
> >>
> >>> +
> >>> +/**
> >>> +  Digests the input data and updates EVP_MD context.
> >>> +
> >>> +  This function performs EVP digest on a data buffer of the specified size.
> >>> +  It can be called multiple times to compute the digest of long or
> >> discontinuous data streams.
> >>> +  EVP_MD context should be already correctly initialized by EvpMdInit(),
> and
> >> should not
> >>> +  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
> >>> +
> >>> +  If EvpMdContext is NULL, then return FALSE.
> >>> +  If Data is NULL and DataSize is not zero, return FALSE.
> >>> +
> >>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> >>> +  @param[in]       Data               Pointer to the buffer containing the data to
> be
> >> digested.
> >>> +  @param[in]       DataSize           Size of Data buffer in bytes.
> >>> +
> >>> +  @retval TRUE   EVP data digest succeeded.
> >>> +  @retval FALSE  EVP data digest failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdUpdate (
> >>> +  IN OUT  VOID        *EvpMdContext,
> >>> +  IN      CONST VOID  *Data,
> >>> +  IN      UINTN       DataSize
> >>> +  )
> >>> +{
> >>> +  //
> >>> +  // Check input parameters.
> >>> +  //
> >>> +  if (EvpMdContext == NULL) {
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  //
> >>> +  // Check invalid parameters, in case only DataLength was checked in
> >> OpenSSL
> >>> +  //
> >>> +  if (Data == NULL && DataSize != 0) {
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  //
> >>> +  // OpenSSL EVP digest update
> >>> +  //
> >>> +  if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  return TRUE;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Completes computation of the EVP digest value.
> >>> +  Releases the specified EVP_MD_CTX context.
> >>> +
> >>> +  This function completes EVP hash computation and retrieves the digest
> >> value into
> >>> +  the specified memory. After this function has been called, the EVP context
> >> cannot
> >>> +  be used again.
> >>> +  EVP context should be already correctly initialized by EvpMdInit(), and
> >> should
> >>> +  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
> >> undefined.
> >>> +
> >>> +  If EvpMdContext is NULL, then return FALSE.
> >>> +  If DigestValue is NULL, free the Context then return FALSE.
> >>> +
> >>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> >>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP
> digest
> >> value.
> >>> +
> >>> +  @retval TRUE   EVP digest computation succeeded.
> >>> +  @retval FALSE  EVP digest computation failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdFinal (
> >>> +  IN OUT  VOID   *EvpMdContext,
> >>> +  OUT     UINT8  *DigestValue
> >>> +  )
> >>> +{
> >>> +  UINT32    Length;
> >>> +  BOOLEAN   ReturnValue;
> >>> +
> >>> +  ReturnValue = TRUE;
> >>> +
> >>> +  //
> >>> +  // Check input parameters.
> >>> +  //
> >>> +  if (EvpMdContext == NULL) {
> >>> +    return FALSE;
> >>> +  }
> >>> +  if (DigestValue == NULL) {
> >>> +    EVP_MD_CTX_free (EvpMdContext);
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  //
> >>> +  // OpenSSL EVP digest finalization
> >>> +  //
> >>> +  if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
> >>> +    ReturnValue = FALSE;
> >>> +  }
> >>
> >>
> >> (7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
> >> deals fine with the third parameter being NULL, according to the docs
> >> (and the code).
> >>
> >>
> >>> +
> >>> +  //
> >>> +  // Free OpenSSL EVP_MD_CTX Context
> >>> +  //
> >>> +  EVP_MD_CTX_free (EvpMdContext);
> >>> +
> >>> +  return ReturnValue;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Computes the message digest of an input data buffer.
> >>> +
> >>> +  This function performs the message digest of a given data buffer, and
> places
> >>> +  the digest value into the specified memory.
> >>> +
> >>> +  If DigestName is NULL, return FALSE.
> >>> +  If Data is NULL and DataSize is not zero, return FALSE.
> >>> +  If HashValue is NULL, return FALSE.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name.
> >>> +  @param[in]    Data          Pointer to the buffer containing the data to be
> >> hashed.
> >>> +  @param[in]    DataSize      Size of Data buffer in bytes.
> >>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> >> value.
> >>> +
> >>> +  @retval TRUE   Digest computation succeeded.
> >>> +  @retval FALSE  Digest computation failed.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdHashAll (
> >>> +  IN  CONST CHAR8   *DigestName,
> >>> +  IN  CONST VOID    *Data,
> >>> +  IN  UINTN         DataSize,
> >>> +  OUT UINT8         *HashValue
> >>> +  )
> >>> +{
> >>> +  BOOLEAN   Result;
> >>> +  VOID      *EvpMdContext;
> >>> +
> >>> +  EvpMdContext = EvpMdInit (DigestName);
> >>> +  if (EvpMdContext == NULL) {
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
> >>> +  if (Result == FALSE) {
> >>
> >> (8) Style: please write (!Result).
> >>
> >>
> >>> +    EvpMdFinal (EvpMdContext, NULL);
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  Result = EvpMdFinal (EvpMdContext, HashValue);
> >>> +
> >>> +  return Result;
> >>> +}
> >>> diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> >> b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> >>> new file mode 100644
> >>> index 0000000000..038f63801f
> >>> --- /dev/null
> >>> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> >>> @@ -0,0 +1,128 @@
> >>> +/** @file
> >>> +  EVP MD Wrapper Null Library.
> >>> +
> >>> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> >>> +SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> +
> >>> +**/
> >>> +
> >>> +#include "InternalCryptLib.h"
> >>> +
> >>> +/**
> >>> +  Allocates and initializes one EVP_MD_CTX context for subsequent
> EVP_MD
> >> use.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> >> terminated ASCII string.
> >>> +                              Valid digest names are:
> >>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> >>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> >>> +                              SM3
> >>> +
> >>> +  @return NULL  This interface is not supported.
> >>> +
> >>> +**/
> >>> +VOID *
> >>> +EFIAPI
> >>> +EvpMdInit (
> >>> +  IN  CONST CHAR8   *DigestName
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return NULL;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Makes a copy of an existing EVP_MD context.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> >>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdDuplicate (
> >>> +  IN  CONST VOID    *EvpMdContext,
> >>> +  OUT VOID          *NewEvpMdContext
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Digests the input data and updates EVP_MD context.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> >>> +  @param[in]       Data               Pointer to the buffer containing the data to
> be
> >> digested.
> >>> +  @param[in]       DataSize           Size of Data buffer in bytes.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdUpdate (
> >>> +  IN OUT  VOID        *EvpMdContext,
> >>> +  IN      CONST VOID  *Data,
> >>> +  IN      UINTN       DataSize
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Completes computation of the EVP digest value.
> >>> +  Releases the specified EVP_MD_CTX context.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> >>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP
> digest
> >> value.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdFinal (
> >>> +  IN OUT  VOID   *EvpMdContext,
> >>> +  OUT     UINT8  *DigestValue
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Computes the message digest of an input data buffer.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name.
> >>> +  @param[in]    Data          Pointer to the buffer containing the data to be
> >> hashed.
> >>> +  @param[in]    DataSize      Size of Data buffer in bytes.
> >>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> >> value.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdHashAll (
> >>> +  IN  CONST CHAR8   *DigestName,
> >>> +  IN  CONST VOID    *Data,
> >>> +  IN  UINTN         DataSize,
> >>> +  OUT UINT8         *HashValue
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>> diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> >> b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> >>> new file mode 100644
> >>> index 0000000000..038f63801f
> >>> --- /dev/null
> >>> +++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> >>> @@ -0,0 +1,128 @@
> >>> +/** @file
> >>> +  EVP MD Wrapper Null Library.
> >>> +
> >>> +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> >>> +SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> +
> >>> +**/
> >>> +
> >>> +#include "InternalCryptLib.h"
> >>> +
> >>> +/**
> >>> +  Allocates and initializes one EVP_MD_CTX context for subsequent
> EVP_MD
> >> use.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name as a NULL-
> >> terminated ASCII string.
> >>> +                              Valid digest names are:
> >>> +                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> >>> +                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
> >>> +                              SM3
> >>> +
> >>> +  @return NULL  This interface is not supported.
> >>> +
> >>> +**/
> >>> +VOID *
> >>> +EFIAPI
> >>> +EvpMdInit (
> >>> +  IN  CONST CHAR8   *DigestName
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return NULL;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Makes a copy of an existing EVP_MD context.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
> >>> +  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdDuplicate (
> >>> +  IN  CONST VOID    *EvpMdContext,
> >>> +  OUT VOID          *NewEvpMdContext
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Digests the input data and updates EVP_MD context.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
> >>> +  @param[in]       Data               Pointer to the buffer containing the data to
> be
> >> digested.
> >>> +  @param[in]       DataSize           Size of Data buffer in bytes.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdUpdate (
> >>> +  IN OUT  VOID        *EvpMdContext,
> >>> +  IN      CONST VOID  *Data,
> >>> +  IN      UINTN       DataSize
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Completes computation of the EVP digest value.
> >>> +  Releases the specified EVP_MD_CTX context.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in, out]  EvpMdContext   Pointer to the EVP context.
> >>> +  @param[out]      Digest         Pointer to a buffer that receives the EVP
> digest
> >> value.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdFinal (
> >>> +  IN OUT  VOID   *EvpMdContext,
> >>> +  OUT     UINT8  *DigestValue
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>> +
> >>> +/**
> >>> +  Computes the message digest of an input data buffer.
> >>> +
> >>> +  Return FALSE to indicate this interface is not supported.
> >>> +
> >>> +  @param[in]    DigestName    Pointer to the digest name.
> >>> +  @param[in]    Data          Pointer to the buffer containing the data to be
> >> hashed.
> >>> +  @param[in]    DataSize      Size of Data buffer in bytes.
> >>> +  @param[out]   HashValue     Pointer to a buffer that receives the digest
> >> value.
> >>> +
> >>> +  @retval FALSE  This interface is not supported.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EFIAPI
> >>> +EvpMdHashAll (
> >>> +  IN  CONST CHAR8   *DigestName,
> >>> +  IN  CONST VOID    *Data,
> >>> +  IN  UINTN         DataSize,
> >>> +  OUT UINT8         *HashValue
> >>> +  )
> >>> +{
> >>> +  ASSERT (FALSE);
> >>> +  return FALSE;
> >>> +}
> >>>
> >>
> >> Thanks,
> >> Laszlo
> >


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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16 14:17         ` Yao, Jiewen
@ 2020-09-16 16:04           ` Laszlo Ersek
  2020-09-17  1:21             ` Zurcher, Christopher J
  0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2020-09-16 16:04 UTC (permalink / raw)
  To: Yao, Jiewen, devel@edk2.groups.io, Zurcher, Christopher J
  Cc: Wang, Jian J, Lu, XiaoyuX

On 09/16/20 16:17, Yao, Jiewen wrote:
> Thank you Laszlo.
> You forced me to read the code again and more carefully. :-)
> 
> I believe I understand why you use NULL to free the context now - to support error path.
> 
> First, I did have some new thought for EVP API. Let’s consider 3 cases, 1) Hash all data one time, 2) Hash update data multiple times, 3) Error during update.
> 
> A. In current design, the API sequence is:
> 1) EvpMdHashAll (Digest)
> 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> 3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)
> 
> B. We can auto free context in EvpMdUpdate in error path  - the API sequence is:
> 1) EvpMdHashAll (Digest)
> 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> 3) EvpMdInit, EvpMdUpdate->ERROR

I don't like "B" because in the loop body where you call EvpMdUpdate(),
you may encounter an error from a different source than EvpMdUpdate()
itself. For example, you may be reading data from the network or a disk
file. If fetching the next chunk fails, we'd still want to clean up the
EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
context, if it failed, then that would *complicate* the error path. (=
Clean up the context after the loop body *only* if something different
from EvpMdUpdate() failed.)

> 
> C: We can use New/Free style - similar to HMAC
> 1) EvpMdHashAll (Digest)
> 2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest), EvpMdFree
> 3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree

Yes, this was the first pattern, the one that caused me to say "please
don't do this". In this pattern, the context may exist between "New" and
"Init", and also between "Final" and "Free". Those two states are
useless and only good for causing confusion.

For example, are you allowed to Duplicate a context in those states?
It's best to prevent even the *asking* of that question.

> 
> I checked below APIs:
> 1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate, EVP_DigestFinal_ex, EVP_MD_CTX_free)
> 2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret, mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret, mbedtls_sha256_free)
> 3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey, HmacSha256Update, HmacSha256Final, HmacSha256Free)
> 
> All APIs include free function to free the context, I don’t think it is a bad idea to have EvpMdFree() API.
> I would like to propose option - C.

- I cannot comment on mbedtls (2).

- I think the current BaseCryptLib HMAC APIs (3) are not great, and we
should use this opportunity to improve upon them.

- Regarding openssl (1), as I understand it, the situation is different
from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
C language terms, it is not an "incomplete" type, but a "complete"
type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.

The consequence is that OpenSSL code itself can use the following style:

void func(void)
{
  EVP_MD_CTX ctx;

  EVP_DigestInit_ex(&ctx, ...);
  ...
  EVP_DigestFinal_ex(&ctx, ...)
}

In other words, OpenSSL-native code is permitted to know the internals
of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
EVP_MD_CTX_free().

The caller of (Init, Final) may use (New, Free) for memory management,
or may use something completely different (for example, local variables).

Therefore, offering the (Init, Final) APIs separately from (New, Free)
is meaningful.

But the situation in edk2 -- or any other OpenSSL *consumer* -- is
different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
an "incomplete" type. OpenSSL deliberately hides the internals of
EVP_MD_CTX.

See "CryptoPkg/Library/OpensslLib/openssl/include/openssl/ossl_typ.h":

> typedef struct evp_md_ctx_st EVP_MD_CTX;

and the "evp_md_ctx_st" structure type is only defined in
"CryptoPkg/Library/OpensslLib/openssl/crypto/evp/evp_local.h", whose
contents -- I think! -- OpenSSL client code cannot, or *should not*,
refer to.

This means that OpenSSL consumer code can *only* rely on
EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
defining a local or global variable of type EVP_MD_CTX is also not valid.

This means that the only reason for separating (Init, Final) from (New,
Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
there is no reason to keep *both* pairs of APIs.

Please note that, this (very prudent) information hiding / encapsulation
in OpenSSL used to be violated by edk2 in the past, intentionally.
That's what the HmacXxxGetContextSize() APIs were about -- those APIs
forcefully leaked the context sizes to client code, so that client code
in edk2 could perform its own allocation.

But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
eliminated HmacXxxGetContextSize(). As a part of that, we also
simplified the HMAC API -- note that Init was replaced by SetKey. And
because we were reworking an existent API, I didn't propose very
intrusive changes -- I didn't propose fusing Final and Free, for example.

Now, the EVP MD APIs are just being introduced to edk2. So we have a
chance at getting them right, and making them minimal.

> Second, I believe EVP is just something in openssl. It does not mean that we *have to* design API to follow openssl.
> After rethink the size impact, I do have concern to merge all Hash implementation together in one function.
> It might means nothing if the crypto library is based upon openssl.
> But if the cryptolib implementation is based upon other crypto, such as Intel IPP (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCommonPkg/Library/IppCryptoLib) or mbedTls (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then we can NOT get size benefit by separating the hash algorithm.
> 
> 
> I would like to propose separate EvpMdxxx.
> EvpMdNew -> Sha256New, Sha384New
> EvpMdInit -> Sha256Init, Sha384Init
> EvpMdUpdate -> Sha256Update, Sha384Update
> EvpMdFinal -> Sha256Final, Sha384Final
> EvpMdFree -> Sha256Free, Sha384Free

I have no comment on this.

> We can do similar thing with Hmac, to deprecate Sha256GetContextSize() API,

Yes, good idea.

> and replace caller with Sha256New() / Sha384Free()

Indeed.

But then -- there's no reason left for keeping (New, Free) separate from
(Init, Final).

Thanks
Laszlo

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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16 16:04           ` Laszlo Ersek
@ 2020-09-17  1:21             ` Zurcher, Christopher J
  2020-09-17  1:33               ` Yao, Jiewen
  0 siblings, 1 reply; 13+ messages in thread
From: Zurcher, Christopher J @ 2020-09-17  1:21 UTC (permalink / raw)
  To: Laszlo Ersek, Yao, Jiewen, devel@edk2.groups.io; +Cc: Wang, Jian J, Lu, XiaoyuX

Jiewen,

This patch set assumes that EDK2 is linked with OpenSSL for the foreseeable future. The end goal would be to move all applicable Crypto access to the EVP interface and remove the individual modules we maintain for each algorithm. The primary benefit here is reducing complexity and code duplication. Without this end goal, this patch set will not be particularly useful.

If the design goal of BaseCryptLib is to allow OpenSSL to be replaced by other crypto providers, I should abandon this patch set, and we can save the EVP transition for when moving to OpenSSL 3 becomes mandatory. At that time, the CryptX.c family can be modified to call EVP functions.
(This could even be done transparently, by returning UINTN from GetContextSize and using the user-supplied "context" to store the OpenSSL-supplied context pointer.)
Delaying EVP adoption in this case would also allow more time for platforms to adopt the Crypto Services model, which should help offset the size impact.

Please let me know which path should be taken.

Thanks,
Christopher Zurcher

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, September 16, 2020 09:05
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher, Christopher J
> <christopher.j.zurcher@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
> 
> On 09/16/20 16:17, Yao, Jiewen wrote:
> > Thank you Laszlo.
> > You forced me to read the code again and more carefully. :-)
> >
> > I believe I understand why you use NULL to free the context now - to support error path.
> >
> > First, I did have some new thought for EVP API. Let’s consider 3 cases, 1) Hash all data one time, 2) Hash update
> data multiple times, 3) Error during update.
> >
> > A. In current design, the API sequence is:
> > 1) EvpMdHashAll (Digest)
> > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > 3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)
> >
> > B. We can auto free context in EvpMdUpdate in error path  - the API sequence is:
> > 1) EvpMdHashAll (Digest)
> > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > 3) EvpMdInit, EvpMdUpdate->ERROR
> 
> I don't like "B" because in the loop body where you call EvpMdUpdate(),
> you may encounter an error from a different source than EvpMdUpdate()
> itself. For example, you may be reading data from the network or a disk
> file. If fetching the next chunk fails, we'd still want to clean up the
> EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
> context, if it failed, then that would *complicate* the error path. (=
> Clean up the context after the loop body *only* if something different
> from EvpMdUpdate() failed.)
> 
> >
> > C: We can use New/Free style - similar to HMAC
> > 1) EvpMdHashAll (Digest)
> > 2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest), EvpMdFree
> > 3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree
> 
> Yes, this was the first pattern, the one that caused me to say "please
> don't do this". In this pattern, the context may exist between "New" and
> "Init", and also between "Final" and "Free". Those two states are
> useless and only good for causing confusion.
> 
> For example, are you allowed to Duplicate a context in those states?
> It's best to prevent even the *asking* of that question.
> 
> >
> > I checked below APIs:
> > 1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate, EVP_DigestFinal_ex, EVP_MD_CTX_free)
> > 2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret, mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret,
> mbedtls_sha256_free)
> > 3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey, HmacSha256Update, HmacSha256Final, HmacSha256Free)
> >
> > All APIs include free function to free the context, I don’t think it is a bad idea to have EvpMdFree() API.
> > I would like to propose option - C.
> 
> - I cannot comment on mbedtls (2).
> 
> - I think the current BaseCryptLib HMAC APIs (3) are not great, and we
> should use this opportunity to improve upon them.
> 
> - Regarding openssl (1), as I understand it, the situation is different
> from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
> C language terms, it is not an "incomplete" type, but a "complete"
> type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.
> 
> The consequence is that OpenSSL code itself can use the following style:
> 
> void func(void)
> {
>   EVP_MD_CTX ctx;
> 
>   EVP_DigestInit_ex(&ctx, ...);
>   ...
>   EVP_DigestFinal_ex(&ctx, ...)
> }
> 
> In other words, OpenSSL-native code is permitted to know the internals
> of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
> EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
> EVP_MD_CTX_free().
> 
> The caller of (Init, Final) may use (New, Free) for memory management,
> or may use something completely different (for example, local variables).
> 
> Therefore, offering the (Init, Final) APIs separately from (New, Free)
> is meaningful.
> 
> But the situation in edk2 -- or any other OpenSSL *consumer* -- is
> different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
> an "incomplete" type. OpenSSL deliberately hides the internals of
> EVP_MD_CTX.
> 
> See "CryptoPkg/Library/OpensslLib/openssl/include/openssl/ossl_typ.h":
> 
> > typedef struct evp_md_ctx_st EVP_MD_CTX;
> 
> and the "evp_md_ctx_st" structure type is only defined in
> "CryptoPkg/Library/OpensslLib/openssl/crypto/evp/evp_local.h", whose
> contents -- I think! -- OpenSSL client code cannot, or *should not*,
> refer to.
> 
> This means that OpenSSL consumer code can *only* rely on
> EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
> context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
> defining a local or global variable of type EVP_MD_CTX is also not valid.
> 
> This means that the only reason for separating (Init, Final) from (New,
> Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
> there is no reason to keep *both* pairs of APIs.
> 
> 
> Please note that, this (very prudent) information hiding / encapsulation
> in OpenSSL used to be violated by edk2 in the past, intentionally.
> That's what the HmacXxxGetContextSize() APIs were about -- those APIs
> forcefully leaked the context sizes to client code, so that client code
> in edk2 could perform its own allocation.
> 
> But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
> eliminated HmacXxxGetContextSize(). As a part of that, we also
> simplified the HMAC API -- note that Init was replaced by SetKey. And
> because we were reworking an existent API, I didn't propose very
> intrusive changes -- I didn't propose fusing Final and Free, for example.
> 
> Now, the EVP MD APIs are just being introduced to edk2. So we have a
> chance at getting them right, and making them minimal.
> 
> > Second, I believe EVP is just something in openssl. It does not mean that we *have to* design API to follow
> openssl.
> > After rethink the size impact, I do have concern to merge all Hash implementation together in one function.
> > It might means nothing if the crypto library is based upon openssl.
> > But if the cryptolib implementation is based upon other crypto, such as Intel IPP
> (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCommonPkg/Library/IppCryptoLib) or mbedTls
> (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then we can NOT get size benefit by separating
> the hash algorithm.
> >
> >
> > I would like to propose separate EvpMdxxx.
> > EvpMdNew -> Sha256New, Sha384New
> > EvpMdInit -> Sha256Init, Sha384Init
> > EvpMdUpdate -> Sha256Update, Sha384Update
> > EvpMdFinal -> Sha256Final, Sha384Final
> > EvpMdFree -> Sha256Free, Sha384Free
> 
> I have no comment on this.
> 
> > We can do similar thing with Hmac, to deprecate Sha256GetContextSize() API,
> 
> Yes, good idea.
> 
> > and replace caller with Sha256New() / Sha384Free()
> 
> Indeed.
> 
> But then -- there's no reason left for keeping (New, Free) separate from
> (Init, Final).
> 
> Thanks
> Laszlo


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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-17  1:21             ` Zurcher, Christopher J
@ 2020-09-17  1:33               ` Yao, Jiewen
  2020-09-17  1:44                 ` Zurcher, Christopher J
  0 siblings, 1 reply; 13+ messages in thread
From: Yao, Jiewen @ 2020-09-17  1:33 UTC (permalink / raw)
  To: Zurcher, Christopher J, Laszlo Ersek, devel@edk2.groups.io
  Cc: Wang, Jian J, Lu, XiaoyuX

Thanks to ask this question Christopher.

When we create the CryptoPkg, our goal is to pick openssl as implementation choice. We don’t want to mandate any EDKII consumer use openssl.

The EDKII consumer shall have freedom to choose whatever crypto library they want to use. Even Intel IPP and mebdTLS are options. A vendor may choose a GPL wolfssl, or vendor proprietary close source implementation. We choose openssl just because it is the BSD license and it is used widely when we make decision.

That is why we spend effort to design BaseCryptoLib API to abstract the crypto action, instead of calling openssl function directly in our code.
Whenever we add new API, we need evaluate if it is applicable for any other crypto implantation and have negative impact.

That assumption that EDKII must link openssl is NOT our original intent.
Feel free to drop the patch or reject the Bugzilla, if you want.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> Sent: Thursday, September 17, 2020 9:21 AM
> To: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> 
> Jiewen,
> 
> This patch set assumes that EDK2 is linked with OpenSSL for the foreseeable
> future. The end goal would be to move all applicable Crypto access to the EVP
> interface and remove the individual modules we maintain for each algorithm.
> The primary benefit here is reducing complexity and code duplication. Without
> this end goal, this patch set will not be particularly useful.
> 
> If the design goal of BaseCryptLib is to allow OpenSSL to be replaced by other
> crypto providers, I should abandon this patch set, and we can save the EVP
> transition for when moving to OpenSSL 3 becomes mandatory. At that time, the
> CryptX.c family can be modified to call EVP functions.
> (This could even be done transparently, by returning UINTN from GetContextSize
> and using the user-supplied "context" to store the OpenSSL-supplied context
> pointer.)
> Delaying EVP adoption in this case would also allow more time for platforms to
> adopt the Crypto Services model, which should help offset the size impact.
> 
> Please let me know which path should be taken.
> 
> Thanks,
> Christopher Zurcher
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Wednesday, September 16, 2020 09:05
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher,
> Christopher J
> > <christopher.j.zurcher@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> >
> > On 09/16/20 16:17, Yao, Jiewen wrote:
> > > Thank you Laszlo.
> > > You forced me to read the code again and more carefully. :-)
> > >
> > > I believe I understand why you use NULL to free the context now - to support
> error path.
> > >
> > > First, I did have some new thought for EVP API. Let’s consider 3 cases, 1)
> Hash all data one time, 2) Hash update
> > data multiple times, 3) Error during update.
> > >
> > > A. In current design, the API sequence is:
> > > 1) EvpMdHashAll (Digest)
> > > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > > 3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)
> > >
> > > B. We can auto free context in EvpMdUpdate in error path  - the API
> sequence is:
> > > 1) EvpMdHashAll (Digest)
> > > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > > 3) EvpMdInit, EvpMdUpdate->ERROR
> >
> > I don't like "B" because in the loop body where you call EvpMdUpdate(),
> > you may encounter an error from a different source than EvpMdUpdate()
> > itself. For example, you may be reading data from the network or a disk
> > file. If fetching the next chunk fails, we'd still want to clean up the
> > EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
> > context, if it failed, then that would *complicate* the error path. (=
> > Clean up the context after the loop body *only* if something different
> > from EvpMdUpdate() failed.)
> >
> > >
> > > C: We can use New/Free style - similar to HMAC
> > > 1) EvpMdHashAll (Digest)
> > > 2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal
> (Digest), EvpMdFree
> > > 3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree
> >
> > Yes, this was the first pattern, the one that caused me to say "please
> > don't do this". In this pattern, the context may exist between "New" and
> > "Init", and also between "Final" and "Free". Those two states are
> > useless and only good for causing confusion.
> >
> > For example, are you allowed to Duplicate a context in those states?
> > It's best to prevent even the *asking* of that question.
> >
> > >
> > > I checked below APIs:
> > > 1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate,
> EVP_DigestFinal_ex, EVP_MD_CTX_free)
> > > 2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret,
> mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret,
> > mbedtls_sha256_free)
> > > 3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey,
> HmacSha256Update, HmacSha256Final, HmacSha256Free)
> > >
> > > All APIs include free function to free the context, I don’t think it is a bad idea
> to have EvpMdFree() API.
> > > I would like to propose option - C.
> >
> > - I cannot comment on mbedtls (2).
> >
> > - I think the current BaseCryptLib HMAC APIs (3) are not great, and we
> > should use this opportunity to improve upon them.
> >
> > - Regarding openssl (1), as I understand it, the situation is different
> > from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
> > C language terms, it is not an "incomplete" type, but a "complete"
> > type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.
> >
> > The consequence is that OpenSSL code itself can use the following style:
> >
> > void func(void)
> > {
> >   EVP_MD_CTX ctx;
> >
> >   EVP_DigestInit_ex(&ctx, ...);
> >   ...
> >   EVP_DigestFinal_ex(&ctx, ...)
> > }
> >
> > In other words, OpenSSL-native code is permitted to know the internals
> > of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
> > EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
> > EVP_MD_CTX_free().
> >
> > The caller of (Init, Final) may use (New, Free) for memory management,
> > or may use something completely different (for example, local variables).
> >
> > Therefore, offering the (Init, Final) APIs separately from (New, Free)
> > is meaningful.
> >
> > But the situation in edk2 -- or any other OpenSSL *consumer* -- is
> > different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
> > an "incomplete" type. OpenSSL deliberately hides the internals of
> > EVP_MD_CTX.
> >
> > See "CryptoPkg/Library/OpensslLib/openssl/include/openssl/ossl_typ.h":
> >
> > > typedef struct evp_md_ctx_st EVP_MD_CTX;
> >
> > and the "evp_md_ctx_st" structure type is only defined in
> > "CryptoPkg/Library/OpensslLib/openssl/crypto/evp/evp_local.h", whose
> > contents -- I think! -- OpenSSL client code cannot, or *should not*,
> > refer to.
> >
> > This means that OpenSSL consumer code can *only* rely on
> > EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
> > context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
> > defining a local or global variable of type EVP_MD_CTX is also not valid.
> >
> > This means that the only reason for separating (Init, Final) from (New,
> > Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
> > there is no reason to keep *both* pairs of APIs.
> >
> >
> > Please note that, this (very prudent) information hiding / encapsulation
> > in OpenSSL used to be violated by edk2 in the past, intentionally.
> > That's what the HmacXxxGetContextSize() APIs were about -- those APIs
> > forcefully leaked the context sizes to client code, so that client code
> > in edk2 could perform its own allocation.
> >
> > But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
> > eliminated HmacXxxGetContextSize(). As a part of that, we also
> > simplified the HMAC API -- note that Init was replaced by SetKey. And
> > because we were reworking an existent API, I didn't propose very
> > intrusive changes -- I didn't propose fusing Final and Free, for example.
> >
> > Now, the EVP MD APIs are just being introduced to edk2. So we have a
> > chance at getting them right, and making them minimal.
> >
> > > Second, I believe EVP is just something in openssl. It does not mean that we
> *have to* design API to follow
> > openssl.
> > > After rethink the size impact, I do have concern to merge all Hash
> implementation together in one function.
> > > It might means nothing if the crypto library is based upon openssl.
> > > But if the cryptolib implementation is based upon other crypto, such as Intel
> IPP
> >
> (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCo
> mmonPkg/Library/IppCryptoLib) or mbedTls
> > (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then
> we can NOT get size benefit by separating
> > the hash algorithm.
> > >
> > >
> > > I would like to propose separate EvpMdxxx.
> > > EvpMdNew -> Sha256New, Sha384New
> > > EvpMdInit -> Sha256Init, Sha384Init
> > > EvpMdUpdate -> Sha256Update, Sha384Update
> > > EvpMdFinal -> Sha256Final, Sha384Final
> > > EvpMdFree -> Sha256Free, Sha384Free
> >
> > I have no comment on this.
> >
> > > We can do similar thing with Hmac, to deprecate Sha256GetContextSize()
> API,
> >
> > Yes, good idea.
> >
> > > and replace caller with Sha256New() / Sha384Free()
> >
> > Indeed.
> >
> > But then -- there's no reason left for keeping (New, Free) separate from
> > (Init, Final).
> >
> > Thanks
> > Laszlo


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

* Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-17  1:33               ` Yao, Jiewen
@ 2020-09-17  1:44                 ` Zurcher, Christopher J
  0 siblings, 0 replies; 13+ messages in thread
From: Zurcher, Christopher J @ 2020-09-17  1:44 UTC (permalink / raw)
  To: Yao, Jiewen, Laszlo Ersek, devel@edk2.groups.io; +Cc: Wang, Jian J, Lu, XiaoyuX

OK, abandoning this patch set.

The BZ can stay open as we will still have to migrate the existing files to EVP eventually.

--
Christopher Zurcher

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Wednesday, September 16, 2020 18:34
> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
> 
> Thanks to ask this question Christopher.
> 
> When we create the CryptoPkg, our goal is to pick openssl as implementation choice. We don’t want to mandate any
> EDKII consumer use openssl.
> 
> The EDKII consumer shall have freedom to choose whatever crypto library they want to use. Even Intel IPP and mebdTLS
> are options. A vendor may choose a GPL wolfssl, or vendor proprietary close source implementation. We choose openssl
> just because it is the BSD license and it is used widely when we make decision.
> 
> That is why we spend effort to design BaseCryptoLib API to abstract the crypto action, instead of calling openssl
> function directly in our code.
> Whenever we add new API, we need evaluate if it is applicable for any other crypto implantation and have negative
> impact.
> 
> That assumption that EDKII must link openssl is NOT our original intent.
> Feel free to drop the patch or reject the Bugzilla, if you want.
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> > Sent: Thursday, September 17, 2020 9:21 AM
> > To: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> > devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> > (Envelope) Digest interface
> >
> > Jiewen,
> >
> > This patch set assumes that EDK2 is linked with OpenSSL for the foreseeable
> > future. The end goal would be to move all applicable Crypto access to the EVP
> > interface and remove the individual modules we maintain for each algorithm.
> > The primary benefit here is reducing complexity and code duplication. Without
> > this end goal, this patch set will not be particularly useful.
> >
> > If the design goal of BaseCryptLib is to allow OpenSSL to be replaced by other
> > crypto providers, I should abandon this patch set, and we can save the EVP
> > transition for when moving to OpenSSL 3 becomes mandatory. At that time, the
> > CryptX.c family can be modified to call EVP functions.
> > (This could even be done transparently, by returning UINTN from GetContextSize
> > and using the user-supplied "context" to store the OpenSSL-supplied context
> > pointer.)
> > Delaying EVP adoption in this case would also allow more time for platforms to
> > adopt the Crypto Services model, which should help offset the size impact.
> >
> > Please let me know which path should be taken.
> >
> > Thanks,
> > Christopher Zurcher
> >
> > > -----Original Message-----
> > > From: Laszlo Ersek <lersek@redhat.com>
> > > Sent: Wednesday, September 16, 2020 09:05
> > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher,
> > Christopher J
> > > <christopher.j.zurcher@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
> > (Envelope) Digest interface
> > >
> > > On 09/16/20 16:17, Yao, Jiewen wrote:
> > > > Thank you Laszlo.
> > > > You forced me to read the code again and more carefully. :-)
> > > >
> > > > I believe I understand why you use NULL to free the context now - to support
> > error path.
> > > >
> > > > First, I did have some new thought for EVP API. Let’s consider 3 cases, 1)
> > Hash all data one time, 2) Hash update
> > > data multiple times, 3) Error during update.
> > > >
> > > > A. In current design, the API sequence is:
> > > > 1) EvpMdHashAll (Digest)
> > > > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > > > 3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)
> > > >
> > > > B. We can auto free context in EvpMdUpdate in error path  - the API
> > sequence is:
> > > > 1) EvpMdHashAll (Digest)
> > > > 2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
> > > > 3) EvpMdInit, EvpMdUpdate->ERROR
> > >
> > > I don't like "B" because in the loop body where you call EvpMdUpdate(),
> > > you may encounter an error from a different source than EvpMdUpdate()
> > > itself. For example, you may be reading data from the network or a disk
> > > file. If fetching the next chunk fails, we'd still want to clean up the
> > > EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
> > > context, if it failed, then that would *complicate* the error path. (=
> > > Clean up the context after the loop body *only* if something different
> > > from EvpMdUpdate() failed.)
> > >
> > > >
> > > > C: We can use New/Free style - similar to HMAC
> > > > 1) EvpMdHashAll (Digest)
> > > > 2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal
> > (Digest), EvpMdFree
> > > > 3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree
> > >
> > > Yes, this was the first pattern, the one that caused me to say "please
> > > don't do this". In this pattern, the context may exist between "New" and
> > > "Init", and also between "Final" and "Free". Those two states are
> > > useless and only good for causing confusion.
> > >
> > > For example, are you allowed to Duplicate a context in those states?
> > > It's best to prevent even the *asking* of that question.
> > >
> > > >
> > > > I checked below APIs:
> > > > 1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate,
> > EVP_DigestFinal_ex, EVP_MD_CTX_free)
> > > > 2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret,
> > mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret,
> > > mbedtls_sha256_free)
> > > > 3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey,
> > HmacSha256Update, HmacSha256Final, HmacSha256Free)
> > > >
> > > > All APIs include free function to free the context, I don’t think it is a bad idea
> > to have EvpMdFree() API.
> > > > I would like to propose option - C.
> > >
> > > - I cannot comment on mbedtls (2).
> > >
> > > - I think the current BaseCryptLib HMAC APIs (3) are not great, and we
> > > should use this opportunity to improve upon them.
> > >
> > > - Regarding openssl (1), as I understand it, the situation is different
> > > from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
> > > C language terms, it is not an "incomplete" type, but a "complete"
> > > type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.
> > >
> > > The consequence is that OpenSSL code itself can use the following style:
> > >
> > > void func(void)
> > > {
> > >   EVP_MD_CTX ctx;
> > >
> > >   EVP_DigestInit_ex(&ctx, ...);
> > >   ...
> > >   EVP_DigestFinal_ex(&ctx, ...)
> > > }
> > >
> > > In other words, OpenSSL-native code is permitted to know the internals
> > > of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
> > > EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
> > > EVP_MD_CTX_free().
> > >
> > > The caller of (Init, Final) may use (New, Free) for memory management,
> > > or may use something completely different (for example, local variables).
> > >
> > > Therefore, offering the (Init, Final) APIs separately from (New, Free)
> > > is meaningful.
> > >
> > > But the situation in edk2 -- or any other OpenSSL *consumer* -- is
> > > different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
> > > an "incomplete" type. OpenSSL deliberately hides the internals of
> > > EVP_MD_CTX.
> > >
> > > See "CryptoPkg/Library/OpensslLib/openssl/include/openssl/ossl_typ.h":
> > >
> > > > typedef struct evp_md_ctx_st EVP_MD_CTX;
> > >
> > > and the "evp_md_ctx_st" structure type is only defined in
> > > "CryptoPkg/Library/OpensslLib/openssl/crypto/evp/evp_local.h", whose
> > > contents -- I think! -- OpenSSL client code cannot, or *should not*,
> > > refer to.
> > >
> > > This means that OpenSSL consumer code can *only* rely on
> > > EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
> > > context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
> > > defining a local or global variable of type EVP_MD_CTX is also not valid.
> > >
> > > This means that the only reason for separating (Init, Final) from (New,
> > > Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
> > > there is no reason to keep *both* pairs of APIs.
> > >
> > >
> > > Please note that, this (very prudent) information hiding / encapsulation
> > > in OpenSSL used to be violated by edk2 in the past, intentionally.
> > > That's what the HmacXxxGetContextSize() APIs were about -- those APIs
> > > forcefully leaked the context sizes to client code, so that client code
> > > in edk2 could perform its own allocation.
> > >
> > > But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
> > > eliminated HmacXxxGetContextSize(). As a part of that, we also
> > > simplified the HMAC API -- note that Init was replaced by SetKey. And
> > > because we were reworking an existent API, I didn't propose very
> > > intrusive changes -- I didn't propose fusing Final and Free, for example.
> > >
> > > Now, the EVP MD APIs are just being introduced to edk2. So we have a
> > > chance at getting them right, and making them minimal.
> > >
> > > > Second, I believe EVP is just something in openssl. It does not mean that we
> > *have to* design API to follow
> > > openssl.
> > > > After rethink the size impact, I do have concern to merge all Hash
> > implementation together in one function.
> > > > It might means nothing if the crypto library is based upon openssl.
> > > > But if the cryptolib implementation is based upon other crypto, such as Intel
> > IPP
> > >
> > (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCo
> > mmonPkg/Library/IppCryptoLib) or mbedTls
> > > (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then
> > we can NOT get size benefit by separating
> > > the hash algorithm.
> > > >
> > > >
> > > > I would like to propose separate EvpMdxxx.
> > > > EvpMdNew -> Sha256New, Sha384New
> > > > EvpMdInit -> Sha256Init, Sha384Init
> > > > EvpMdUpdate -> Sha256Update, Sha384Update
> > > > EvpMdFinal -> Sha256Final, Sha384Final
> > > > EvpMdFree -> Sha256Free, Sha384Free
> > >
> > > I have no comment on this.
> > >
> > > > We can do similar thing with Hmac, to deprecate Sha256GetContextSize()
> > API,
> > >
> > > Yes, good idea.
> > >
> > > > and replace caller with Sha256New() / Sha384Free()
> > >
> > > Indeed.
> > >
> > > But then -- there's no reason left for keeping (New, Free) separate from
> > > (Init, Final).
> > >
> > > Thanks
> > > Laszlo


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

* [PATCH v3 2/3] CryptoPkg: Add EVP to Crypto Service driver interface
  2020-09-16  0:58 [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J
  2020-09-16  0:59 ` [PATCH v3 1/3] " Zurcher, Christopher J
@ 2020-09-16  0:59 ` Zurcher, Christopher J
  2020-09-16  0:59 ` [PATCH v3 3/3] SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP interface Zurcher, Christopher J
  2020-09-16  3:00 ` [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Yao, Jiewen
  3 siblings, 0 replies; 13+ messages in thread
From: Zurcher, Christopher J @ 2020-09-16  0:59 UTC (permalink / raw)
  To: devel; +Cc: Laszlo Ersek, Jiewen Yao, Jian J Wang, Xiaoyu Lu

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
 CryptoPkg/CryptoPkg.dsc                                |   3 +
 CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h   |  10 ++
 CryptoPkg/Private/Protocol/Crypto.h                    | 131 +++++++++++++++++
 CryptoPkg/Driver/Crypto.c                              | 152 +++++++++++++++++++-
 CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 144 +++++++++++++++++++
 5 files changed, 439 insertions(+), 1 deletion(-)

diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
index 1af78468a1..af3fceb99f 100644
--- a/CryptoPkg/CryptoPkg.dsc
+++ b/CryptoPkg/CryptoPkg.dsc
@@ -159,6 +159,7 @@
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Tls.Family                               | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.TlsSet.Family                            | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.TlsGet.Family                            | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.EvpMd.Family                             | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
 !endif
 
 !if $(CRYPTO_SERVICES) == MIN_PEI
@@ -173,6 +174,7 @@
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Rsa.Services.Free               | TRUE
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Rsa.Services.SetKey             | TRUE
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Pkcs.Services.Pkcs5HashPassword | TRUE
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.EvpMd.Family                    | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
 !endif
 
 !if $(CRYPTO_SERVICES) == MIN_DXE_MIN_SMM
@@ -203,6 +205,7 @@
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Aes.Services.Init                        | TRUE
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Aes.Services.CbcEncrypt                  | TRUE
   gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Aes.Services.CbcDecrypt                  | TRUE
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.EvpMd.Family                             | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
 !endif
 
 ###################################################################################################
diff --git a/CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h b/CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h
index 44fb0262f4..b79c98d679 100644
--- a/CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h
+++ b/CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h
@@ -288,6 +288,16 @@ typedef struct {
     } Services;
     UINT32    Family;
   } TlsGet;
+  union {
+    struct {
+      UINT8  Init:1;
+      UINT8  Duplicate:1;
+      UINT8  Update:1;
+      UINT8  Final:1;
+      UINT8  HashAll:1;
+    } Services;
+    UINT32    Family;
+  } EvpMd;
 } PCD_CRYPTO_SERVICE_FAMILY_ENABLE;
 
 #endif
diff --git a/CryptoPkg/Private/Protocol/Crypto.h b/CryptoPkg/Private/Protocol/Crypto.h
index c399e0d67a..a3dffc0ce0 100644
--- a/CryptoPkg/Private/Protocol/Crypto.h
+++ b/CryptoPkg/Private/Protocol/Crypto.h
@@ -3434,6 +3434,131 @@ EFI_STATUS
   IN OUT UINTN                    *DataSize
   );
 
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  If DigestName is NULL, then return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
+                              Valid digest names are:
+                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
+                              SM3
+
+  @return  Pointer to the EVP_MD_CTX context that has been allocated and initialized.
+           If DigestName is invalid, returns NULL.
+           If the allocations fails, returns NULL.
+           If initialization fails, returns NULL.
+
+**/
+typedef
+VOID *
+(EFIAPI* EDKII_CRYPTO_EVPMD_INIT)(
+  IN  CONST CHAR8   *DigestName
+  );
+
+/**
+  Makes a copy of an existing EVP_MD context.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If NewEvpMdContext is NULL, then return FALSE.
+
+  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
+  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
+
+  @retval TRUE   EVP_MD context copy succeeded.
+  @retval FALSE  EVP_MD context copy failed.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI* EDKII_CRYPTO_EVPMD_DUPLICATE)(
+  IN  CONST VOID    *EvpMdContext,
+  OUT VOID          *NewEvpMdContext
+  );
+
+/**
+  Digests the input data and updates EVP_MD context.
+
+  This function performs EVP digest on a data buffer of the specified size.
+  It can be called multiple times to compute the digest of long or discontinuous data streams.
+  EVP_MD context should be already correctly initialized by EvpMdInit(), and should not
+  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+
+  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
+  @param[in]       Data               Pointer to the buffer containing the data to be digested.
+  @param[in]       DataSize           Size of Data buffer in bytes.
+
+  @retval TRUE   EVP data digest succeeded.
+  @retval FALSE  EVP data digest failed.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI* EDKII_CRYPTO_EVPMD_UPDATE)(
+  IN OUT  VOID        *EvpMdContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  );
+
+/**
+  Completes computation of the EVP digest value.
+  Releases the specified EVP_MD_CTX context.
+
+  This function completes EVP hash computation and retrieves the digest value into
+  the specified memory. After this function has been called, the EVP context cannot
+  be used again.
+  EVP context should be already correctly initialized by EvpMdInit(), and should
+  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If DigestValue is NULL, free the Context then return FALSE.
+
+  @param[in, out]  EvpMdContext   Pointer to the EVP context.
+  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
+
+  @retval TRUE   EVP digest computation succeeded.
+  @retval FALSE  EVP digest computation failed.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI* EDKII_CRYPTO_EVPMD_FINAL)(
+  IN OUT  VOID   *EvpMdContext,
+  OUT     UINT8  *DigestValue
+  );
+
+/**
+  Computes the message digest of an input data buffer.
+
+  This function performs the message digest of a given data buffer, and places
+  the digest value into the specified memory.
+
+  If DigestName is NULL, return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+  If HashValue is NULL, return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name.
+  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
+  @param[in]    DataSize      Size of Data buffer in bytes.
+  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
+
+  @retval TRUE   Digest computation succeeded.
+  @retval FALSE  Digest computation failed.
+
+**/
+typedef
+BOOLEAN
+(EFIAPI* EDKII_CRYPTO_EVPMD_HASH_ALL)(
+  IN  CONST CHAR8   *DigestName,
+  IN  CONST VOID    *Data,
+  IN  UINTN         DataSize,
+  OUT UINT8         *HashValue
+  );
+
 
 ///
 /// EDK II Crypto Protocol
@@ -3619,6 +3744,12 @@ struct _EDKII_CRYPTO_PROTOCOL {
   EDKII_CRYPTO_TLS_GET_HOST_PUBLIC_CERT           TlsGetHostPublicCert;
   EDKII_CRYPTO_TLS_GET_HOST_PRIVATE_KEY           TlsGetHostPrivateKey;
   EDKII_CRYPTO_TLS_GET_CERT_REVOCATION_LIST       TlsGetCertRevocationList;
+  /// Digest Envelope (EVP MD)
+  EDKII_CRYPTO_EVPMD_INIT                         EvpMdInit;
+  EDKII_CRYPTO_EVPMD_DUPLICATE                    EvpMdDuplicate;
+  EDKII_CRYPTO_EVPMD_UPDATE                       EvpMdUpdate;
+  EDKII_CRYPTO_EVPMD_FINAL                        EvpMdFinal;
+  EDKII_CRYPTO_EVPMD_HASH_ALL                     EvpMdHashAll;
 };
 
 extern GUID gEdkiiCryptoProtocolGuid;
diff --git a/CryptoPkg/Driver/Crypto.c b/CryptoPkg/Driver/Crypto.c
index d9096ea603..c50ac4a6da 100644
--- a/CryptoPkg/Driver/Crypto.c
+++ b/CryptoPkg/Driver/Crypto.c
@@ -4463,6 +4463,150 @@ CryptoServiceTlsGetCertRevocationList (
   return CALL_BASECRYPTLIB (TlsGet.Services.CertRevocationList, TlsGetCertRevocationList, (Data, DataSize), EFI_UNSUPPORTED);
 }
 
+//=====================================================================================
+//    EVP (Envelope) Primitive
+//=====================================================================================
+
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  If DigestName is NULL, then return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
+                              Valid digest names are:
+                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
+                              SM3
+
+  @return  Pointer to the EVP_MD_CTX context that has been allocated and initialized.
+           If DigestName is invalid, returns NULL.
+           If the allocations fails, returns NULL.
+           If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+CryptoServiceEvpMdInit (
+  IN  CONST CHAR8   *DigestName
+  )
+{
+  return CALL_BASECRYPTLIB (EvpMd.Services.Init, EvpMdInit, (DigestName), NULL);
+}
+
+/**
+  Makes a copy of an existing EVP_MD context.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If NewEvpMdContext is NULL, then return FALSE.
+
+  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
+  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
+
+  @retval TRUE   EVP_MD context copy succeeded.
+  @retval FALSE  EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+CryptoServiceEvpMdDuplicate (
+  IN  CONST VOID    *EvpMdContext,
+  OUT VOID          *NewEvpMdContext
+  )
+{
+  return CALL_BASECRYPTLIB (EvpMd.Services.Duplicate, EvpMdDuplicate, (EvpMdContext, NewEvpMdContext), FALSE);
+}
+
+/**
+  Digests the input data and updates EVP_MD context.
+
+  This function performs EVP digest on a data buffer of the specified size.
+  It can be called multiple times to compute the digest of long or discontinuous data streams.
+  EVP_MD context should be already correctly initialized by EvpMdInit(), and should not
+  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+
+  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
+  @param[in]       Data               Pointer to the buffer containing the data to be digested.
+  @param[in]       DataSize           Size of Data buffer in bytes.
+
+  @retval TRUE   EVP data digest succeeded.
+  @retval FALSE  EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+CryptoServiceEvpMdUpdate (
+  IN OUT  VOID        *EvpMdContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  )
+{
+  return CALL_BASECRYPTLIB (EvpMd.Services.Update, EvpMdUpdate, (EvpMdContext, Data, DataSize), FALSE);
+}
+
+/**
+  Completes computation of the EVP digest value.
+  Releases the specified EVP_MD_CTX context.
+
+  This function completes EVP hash computation and retrieves the digest value into
+  the specified memory. After this function has been called, the EVP context cannot
+  be used again.
+  EVP context should be already correctly initialized by EvpMdInit(), and should
+  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If DigestValue is NULL, free the Context then return FALSE.
+
+  @param[in, out]  EvpMdContext   Pointer to the EVP context.
+  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
+
+  @retval TRUE   EVP digest computation succeeded.
+  @retval FALSE  EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+CryptoServiceEvpMdFinal (
+  IN OUT  VOID   *EvpMdContext,
+  OUT     UINT8  *DigestValue
+  )
+{
+  return CALL_BASECRYPTLIB (EvpMd.Services.Final, EvpMdFinal, (EvpMdContext, DigestValue), FALSE);
+}
+
+/**
+  Computes the message digest of an input data buffer.
+
+  This function performs the message digest of a given data buffer, and places
+  the digest value into the specified memory.
+
+  If DigestName is NULL, return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+  If HashValue is NULL, return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name.
+  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
+  @param[in]    DataSize      Size of Data buffer in bytes.
+  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
+
+  @retval TRUE   Digest computation succeeded.
+  @retval FALSE  Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+CryptoServiceEvpMdHashAll (
+  IN  CONST CHAR8   *DigestName,
+  IN  CONST VOID    *Data,
+  IN  UINTN         DataSize,
+  OUT UINT8         *HashValue
+  )
+{
+  return CALL_BASECRYPTLIB (EvpMd.Services.HashAll, EvpMdHashAll, (DigestName, Data, DataSize, HashValue), FALSE);
+}
+
 const EDKII_CRYPTO_PROTOCOL mEdkiiCrypto = {
   /// Version
   CryptoServiceGetCryptoVersion,
@@ -4663,5 +4807,11 @@ const EDKII_CRYPTO_PROTOCOL mEdkiiCrypto = {
   CryptoServiceTlsGetCaCertificate,
   CryptoServiceTlsGetHostPublicCert,
   CryptoServiceTlsGetHostPrivateKey,
-  CryptoServiceTlsGetCertRevocationList
+  CryptoServiceTlsGetCertRevocationList,
+  /// Digest Envelope (EVP MD)
+  CryptoServiceEvpMdInit,
+  CryptoServiceEvpMdDuplicate,
+  CryptoServiceEvpMdUpdate,
+  CryptoServiceEvpMdFinal,
+  CryptoServiceEvpMdHashAll
 };
diff --git a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
index 3f14c6d262..0a68d0682e 100644
--- a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
+++ b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
@@ -3499,3 +3499,147 @@ TlsGetCertRevocationList (
 {
   CALL_CRYPTO_SERVICE (TlsGetCertRevocationList, (Data, DataSize), EFI_UNSUPPORTED);
 }
+
+//=====================================================================================
+//    EVP (Envelope) Primitive
+//=====================================================================================
+
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  If DigestName is NULL, then return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name as a NULL-terminated ASCII string.
+                              Valid digest names are:
+                              MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+                              SHA3-224, SHA3-256, SHA3-384, SHA3-512
+                              SM3
+
+  @return  Pointer to the EVP_MD_CTX context that has been allocated and initialized.
+           If DigestName is invalid, returns NULL.
+           If the allocations fails, returns NULL.
+           If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+  IN  CONST CHAR8   *DigestName
+  )
+{
+  CALL_CRYPTO_SERVICE (EvpMdInit, (DigestName), NULL);
+}
+
+/**
+  Makes a copy of an existing EVP_MD context.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If NewEvpMdContext is NULL, then return FALSE.
+
+  @param[in]  EvpMdContext     Pointer to EVP_MD context being copied.
+  @param[out] NewEvpMdContext  Pointer to new EVP_MD context.
+
+  @retval TRUE   EVP_MD context copy succeeded.
+  @retval FALSE  EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+  IN  CONST VOID    *EvpMdContext,
+  OUT VOID          *NewEvpMdContext
+  )
+{
+  CALL_CRYPTO_SERVICE (EvpMdDuplicate, (EvpMdContext, NewEvpMdContext), FALSE);
+}
+
+/**
+  Digests the input data and updates EVP_MD context.
+
+  This function performs EVP digest on a data buffer of the specified size.
+  It can be called multiple times to compute the digest of long or discontinuous data streams.
+  EVP_MD context should be already correctly initialized by EvpMdInit(), and should not
+  be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+
+  @param[in, out]  EvpMdContext       Pointer to the EVP_MD context.
+  @param[in]       Data               Pointer to the buffer containing the data to be digested.
+  @param[in]       DataSize           Size of Data buffer in bytes.
+
+  @retval TRUE   EVP data digest succeeded.
+  @retval FALSE  EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+  IN OUT  VOID        *EvpMdContext,
+  IN      CONST VOID  *Data,
+  IN      UINTN       DataSize
+  )
+{
+  CALL_CRYPTO_SERVICE (EvpMdUpdate, (EvpMdContext, Data, DataSize), FALSE);
+}
+
+/**
+  Completes computation of the EVP digest value.
+  Releases the specified EVP_MD_CTX context.
+
+  This function completes EVP hash computation and retrieves the digest value into
+  the specified memory. After this function has been called, the EVP context cannot
+  be used again.
+  EVP context should be already correctly initialized by EvpMdInit(), and should
+  not be finalized by EvpMdFinal(). Behavior with invalid EVP context is undefined.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If DigestValue is NULL, free the Context then return FALSE.
+
+  @param[in, out]  EvpMdContext   Pointer to the EVP context.
+  @param[out]      Digest         Pointer to a buffer that receives the EVP digest value.
+
+  @retval TRUE   EVP digest computation succeeded.
+  @retval FALSE  EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+  IN OUT  VOID   *EvpMdContext,
+  OUT     UINT8  *DigestValue
+  )
+{
+  CALL_CRYPTO_SERVICE (EvpMdFinal, (EvpMdContext, DigestValue), FALSE);
+}
+
+/**
+  Computes the message digest of an input data buffer.
+
+  This function performs the message digest of a given data buffer, and places
+  the digest value into the specified memory.
+
+  If DigestName is NULL, return FALSE.
+  If Data is NULL and DataSize is not zero, return FALSE.
+  If HashValue is NULL, return FALSE.
+
+  @param[in]    DigestName    Pointer to the digest name.
+  @param[in]    Data          Pointer to the buffer containing the data to be hashed.
+  @param[in]    DataSize      Size of Data buffer in bytes.
+  @param[out]   HashValue     Pointer to a buffer that receives the digest value.
+
+  @retval TRUE   Digest computation succeeded.
+  @retval FALSE  Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+  IN  CONST CHAR8   *DigestName,
+  IN  CONST VOID    *Data,
+  IN  UINTN         DataSize,
+  OUT UINT8         *HashValue
+  )
+{
+  CALL_CRYPTO_SERVICE (EvpMdHashAll, (DigestName, Data, DataSize, HashValue), FALSE);
+}
-- 
2.28.0.windows.1


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

* [PATCH v3 3/3] SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP interface
  2020-09-16  0:58 [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J
  2020-09-16  0:59 ` [PATCH v3 1/3] " Zurcher, Christopher J
  2020-09-16  0:59 ` [PATCH v3 2/3] CryptoPkg: Add EVP to Crypto Service driver interface Zurcher, Christopher J
@ 2020-09-16  0:59 ` Zurcher, Christopher J
  2020-09-16  3:00 ` [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Yao, Jiewen
  3 siblings, 0 replies; 13+ messages in thread
From: Zurcher, Christopher J @ 2020-09-16  0:59 UTC (permalink / raw)
  To: devel; +Cc: Laszlo Ersek, Jiewen Yao, Jian J Wang

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
 SecurityPkg/Hash2DxeCrypto/Driver.h         |   1 -
 SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c | 345 ++------------------
 2 files changed, 31 insertions(+), 315 deletions(-)

diff --git a/SecurityPkg/Hash2DxeCrypto/Driver.h b/SecurityPkg/Hash2DxeCrypto/Driver.h
index 7b8996912a..ac811b3977 100644
--- a/SecurityPkg/Hash2DxeCrypto/Driver.h
+++ b/SecurityPkg/Hash2DxeCrypto/Driver.h
@@ -50,7 +50,6 @@ typedef struct {
   LIST_ENTRY                       InstEntry;
   EFI_HASH2_PROTOCOL               Hash2Protocol;
   VOID                             *HashContext;
-  VOID                             *HashInfoContext;
   BOOLEAN                          Updated;
 } HASH2_INSTANCE_DATA;
 
diff --git a/SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c b/SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c
index d96bc136e2..f31bc79f04 100644
--- a/SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c
+++ b/SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c
@@ -2,7 +2,7 @@
   This module implements Hash2 Protocol.
 
 (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -18,241 +18,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "Driver.h"
 
-/**
-  Retrieves the size, in bytes, of the context buffer required for hash operations.
-
-  If this interface is not supported, then return zero.
-
-  @return  The size, in bytes, of the context buffer required for hash operations.
-  @retval  0   This interface is not supported.
-
-**/
-typedef
-UINTN
-(EFIAPI *EFI_HASH_GET_CONTEXT_SIZE) (
-  VOID
-  );
-
-/**
-  Initializes user-supplied memory pointed by Sha1Context as hash context for
-  subsequent use.
-
-  If HashContext is NULL, then return FALSE.
-  If this interface is not supported, then return FALSE.
-
-  @param[out]  HashContext  Pointer to Hashcontext being initialized.
-
-  @retval TRUE   Hash context initialization succeeded.
-  @retval FALSE  Hash context initialization failed.
-  @retval FALSE  This interface is not supported.
-
-**/
-typedef
-BOOLEAN
-(EFIAPI *EFI_HASH_INIT) (
-  OUT  VOID  *HashContext
-  );
-
-/**
-  Digests the input data and updates Hash context.
-
-  This function performs Hash digest on a data buffer of the specified size.
-  It can be called multiple times to compute the digest of long or discontinuous data streams.
-  Hash context should be already correctly initialized by HashInit(), and should not be finalized
-  by HashFinal(). Behavior with invalid context is undefined.
-
-  If HashContext is NULL, then return FALSE.
-  If this interface is not supported, then return FALSE.
-
-  @param[in, out]  HashContext  Pointer to the Hash context.
-  @param[in]       Data         Pointer to the buffer containing the data to be hashed.
-  @param[in]       DataSize     Size of Data buffer in bytes.
-
-  @retval TRUE   SHA-1 data digest succeeded.
-  @retval FALSE  SHA-1 data digest failed.
-  @retval FALSE  This interface is not supported.
-
-**/
-typedef
-BOOLEAN
-(EFIAPI *EFI_HASH_UPDATE) (
-  IN OUT  VOID        *HashContext,
-  IN      CONST VOID  *Data,
-  IN      UINTN       DataSize
-  );
-
-/**
-  Completes computation of the Hash digest value.
-
-  This function completes hash computation and retrieves the digest value into
-  the specified memory. After this function has been called, the Hash context cannot
-  be used again.
-  Hash context should be already correctly initialized by HashInit(), and should not be
-  finalized by HashFinal(). Behavior with invalid Hash context is undefined.
-
-  If HashContext is NULL, then return FALSE.
-  If HashValue is NULL, then return FALSE.
-  If this interface is not supported, then return FALSE.
-
-  @param[in, out]  HashContext  Pointer to the Hash context.
-  @param[out]      HashValue    Pointer to a buffer that receives the Hash digest
-                                value.
-
-  @retval TRUE   Hash digest computation succeeded.
-  @retval FALSE  Hash digest computation failed.
-  @retval FALSE  This interface is not supported.
-
-**/
-typedef
-BOOLEAN
-(EFIAPI *EFI_HASH_FINAL) (
-  IN OUT  VOID   *HashContext,
-  OUT     UINT8  *HashValue
-  );
-
 typedef struct {
-  EFI_GUID                   *Guid;
-  UINT32                     HashSize;
-  EFI_HASH_GET_CONTEXT_SIZE  GetContextSize;
-  EFI_HASH_INIT              Init;
-  EFI_HASH_UPDATE            Update;
-  EFI_HASH_FINAL             Final;
+  EFI_GUID                  *Guid;
+  UINT32                    HashSize;
+  CONST CHAR8               *DigestName;
 } EFI_HASH_INFO;
 
 EFI_HASH_INFO  mHashInfo[] = {
-  {&gEfiHashAlgorithmMD5Guid,     sizeof(EFI_MD5_HASH2),    Md5GetContextSize,    Md5Init,    Md5Update,    Md5Final  },
-  {&gEfiHashAlgorithmSha1Guid,    sizeof(EFI_SHA1_HASH2),   Sha1GetContextSize,   Sha1Init,   Sha1Update,   Sha1Final   },
-  {&gEfiHashAlgorithmSha256Guid,  sizeof(EFI_SHA256_HASH2), Sha256GetContextSize, Sha256Init, Sha256Update, Sha256Final },
-  {&gEfiHashAlgorithmSha384Guid,  sizeof(EFI_SHA384_HASH2), Sha384GetContextSize, Sha384Init, Sha384Update, Sha384Final },
-  {&gEfiHashAlgorithmSha512Guid,  sizeof(EFI_SHA512_HASH2), Sha512GetContextSize, Sha512Init, Sha512Update, Sha512Final },
-};
-
-/**
-  Returns the size of the hash which results from a specific algorithm.
-
-  @param[in]  This                  Points to this instance of EFI_HASH2_PROTOCOL.
-  @param[in]  HashAlgorithm         Points to the EFI_GUID which identifies the algorithm to use.
-  @param[out] HashSize              Holds the returned size of the algorithm's hash.
-
-  @retval EFI_SUCCESS           Hash size returned successfully.
-  @retval EFI_INVALID_PARAMETER This or HashSize is NULL.
-  @retval EFI_UNSUPPORTED       The algorithm specified by HashAlgorithm is not supported by this driver
-                                or HashAlgorithm is null.
-
-**/
-EFI_STATUS
-EFIAPI
-BaseCrypto2GetHashSize (
-  IN  CONST EFI_HASH2_PROTOCOL     *This,
-  IN  CONST EFI_GUID               *HashAlgorithm,
-  OUT UINTN                        *HashSize
-  );
-
-/**
-  Creates a hash for the specified message text. The hash is not extendable.
-  The output is final with any algorithm-required padding added by the function.
-
-  @param[in]  This          Points to this instance of EFI_HASH2_PROTOCOL.
-  @param[in]  HashAlgorithm Points to the EFI_GUID which identifies the algorithm to use.
-  @param[in]  Message       Points to the start of the message.
-  @param[in]  MessageSize   The size of Message, in bytes.
-  @param[in,out]  Hash      On input, points to a caller-allocated buffer of the size
-                              returned by GetHashSize() for the specified HashAlgorithm.
-                            On output, the buffer holds the resulting hash computed from the message.
-
-  @retval EFI_SUCCESS           Hash returned successfully.
-  @retval EFI_INVALID_PARAMETER This or Hash is NULL.
-  @retval EFI_UNSUPPORTED       The algorithm specified by HashAlgorithm is not supported by this driver
-                                or HashAlgorithm is Null.
-  @retval EFI_OUT_OF_RESOURCES  Some resource required by the function is not available
-                                or MessageSize is greater than platform maximum.
-
-**/
-EFI_STATUS
-EFIAPI
-BaseCrypto2Hash (
-  IN CONST EFI_HASH2_PROTOCOL      *This,
-  IN CONST EFI_GUID                *HashAlgorithm,
-  IN CONST UINT8                   *Message,
-  IN UINTN                         MessageSize,
-  IN OUT EFI_HASH2_OUTPUT          *Hash
-  );
-
-/**
-  This function must be called to initialize a digest calculation to be subsequently performed using the
-  EFI_HASH2_PROTOCOL functions HashUpdate() and HashFinal().
-
-  @param[in]  This          Points to this instance of EFI_HASH2_PROTOCOL.
-  @param[in]  HashAlgorithm Points to the EFI_GUID which identifies the algorithm to use.
-
-  @retval EFI_SUCCESS           Initialized successfully.
-  @retval EFI_INVALID_PARAMETER This is NULL.
-  @retval EFI_UNSUPPORTED       The algorithm specified by HashAlgorithm is not supported by this driver
-                                or HashAlgorithm is Null.
-  @retval EFI_OUT_OF_RESOURCES  Process failed due to lack of required resource.
-  @retval EFI_ALREADY_STARTED   This function is called when the operation in progress is still in processing Hash(),
-                                or HashInit() is already called before and not terminated by HashFinal() yet on the same instance.
-
-**/
-EFI_STATUS
-EFIAPI
-BaseCrypto2HashInit (
-  IN CONST EFI_HASH2_PROTOCOL      *This,
-  IN CONST EFI_GUID                *HashAlgorithm
-  );
-
-/**
-  Updates the hash of a computation in progress by adding a message text.
-
-  @param[in]  This          Points to this instance of EFI_HASH2_PROTOCOL.
-  @param[in]  Message       Points to the start of the message.
-  @param[in]  MessageSize   The size of Message, in bytes.
-
-  @retval EFI_SUCCESS           Digest in progress updated successfully.
-  @retval EFI_INVALID_PARAMETER This or Hash is NULL.
-  @retval EFI_OUT_OF_RESOURCES  Some resource required by the function is not available
-                                or MessageSize is greater than platform maximum.
-  @retval EFI_NOT_READY         This call was not preceded by a valid call to HashInit(),
-                                or the operation in progress was terminated by a call to Hash() or HashFinal() on the same instance.
-
-**/
-EFI_STATUS
-EFIAPI
-BaseCrypto2HashUpdate (
-  IN CONST EFI_HASH2_PROTOCOL      *This,
-  IN CONST UINT8                   *Message,
-  IN UINTN                         MessageSize
-  );
-
-/**
-  Finalizes a hash operation in progress and returns calculation result.
-  The output is final with any necessary padding added by the function.
-  The hash may not be further updated or extended after HashFinal().
-
-  @param[in]  This          Points to this instance of EFI_HASH2_PROTOCOL.
-  @param[in,out]  Hash      On input, points to a caller-allocated buffer of the size
-                              returned by GetHashSize() for the specified HashAlgorithm specified in preceding HashInit().
-                            On output, the buffer holds the resulting hash computed from the message.
-
-  @retval EFI_SUCCESS           Hash returned successfully.
-  @retval EFI_INVALID_PARAMETER This or Hash is NULL.
-  @retval EFI_NOT_READY         This call was not preceded by a valid call to HashInit() and at least one call to HashUpdate(),
-                                or the operation in progress was canceled by a call to Hash() on the same instance.
-
-**/
-EFI_STATUS
-EFIAPI
-BaseCrypto2HashFinal (
-  IN CONST EFI_HASH2_PROTOCOL      *This,
-  IN OUT EFI_HASH2_OUTPUT          *Hash
-  );
-
-EFI_HASH2_PROTOCOL mHash2Protocol = {
-  BaseCrypto2GetHashSize,
-  BaseCrypto2Hash,
-  BaseCrypto2HashInit,
-  BaseCrypto2HashUpdate,
-  BaseCrypto2HashFinal,
+  {&gEfiHashAlgorithmMD5Guid,     sizeof(EFI_MD5_HASH2),    "MD5"},
+  {&gEfiHashAlgorithmSha1Guid,    sizeof(EFI_SHA1_HASH2),   "SHA1"},
+  {&gEfiHashAlgorithmSha256Guid,  sizeof(EFI_SHA256_HASH2), "SHA256"},
+  {&gEfiHashAlgorithmSha384Guid,  sizeof(EFI_SHA384_HASH2), "SHA384"},
+  {&gEfiHashAlgorithmSha512Guid,  sizeof(EFI_SHA512_HASH2), "SHA512"},
 };
 
 /**
@@ -347,12 +124,7 @@ BaseCrypto2Hash (
   IN OUT EFI_HASH2_OUTPUT          *Hash
   )
 {
-  EFI_HASH_INFO            *HashInfo;
-  VOID                     *HashCtx;
-  UINTN                    CtxSize;
-  BOOLEAN                  Ret;
   EFI_STATUS               Status;
-  HASH2_INSTANCE_DATA      *Instance;
 
   Status = EFI_SUCCESS;
 
@@ -364,60 +136,18 @@ BaseCrypto2Hash (
     return EFI_UNSUPPORTED;
   }
 
-  HashInfo = GetHashInfo (HashAlgorithm);
-  if (HashInfo == NULL) {
-    return EFI_UNSUPPORTED;
-  }
-
-  Instance = HASH2_INSTANCE_DATA_FROM_THIS(This);
-  if (Instance->HashContext != NULL) {
-    FreePool (Instance->HashContext);
-  }
-  Instance->HashInfoContext = NULL;
-  Instance->HashContext = NULL;
-
-  //
-  // Start hash sequence
-  //
-  CtxSize = HashInfo->GetContextSize ();
-  if (CtxSize == 0) {
-    return EFI_UNSUPPORTED;
-  }
-  HashCtx = AllocatePool (CtxSize);
-  if (HashCtx == NULL) {
-    return EFI_OUT_OF_RESOURCES;
+  Status = This->HashInit (This, HashAlgorithm);
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
-  Ret = HashInfo->Init (HashCtx);
-  if (!Ret) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Done;
+  Status = This->HashUpdate (This, Message, MessageSize);
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
-  //
-  // Setup the context
-  //
-  Instance->HashContext = HashCtx;
-  Instance->HashInfoContext = HashInfo;
-
-  Ret = HashInfo->Update (HashCtx, Message, MessageSize);
-  if (!Ret) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Done;
-  }
+  Status = This->HashFinal (This, Hash);
 
-  Ret = HashInfo->Final (HashCtx, (UINT8 *)Hash->Sha1Hash);
-  if (!Ret) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Done;
-  }
-Done:
-  //
-  // Cleanup the context
-  //
-  FreePool (HashCtx);
-  Instance->HashInfoContext = NULL;
-  Instance->HashContext = NULL;
   return Status;
 }
 
@@ -446,8 +176,6 @@ BaseCrypto2HashInit (
 {
   EFI_HASH_INFO            *HashInfo;
   VOID                     *HashCtx;
-  UINTN                    CtxSize;
-  BOOLEAN                  Ret;
   HASH2_INSTANCE_DATA      *Instance;
 
   if (This == NULL) {
@@ -466,34 +194,23 @@ BaseCrypto2HashInit (
   //
   // Consistency Check
   //
-  Instance = HASH2_INSTANCE_DATA_FROM_THIS(This);
-  if ((Instance->HashContext != NULL) || (Instance->HashInfoContext != NULL)) {
+  Instance = HASH2_INSTANCE_DATA_FROM_THIS (This);
+  if (Instance->HashContext != NULL) {
     return EFI_ALREADY_STARTED;
   }
 
   //
   // Start hash sequence
   //
-  CtxSize = HashInfo->GetContextSize ();
-  if (CtxSize == 0) {
-    return EFI_UNSUPPORTED;
-  }
-  HashCtx = AllocatePool (CtxSize);
+  HashCtx = EvpMdInit (HashInfo->DigestName);
   if (HashCtx == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
 
-  Ret = HashInfo->Init (HashCtx);
-  if (!Ret) {
-    FreePool (HashCtx);
-    return EFI_OUT_OF_RESOURCES;
-  }
-
   //
   // Setup the context
   //
   Instance->HashContext = HashCtx;
-  Instance->HashInfoContext = HashInfo;
   Instance->Updated = FALSE;
 
   return EFI_SUCCESS;
@@ -522,7 +239,6 @@ BaseCrypto2HashUpdate (
   IN UINTN                         MessageSize
   )
 {
-  EFI_HASH_INFO            *HashInfo;
   VOID                     *HashCtx;
   BOOLEAN                  Ret;
   HASH2_INSTANCE_DATA      *Instance;
@@ -535,13 +251,12 @@ BaseCrypto2HashUpdate (
   // Consistency Check
   //
   Instance = HASH2_INSTANCE_DATA_FROM_THIS(This);
-  if ((Instance->HashContext == NULL) || (Instance->HashInfoContext == NULL)) {
+  if (Instance->HashContext == NULL) {
     return EFI_NOT_READY;
   }
-  HashInfo = Instance->HashInfoContext;
   HashCtx  = Instance->HashContext;
 
-  Ret = HashInfo->Update (HashCtx, Message, MessageSize);
+  Ret = EvpMdUpdate (HashCtx, Message, MessageSize);
   if (!Ret) {
     return EFI_OUT_OF_RESOURCES;
   }
@@ -574,8 +289,6 @@ BaseCrypto2HashFinal (
   IN OUT EFI_HASH2_OUTPUT          *Hash
   )
 {
-  EFI_HASH_INFO            *HashInfo;
-  VOID                     *HashCtx;
   BOOLEAN                  Ret;
   HASH2_INSTANCE_DATA      *Instance;
 
@@ -587,20 +300,16 @@ BaseCrypto2HashFinal (
   // Consistency Check
   //
   Instance = HASH2_INSTANCE_DATA_FROM_THIS(This);
-  if ((Instance->HashContext == NULL) || (Instance->HashInfoContext == NULL) ||
+  if ((Instance->HashContext == NULL) ||
       (!Instance->Updated)) {
     return EFI_NOT_READY;
   }
-  HashInfo = Instance->HashInfoContext;
-  HashCtx  = Instance->HashContext;
 
-  Ret = HashInfo->Final (HashCtx, (UINT8 *)Hash->Sha1Hash);
+  Ret = EvpMdFinal (Instance->HashContext, (UINT8 *)Hash->Sha1Hash);
 
   //
   // Cleanup the context
   //
-  FreePool (HashCtx);
-  Instance->HashInfoContext = NULL;
   Instance->HashContext = NULL;
   Instance->Updated = FALSE;
 
@@ -610,3 +319,11 @@ BaseCrypto2HashFinal (
 
   return EFI_SUCCESS;
 }
+
+EFI_HASH2_PROTOCOL mHash2Protocol = {
+  BaseCrypto2GetHashSize,
+  BaseCrypto2Hash,
+  BaseCrypto2HashInit,
+  BaseCrypto2HashUpdate,
+  BaseCrypto2HashFinal,
+};
-- 
2.28.0.windows.1


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

* Re: [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-09-16  0:58 [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J
                   ` (2 preceding siblings ...)
  2020-09-16  0:59 ` [PATCH v3 3/3] SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP interface Zurcher, Christopher J
@ 2020-09-16  3:00 ` Yao, Jiewen
  3 siblings, 0 replies; 13+ messages in thread
From: Yao, Jiewen @ 2020-09-16  3:00 UTC (permalink / raw)
  To: Zurcher, Christopher J, devel@edk2.groups.io
  Cc: Laszlo Ersek, Wang, Jian J, Lu, XiaoyuX

The series 1~3: reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

I would like to wait for at least one week to see if anyone has size concern - Hash2DxeCrypto grew from ~26k to ~253k.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> Sent: Wednesday, September 16, 2020 8:59 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest
> interface
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545
> 
> V3 changes:
> Added list of valid Digest Names to EvpMdInit() header
> Added missing copy of CryptEvpMdNull.c in BaseCryptLibNull folder
> 
> V2 changes:
> Added NullLib implementation
> Added Crypto Service implementation
> Rebased Hash2DxeCrypto to use EVP interface instead of low-level functions
> Removed unnecessary casts
> Added "HashAll" utility function
> Merged "New" and "Init" functions as well as "Final" and "Free" functions
>   Retained "Init/Update/Final" naming instead of "New/Update/Free" as this
>   conforms with common usage
> 
> Low-level interfaces to message digest (hash) functions have been deprecated
> in OpenSSL 3. In order to upgrade to OpenSSL 3, all direct calls to
> low-level functions (such as SHA256_Init() in CryptSha256.c) will need to
> be replaced by EVP inteface calls.
> 
> References:
>   https://www.openssl.org/docs/manmaster/man7/evp.html
>   https://www.openssl.org/docs/manmaster/man3/SHA256_Init.html
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> 
> Christopher J Zurcher (3):
>   CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
>   CryptoPkg: Add EVP to Crypto Service driver interface
>   SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP
>     interface
> 
>  CryptoPkg/CryptoPkg.dsc                                 |   3 +
>  CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf         |   1 +
>  CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf          |   1 +
>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf      |   1 +
>  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf          |   1 +
>  CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
>  CryptoPkg/Include/Library/BaseCryptLib.h                | 129 ++++++++
>  CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h    |  10 +
>  CryptoPkg/Private/Protocol/Crypto.h                     | 131 ++++++++
>  SecurityPkg/Hash2DxeCrypto/Driver.h                     |   1 -
>  CryptoPkg/Driver/Crypto.c                               | 152 ++++++++-
>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 257
> +++++++++++++++
>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 128 ++++++++
>  CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128 ++++++++
>  CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c  | 144 ++++++++
>  SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c             | 345 ++------------------
>  16 files changed, 1117 insertions(+), 316 deletions(-)
>  create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
>  create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
>  create mode 100644
> CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
> 
> --
> 2.28.0.windows.1


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

end of thread, other threads:[~2020-09-17  1:44 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-16  0:58 [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J
2020-09-16  0:59 ` [PATCH v3 1/3] " Zurcher, Christopher J
2020-09-16 11:06   ` [edk2-devel] " Laszlo Ersek
2020-09-16 11:44     ` Yao, Jiewen
2020-09-16 13:35       ` Laszlo Ersek
2020-09-16 14:17         ` Yao, Jiewen
2020-09-16 16:04           ` Laszlo Ersek
2020-09-17  1:21             ` Zurcher, Christopher J
2020-09-17  1:33               ` Yao, Jiewen
2020-09-17  1:44                 ` Zurcher, Christopher J
2020-09-16  0:59 ` [PATCH v3 2/3] CryptoPkg: Add EVP to Crypto Service driver interface Zurcher, Christopher J
2020-09-16  0:59 ` [PATCH v3 3/3] SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP interface Zurcher, Christopher J
2020-09-16  3:00 ` [PATCH v3 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Yao, Jiewen

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