public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
@ 2020-08-13  1:20 Zurcher, Christopher J
  2020-08-13  1:20 ` [PATCH 1/1] " Zurcher, Christopher J
  0 siblings, 1 reply; 6+ messages in thread
From: Zurcher, Christopher J @ 2020-08-13  1:20 UTC (permalink / raw)
  To: devel; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu

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

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.
For reviewers: The added file CryptEvpMd.c is based on CryptHmacSha256.c
and can be compared to that file to easily highlight changes.

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

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 (1):
  CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

 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/Include/Library/BaseCryptLib.h           | 122 +++++++++++
 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c    | 228 ++++++++++++++++++++
 6 files changed, 354 insertions(+)
 create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c

-- 
2.28.0.windows.1


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

* [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-08-13  1:20 [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J
@ 2020-08-13  1:20 ` Zurcher, Christopher J
  2020-08-13  9:46   ` [edk2-devel] " Laszlo Ersek
  0 siblings, 1 reply; 6+ messages in thread
From: Zurcher, Christopher J @ 2020-08-13  1:20 UTC (permalink / raw)
  To: devel; +Cc: 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: 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/Include/Library/BaseCryptLib.h           | 122 +++++++++++
 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c    | 228 ++++++++++++++++++++
 6 files changed, 354 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/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
index ae9bde9e37..f3bf8aac0c 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1012,6 +1012,128 @@ HmacSha256Final (
   OUT     UINT8  *HmacValue
   );
 
+//=====================================================================================
+//    EVP (Envelope) Primitive
+//=====================================================================================
+
+/**
+  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
+
+  @return  Pointer to the EVP_MD_CTX context that has been initialized.
+           If the allocations fails, EvpMdNew() returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdNew (
+  VOID
+  );
+
+/**
+  Release the specified EVP_MD_CTX context.
+
+  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be released.
+
+**/
+VOID
+EFIAPI
+EvpMdFree (
+  IN  VOID  *EvpMdContext
+  );
+
+/**
+  Initializes user-supplied memory pointed to by EvpMdContext as EVP_MD_CTX
+  context for subsequent use.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If DigestName is NULL, then return FALSE.
+
+  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being initialized.
+  @param[in]    DigestName    Pointer to the digest name.
+
+  @retval TRUE   EVP_MD_CTX context initialization succeeded.
+  @retval FALSE  EVP_MD_CTX context initialization failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdInit (
+  OUT VOID          *EvpMdContext,
+  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.
+
+  @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.
+
+  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 Digest is NULL, 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
+  );
+
 //=====================================================================================
 //    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..a38c300eb8
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
@@ -0,0 +1,228 @@
+/** @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.
+
+  @return  Pointer to the EVP_MD_CTX context that has been initialized.
+           If the allocations fails, EvpMdNew() returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdNew (
+  VOID
+  )
+{
+  //
+  // Allocates & Initializes EVP_MD_CTX Context by OpenSSL EVP_MD_CTX_new()
+  //
+  return (VOID *) EVP_MD_CTX_new ();
+}
+
+/**
+  Release the specified EVP_MD_CTX context.
+
+  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be released.
+
+**/
+VOID
+EFIAPI
+EvpMdFree (
+  IN  VOID  *EvpMdContext
+  )
+{
+  //
+  // Free OpenSSL EVP_MD_CTX Context
+  //
+  EVP_MD_CTX_free ((EVP_MD_CTX *)EvpMdContext);
+}
+
+/**
+  Initializes user-supplied memory pointed to by EvpMdContext as EVP_MD_CTX
+  context for subsequent use.
+
+  If EvpMdContext is NULL, then return FALSE.
+  If DigestName is NULL, then return FALSE.
+
+  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being initialized.
+  @param[in]    DigestName    Pointer to the digest name.
+
+  @retval TRUE   EVP_MD_CTX context initialization succeeded.
+  @retval FALSE  EVP_MD_CTX context initialization failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdInit (
+  OUT VOID          *EvpMdContext,
+  IN  CONST CHAR8   *DigestName
+  )
+{
+  CONST EVP_MD    *Digest;
+
+  //
+  // Check input parameters.
+  //
+  if (EvpMdContext == NULL || DigestName == NULL) {
+    return FALSE;
+  }
+
+  //
+  // OpenSSL EVP_MD Context Initialization
+  //
+  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
+    return FALSE;
+  }
+
+  Digest = EVP_get_digestbyname (DigestName);
+  if (Digest == NULL) {
+    return FALSE;
+  }
+
+  if (EVP_DigestInit_ex ((EVP_MD_CTX *)EvpMdContext, Digest, NULL) != 1) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+/**
+  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 ((EVP_MD_CTX *)NewEvpMdContext, (EVP_MD_CTX *)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.
+
+  @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 ((EVP_MD_CTX *)EvpMdContext, Data, DataSize) != 1) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+/**
+  Completes computation of the EVP digest value.
+
+  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 Digest is NULL, 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;
+
+  //
+  // Check input parameters.
+  //
+  if (EvpMdContext == NULL || DigestValue == NULL) {
+    return FALSE;
+  }
+
+  //
+  // OpenSSL EVP digest finalization
+  //
+  if (EVP_DigestFinal_ex ((EVP_MD_CTX *)EvpMdContext, DigestValue, &Length) != 1) {
+    return FALSE;
+  }
+  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
-- 
2.28.0.windows.1


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

* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-08-13  1:20 ` [PATCH 1/1] " Zurcher, Christopher J
@ 2020-08-13  9:46   ` Laszlo Ersek
  2020-08-13  9:47     ` Laszlo Ersek
  2020-08-13 14:38     ` Yao, Jiewen
  0 siblings, 2 replies; 6+ messages in thread
From: Laszlo Ersek @ 2020-08-13  9:46 UTC (permalink / raw)
  To: devel, christopher.j.zurcher; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu

Hi Christopher,

(+Mike,

On 08/13/20 03:20, 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: 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/Include/Library/BaseCryptLib.h           | 122 +++++++++++
>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c    | 228 ++++++++++++++++++++
>  6 files changed, 354 insertions(+)

(1) This patch extends the library class header, but updates only one
*set* of the three library instance *sets*. The other two instance
*sets* are:

- BaseCryptLibNull (just one instance), for which it should not be hard
to provide Null implementations of the new functions;

- BaseCryptLibOnProtocolPpi (three instances -- Pei, Dxe, Smm).


BaseCryptLibOnProtocolPpi is a tough nut, because it seems to require
extending:

- the crypto service driver at CryptoPkg/Driver/,

- the interface to that driver (CryptoPkg/Private/Protocol/Crypto.h --
reused by both "CryptoPkg/Private/Ppi/Crypto.h" and
"CryptoPkg/Private/Protocol/SmmCrypto.h"),

- the PCD_CRYPTO_SERVICE_FAMILY_ENABLE structure at
"CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h", for configuring
the driver,

- the various PcdCryptoServiceFamilyEnable settings / build profiles in
CryptoPkg/CryptoPkg.dsc.


> 
> 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/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
> index ae9bde9e37..f3bf8aac0c 100644
> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> @@ -1012,6 +1012,128 @@ HmacSha256Final (
>    OUT     UINT8  *HmacValue
>    );
>  
> +//=====================================================================================
> +//    EVP (Envelope) Primitive
> +//=====================================================================================
> +
> +/**
> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
> +
> +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> +           If the allocations fails, EvpMdNew() returns NULL.
> +
> +**/
> +VOID *
> +EFIAPI
> +EvpMdNew (
> +  VOID
> +  );
> +
> +/**
> +  Release the specified EVP_MD_CTX context.
> +
> +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be released.
> +
> +**/
> +VOID
> +EFIAPI
> +EvpMdFree (
> +  IN  VOID  *EvpMdContext
> +  );
> +
> +/**
> +  Initializes user-supplied memory pointed to by EvpMdContext as EVP_MD_CTX
> +  context for subsequent use.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If DigestName is NULL, then return FALSE.
> +
> +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being initialized.
> +  @param[in]    DigestName    Pointer to the digest name.
> +
> +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> +  @retval FALSE  EVP_MD_CTX context initialization failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdInit (
> +  OUT VOID          *EvpMdContext,
> +  IN  CONST CHAR8   *DigestName
> +  );

(2) I suggest merging the New() and Init() functions into one -- we
should only have a New() function:

VOID *
EFIAPI
EvpMdNew (
  IN  CONST CHAR8 *DigestName
  );

And this function would collect the actions from both the current New()
and Init() functions.

I'm proposing this because:

(2a) Init() is not really useful. In theory we might be able to call
Init() right after Final(), but that seems like a very obscure use case.

(2b) New() is useless without Init(); it makes no sense for a context to
exist, somewhere in the edk2 codebase, *between* New() and Init().

(2c) Init() used to encourage a usage pattern (at least with HMACs)
where the caller would allocate its own context storage first. This
usage was fraught with a memory leak
<https://bugzilla.tianocore.org/show_bug.cgi?id=2095#c7>. This isn't a
practical problem in this case, because we're -- correctly -- not
offering a GetContextSize() in this patch, but Init() is still a vestige
of a bad pattern.

Basically a caller should go through New -> Update[n] -> Final -> Free,
possibly calling Duplicate before or after Update.

... I could even entertain merging "Final" into "Free", as there seems
to be no point for a context to exist, in edk2, between Final and Free.
That would make for New -> Update[n] -> Free.

I understand that this would diverge from the current model of the
"digest function family", but I see it as an improvement.


> +
> +/**
> +  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.
> +
> +  @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.
> +
> +  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 Digest is NULL, 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
> +  );
> +
>  //=====================================================================================
>  //    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..a38c300eb8
> --- /dev/null
> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> @@ -0,0 +1,228 @@
> +/** @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.
> +
> +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> +           If the allocations fails, EvpMdNew() returns NULL.
> +
> +**/
> +VOID *
> +EFIAPI
> +EvpMdNew (
> +  VOID
> +  )
> +{
> +  //
> +  // Allocates & Initializes EVP_MD_CTX Context by OpenSSL EVP_MD_CTX_new()
> +  //
> +  return (VOID *) EVP_MD_CTX_new ();
> +}

(3) This (VOID*) cast is not needed, and makes the code harder to read,
in my opinion.


> +
> +/**
> +  Release the specified EVP_MD_CTX context.
> +
> +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be released.
> +
> +**/
> +VOID
> +EFIAPI
> +EvpMdFree (
> +  IN  VOID  *EvpMdContext
> +  )
> +{
> +  //
> +  // Free OpenSSL EVP_MD_CTX Context
> +  //
> +  EVP_MD_CTX_free ((EVP_MD_CTX *)EvpMdContext);
> +}

(4) Same as (3), just in the reverse direction -- I suggest dropping the
(EVP_MD_CTX *) cast.

(Points (3) and (4) apply to some more locations in this patch; if you
agree with my comments, please review the rest for these superfluous casts.)


(5) If possible, I'd suggest appending at least one other patch to this
series, for putting the new interfaces to use at once. We should pick a
library or driver in edk2 that currently consumes Sha256Init() and its
friends, and convert those calls to EvpMd*().

Thanks,
Laszlo

> +
> +/**
> +  Initializes user-supplied memory pointed to by EvpMdContext as EVP_MD_CTX
> +  context for subsequent use.
> +
> +  If EvpMdContext is NULL, then return FALSE.
> +  If DigestName is NULL, then return FALSE.
> +
> +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being initialized.
> +  @param[in]    DigestName    Pointer to the digest name.
> +
> +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> +  @retval FALSE  EVP_MD_CTX context initialization failed.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +EvpMdInit (
> +  OUT VOID          *EvpMdContext,
> +  IN  CONST CHAR8   *DigestName
> +  )
> +{
> +  CONST EVP_MD    *Digest;
> +
> +  //
> +  // Check input parameters.
> +  //
> +  if (EvpMdContext == NULL || DigestName == NULL) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // OpenSSL EVP_MD Context Initialization
> +  //
> +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> +    return FALSE;
> +  }
> +
> +  Digest = EVP_get_digestbyname (DigestName);
> +  if (Digest == NULL) {
> +    return FALSE;
> +  }
> +
> +  if (EVP_DigestInit_ex ((EVP_MD_CTX *)EvpMdContext, Digest, NULL) != 1) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +/**
> +  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 ((EVP_MD_CTX *)NewEvpMdContext, (EVP_MD_CTX *)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.
> +
> +  @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 ((EVP_MD_CTX *)EvpMdContext, Data, DataSize) != 1) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +/**
> +  Completes computation of the EVP digest value.
> +
> +  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 Digest is NULL, 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;
> +
> +  //
> +  // Check input parameters.
> +  //
> +  if (EvpMdContext == NULL || DigestValue == NULL) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // OpenSSL EVP digest finalization
> +  //
> +  if (EVP_DigestFinal_ex ((EVP_MD_CTX *)EvpMdContext, DigestValue, &Length) != 1) {
> +    return FALSE;
> +  }
> +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> 


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

* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-08-13  9:46   ` [edk2-devel] " Laszlo Ersek
@ 2020-08-13  9:47     ` Laszlo Ersek
  2020-08-13 14:38     ` Yao, Jiewen
  1 sibling, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2020-08-13  9:47 UTC (permalink / raw)
  To: devel, christopher.j.zurcher
  Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu, Michael Kinney

On 08/13/20 11:46, Laszlo Ersek wrote:
> Hi Christopher,
> 
> (+Mike,

bah I forgot to actually CC Mike. Doing that now.

Sorry!
Laszlo

> 
> On 08/13/20 03:20, 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: 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/Include/Library/BaseCryptLib.h           | 122 +++++++++++
>>  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c    | 228 ++++++++++++++++++++
>>  6 files changed, 354 insertions(+)
> 
> (1) This patch extends the library class header, but updates only one
> *set* of the three library instance *sets*. The other two instance
> *sets* are:
> 
> - BaseCryptLibNull (just one instance), for which it should not be hard
> to provide Null implementations of the new functions;
> 
> - BaseCryptLibOnProtocolPpi (three instances -- Pei, Dxe, Smm).
> 
> 
> BaseCryptLibOnProtocolPpi is a tough nut, because it seems to require
> extending:
> 
> - the crypto service driver at CryptoPkg/Driver/,
> 
> - the interface to that driver (CryptoPkg/Private/Protocol/Crypto.h --
> reused by both "CryptoPkg/Private/Ppi/Crypto.h" and
> "CryptoPkg/Private/Protocol/SmmCrypto.h"),
> 
> - the PCD_CRYPTO_SERVICE_FAMILY_ENABLE structure at
> "CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h", for configuring
> the driver,
> 
> - the various PcdCryptoServiceFamilyEnable settings / build profiles in
> CryptoPkg/CryptoPkg.dsc.
> 
> 
>>
>> 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/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
>> index ae9bde9e37..f3bf8aac0c 100644
>> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
>> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
>> @@ -1012,6 +1012,128 @@ HmacSha256Final (
>>    OUT     UINT8  *HmacValue
>>    );
>>  
>> +//=====================================================================================
>> +//    EVP (Envelope) Primitive
>> +//=====================================================================================
>> +
>> +/**
>> +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD use.
>> +
>> +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
>> +           If the allocations fails, EvpMdNew() returns NULL.
>> +
>> +**/
>> +VOID *
>> +EFIAPI
>> +EvpMdNew (
>> +  VOID
>> +  );
>> +
>> +/**
>> +  Release the specified EVP_MD_CTX context.
>> +
>> +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be released.
>> +
>> +**/
>> +VOID
>> +EFIAPI
>> +EvpMdFree (
>> +  IN  VOID  *EvpMdContext
>> +  );
>> +
>> +/**
>> +  Initializes user-supplied memory pointed to by EvpMdContext as EVP_MD_CTX
>> +  context for subsequent use.
>> +
>> +  If EvpMdContext is NULL, then return FALSE.
>> +  If DigestName is NULL, then return FALSE.
>> +
>> +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being initialized.
>> +  @param[in]    DigestName    Pointer to the digest name.
>> +
>> +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
>> +  @retval FALSE  EVP_MD_CTX context initialization failed.
>> +
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +EvpMdInit (
>> +  OUT VOID          *EvpMdContext,
>> +  IN  CONST CHAR8   *DigestName
>> +  );
> 
> (2) I suggest merging the New() and Init() functions into one -- we
> should only have a New() function:
> 
> VOID *
> EFIAPI
> EvpMdNew (
>   IN  CONST CHAR8 *DigestName
>   );
> 
> And this function would collect the actions from both the current New()
> and Init() functions.
> 
> I'm proposing this because:
> 
> (2a) Init() is not really useful. In theory we might be able to call
> Init() right after Final(), but that seems like a very obscure use case.
> 
> (2b) New() is useless without Init(); it makes no sense for a context to
> exist, somewhere in the edk2 codebase, *between* New() and Init().
> 
> (2c) Init() used to encourage a usage pattern (at least with HMACs)
> where the caller would allocate its own context storage first. This
> usage was fraught with a memory leak
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2095#c7>. This isn't a
> practical problem in this case, because we're -- correctly -- not
> offering a GetContextSize() in this patch, but Init() is still a vestige
> of a bad pattern.
> 
> Basically a caller should go through New -> Update[n] -> Final -> Free,
> possibly calling Duplicate before or after Update.
> 
> ... I could even entertain merging "Final" into "Free", as there seems
> to be no point for a context to exist, in edk2, between Final and Free.
> That would make for New -> Update[n] -> Free.
> 
> I understand that this would diverge from the current model of the
> "digest function family", but I see it as an improvement.
> 
> 
>> +
>> +/**
>> +  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.
>> +
>> +  @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.
>> +
>> +  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 Digest is NULL, 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
>> +  );
>> +
>>  //=====================================================================================
>>  //    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..a38c300eb8
>> --- /dev/null
>> +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
>> @@ -0,0 +1,228 @@
>> +/** @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.
>> +
>> +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
>> +           If the allocations fails, EvpMdNew() returns NULL.
>> +
>> +**/
>> +VOID *
>> +EFIAPI
>> +EvpMdNew (
>> +  VOID
>> +  )
>> +{
>> +  //
>> +  // Allocates & Initializes EVP_MD_CTX Context by OpenSSL EVP_MD_CTX_new()
>> +  //
>> +  return (VOID *) EVP_MD_CTX_new ();
>> +}
> 
> (3) This (VOID*) cast is not needed, and makes the code harder to read,
> in my opinion.
> 
> 
>> +
>> +/**
>> +  Release the specified EVP_MD_CTX context.
>> +
>> +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be released.
>> +
>> +**/
>> +VOID
>> +EFIAPI
>> +EvpMdFree (
>> +  IN  VOID  *EvpMdContext
>> +  )
>> +{
>> +  //
>> +  // Free OpenSSL EVP_MD_CTX Context
>> +  //
>> +  EVP_MD_CTX_free ((EVP_MD_CTX *)EvpMdContext);
>> +}
> 
> (4) Same as (3), just in the reverse direction -- I suggest dropping the
> (EVP_MD_CTX *) cast.
> 
> (Points (3) and (4) apply to some more locations in this patch; if you
> agree with my comments, please review the rest for these superfluous casts.)
> 
> 
> (5) If possible, I'd suggest appending at least one other patch to this
> series, for putting the new interfaces to use at once. We should pick a
> library or driver in edk2 that currently consumes Sha256Init() and its
> friends, and convert those calls to EvpMd*().
> 
> Thanks,
> Laszlo
> 
>> +
>> +/**
>> +  Initializes user-supplied memory pointed to by EvpMdContext as EVP_MD_CTX
>> +  context for subsequent use.
>> +
>> +  If EvpMdContext is NULL, then return FALSE.
>> +  If DigestName is NULL, then return FALSE.
>> +
>> +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being initialized.
>> +  @param[in]    DigestName    Pointer to the digest name.
>> +
>> +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
>> +  @retval FALSE  EVP_MD_CTX context initialization failed.
>> +
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +EvpMdInit (
>> +  OUT VOID          *EvpMdContext,
>> +  IN  CONST CHAR8   *DigestName
>> +  )
>> +{
>> +  CONST EVP_MD    *Digest;
>> +
>> +  //
>> +  // Check input parameters.
>> +  //
>> +  if (EvpMdContext == NULL || DigestName == NULL) {
>> +    return FALSE;
>> +  }
>> +
>> +  //
>> +  // OpenSSL EVP_MD Context Initialization
>> +  //
>> +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
>> +    return FALSE;
>> +  }
>> +
>> +  Digest = EVP_get_digestbyname (DigestName);
>> +  if (Digest == NULL) {
>> +    return FALSE;
>> +  }
>> +
>> +  if (EVP_DigestInit_ex ((EVP_MD_CTX *)EvpMdContext, Digest, NULL) != 1) {
>> +    return FALSE;
>> +  }
>> +
>> +  return TRUE;
>> +}
>> +
>> +/**
>> +  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 ((EVP_MD_CTX *)NewEvpMdContext, (EVP_MD_CTX *)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.
>> +
>> +  @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 ((EVP_MD_CTX *)EvpMdContext, Data, DataSize) != 1) {
>> +    return FALSE;
>> +  }
>> +
>> +  return TRUE;
>> +}
>> +
>> +/**
>> +  Completes computation of the EVP digest value.
>> +
>> +  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 Digest is NULL, 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;
>> +
>> +  //
>> +  // Check input parameters.
>> +  //
>> +  if (EvpMdContext == NULL || DigestValue == NULL) {
>> +    return FALSE;
>> +  }
>> +
>> +  //
>> +  // OpenSSL EVP digest finalization
>> +  //
>> +  if (EVP_DigestFinal_ex ((EVP_MD_CTX *)EvpMdContext, DigestValue, &Length) != 1) {
>> +    return FALSE;
>> +  }
>> +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
>> +    return FALSE;
>> +  }
>> +
>> +  return TRUE;
>> +}
>>
> 


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

* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
  2020-08-13  9:46   ` [edk2-devel] " Laszlo Ersek
  2020-08-13  9:47     ` Laszlo Ersek
@ 2020-08-13 14:38     ` Yao, Jiewen
  2020-08-14 21:33       ` Zurcher, Christopher J
  1 sibling, 1 reply; 6+ messages in thread
From: Yao, Jiewen @ 2020-08-13 14:38 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io, Zurcher, Christopher J
  Cc: Wang, Jian J, Lu, XiaoyuX

1) Agree with Laszlo.
We need extend internal protocol/ppi for this new API.

2) Do you have any data on the size difference between old SHA implementation or new MD implementation?

Thank you
Yao Jiewen

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, August 13, 2020 5:47 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 1/1] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> 
> Hi Christopher,
> 
> (+Mike,
> 
> On 08/13/20 03:20, 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: 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/Include/Library/BaseCryptLib.h           | 122 +++++++++++
> >  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c    | 228
> ++++++++++++++++++++
> >  6 files changed, 354 insertions(+)
> 
> (1) This patch extends the library class header, but updates only one
> *set* of the three library instance *sets*. The other two instance
> *sets* are:
> 
> - BaseCryptLibNull (just one instance), for which it should not be hard
> to provide Null implementations of the new functions;
> 
> - BaseCryptLibOnProtocolPpi (three instances -- Pei, Dxe, Smm).
> 
> 
> BaseCryptLibOnProtocolPpi is a tough nut, because it seems to require
> extending:
> 
> - the crypto service driver at CryptoPkg/Driver/,
> 
> - the interface to that driver (CryptoPkg/Private/Protocol/Crypto.h --
> reused by both "CryptoPkg/Private/Ppi/Crypto.h" and
> "CryptoPkg/Private/Protocol/SmmCrypto.h"),
> 
> - the PCD_CRYPTO_SERVICE_FAMILY_ENABLE structure at
> "CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h", for configuring
> the driver,
> 
> - the various PcdCryptoServiceFamilyEnable settings / build profiles in
> CryptoPkg/CryptoPkg.dsc.
> 
> 
> >
> > 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/Include/Library/BaseCryptLib.h
> b/CryptoPkg/Include/Library/BaseCryptLib.h
> > index ae9bde9e37..f3bf8aac0c 100644
> > --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> > @@ -1012,6 +1012,128 @@ HmacSha256Final (
> >    OUT     UINT8  *HmacValue
> >    );
> >
> >
> +//==============================================================
> =======================
> > +//    EVP (Envelope) Primitive
> >
> +//==============================================================
> =======================
> > +
> > +/**
> > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> use.
> > +
> > +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> > +           If the allocations fails, EvpMdNew() returns NULL.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdNew (
> > +  VOID
> > +  );
> > +
> > +/**
> > +  Release the specified EVP_MD_CTX context.
> > +
> > +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be
> released.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +EvpMdFree (
> > +  IN  VOID  *EvpMdContext
> > +  );
> > +
> > +/**
> > +  Initializes user-supplied memory pointed to by EvpMdContext as
> EVP_MD_CTX
> > +  context for subsequent use.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If DigestName is NULL, then return FALSE.
> > +
> > +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being
> initialized.
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +
> > +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> > +  @retval FALSE  EVP_MD_CTX context initialization failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdInit (
> > +  OUT VOID          *EvpMdContext,
> > +  IN  CONST CHAR8   *DigestName
> > +  );
> 
> (2) I suggest merging the New() and Init() functions into one -- we
> should only have a New() function:
> 
> VOID *
> EFIAPI
> EvpMdNew (
>   IN  CONST CHAR8 *DigestName
>   );
> 
> And this function would collect the actions from both the current New()
> and Init() functions.
> 
> I'm proposing this because:
> 
> (2a) Init() is not really useful. In theory we might be able to call
> Init() right after Final(), but that seems like a very obscure use case.
> 
> (2b) New() is useless without Init(); it makes no sense for a context to
> exist, somewhere in the edk2 codebase, *between* New() and Init().
> 
> (2c) Init() used to encourage a usage pattern (at least with HMACs)
> where the caller would allocate its own context storage first. This
> usage was fraught with a memory leak
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2095#c7>. This isn't a
> practical problem in this case, because we're -- correctly -- not
> offering a GetContextSize() in this patch, but Init() is still a vestige
> of a bad pattern.
> 
> Basically a caller should go through New -> Update[n] -> Final -> Free,
> possibly calling Duplicate before or after Update.
> 
> ... I could even entertain merging "Final" into "Free", as there seems
> to be no point for a context to exist, in edk2, between Final and Free.
> That would make for New -> Update[n] -> Free.
> 
> I understand that this would diverge from the current model of the
> "digest function family", but I see it as an improvement.
> 
> 
> > +
> > +/**
> > +  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.
> > +
> > +  @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.
> > +
> > +  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 Digest is NULL, 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
> > +  );
> > +
> >
> //===============================================================
> ======================
> >  //    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..a38c300eb8
> > --- /dev/null
> > +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> > @@ -0,0 +1,228 @@
> > +/** @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.
> > +
> > +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> > +           If the allocations fails, EvpMdNew() returns NULL.
> > +
> > +**/
> > +VOID *
> > +EFIAPI
> > +EvpMdNew (
> > +  VOID
> > +  )
> > +{
> > +  //
> > +  // Allocates & Initializes EVP_MD_CTX Context by OpenSSL
> EVP_MD_CTX_new()
> > +  //
> > +  return (VOID *) EVP_MD_CTX_new ();
> > +}
> 
> (3) This (VOID*) cast is not needed, and makes the code harder to read,
> in my opinion.
> 
> 
> > +
> > +/**
> > +  Release the specified EVP_MD_CTX context.
> > +
> > +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be
> released.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +EvpMdFree (
> > +  IN  VOID  *EvpMdContext
> > +  )
> > +{
> > +  //
> > +  // Free OpenSSL EVP_MD_CTX Context
> > +  //
> > +  EVP_MD_CTX_free ((EVP_MD_CTX *)EvpMdContext);
> > +}
> 
> (4) Same as (3), just in the reverse direction -- I suggest dropping the
> (EVP_MD_CTX *) cast.
> 
> (Points (3) and (4) apply to some more locations in this patch; if you
> agree with my comments, please review the rest for these superfluous casts.)
> 
> 
> (5) If possible, I'd suggest appending at least one other patch to this
> series, for putting the new interfaces to use at once. We should pick a
> library or driver in edk2 that currently consumes Sha256Init() and its
> friends, and convert those calls to EvpMd*().
> 
> Thanks,
> Laszlo
> 
> > +
> > +/**
> > +  Initializes user-supplied memory pointed to by EvpMdContext as
> EVP_MD_CTX
> > +  context for subsequent use.
> > +
> > +  If EvpMdContext is NULL, then return FALSE.
> > +  If DigestName is NULL, then return FALSE.
> > +
> > +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being
> initialized.
> > +  @param[in]    DigestName    Pointer to the digest name.
> > +
> > +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> > +  @retval FALSE  EVP_MD_CTX context initialization failed.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +EvpMdInit (
> > +  OUT VOID          *EvpMdContext,
> > +  IN  CONST CHAR8   *DigestName
> > +  )
> > +{
> > +  CONST EVP_MD    *Digest;
> > +
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL || DigestName == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // OpenSSL EVP_MD Context Initialization
> > +  //
> > +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  Digest = EVP_get_digestbyname (DigestName);
> > +  if (Digest == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  if (EVP_DigestInit_ex ((EVP_MD_CTX *)EvpMdContext, Digest, NULL) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  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 ((EVP_MD_CTX *)NewEvpMdContext, (EVP_MD_CTX
> *)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.
> > +
> > +  @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 ((EVP_MD_CTX *)EvpMdContext, Data, DataSize) != 1)
> {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Completes computation of the EVP digest value.
> > +
> > +  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 Digest is NULL, 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;
> > +
> > +  //
> > +  // Check input parameters.
> > +  //
> > +  if (EvpMdContext == NULL || DigestValue == NULL) {
> > +    return FALSE;
> > +  }
> > +
> > +  //
> > +  // OpenSSL EVP digest finalization
> > +  //
> > +  if (EVP_DigestFinal_ex ((EVP_MD_CTX *)EvpMdContext, DigestValue,
> &Length) != 1) {
> > +    return FALSE;
> > +  }
> > +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> >


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

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

I replaced the MD5 and SHAx functions with EVP functions in Hash2DxeCrypto, and it grew from ~26k to ~253k.

--
Christopher Zurcher

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Thursday, August 13, 2020 07:38
> To: Laszlo Ersek <lersek@redhat.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 1/1] CryptoPkg/BaseCryptLib: Add EVP
> (Envelope) Digest interface
> 
> 1) Agree with Laszlo.
> We need extend internal protocol/ppi for this new API.
> 
> 2) Do you have any data on the size difference between old SHA implementation
> or new MD implementation?
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Thursday, August 13, 2020 5:47 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 1/1] CryptoPkg/BaseCryptLib: Add EVP
> > (Envelope) Digest interface
> >
> > Hi Christopher,
> >
> > (+Mike,
> >
> > On 08/13/20 03:20, 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: 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/Include/Library/BaseCryptLib.h           | 122 +++++++++++
> > >  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c    | 228
> > ++++++++++++++++++++
> > >  6 files changed, 354 insertions(+)
> >
> > (1) This patch extends the library class header, but updates only one
> > *set* of the three library instance *sets*. The other two instance
> > *sets* are:
> >
> > - BaseCryptLibNull (just one instance), for which it should not be hard
> > to provide Null implementations of the new functions;
> >
> > - BaseCryptLibOnProtocolPpi (three instances -- Pei, Dxe, Smm).
> >
> >
> > BaseCryptLibOnProtocolPpi is a tough nut, because it seems to require
> > extending:
> >
> > - the crypto service driver at CryptoPkg/Driver/,
> >
> > - the interface to that driver (CryptoPkg/Private/Protocol/Crypto.h --
> > reused by both "CryptoPkg/Private/Ppi/Crypto.h" and
> > "CryptoPkg/Private/Protocol/SmmCrypto.h"),
> >
> > - the PCD_CRYPTO_SERVICE_FAMILY_ENABLE structure at
> > "CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h", for configuring
> > the driver,
> >
> > - the various PcdCryptoServiceFamilyEnable settings / build profiles in
> > CryptoPkg/CryptoPkg.dsc.
> >
> >
> > >
> > > 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/Include/Library/BaseCryptLib.h
> > b/CryptoPkg/Include/Library/BaseCryptLib.h
> > > index ae9bde9e37..f3bf8aac0c 100644
> > > --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> > > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> > > @@ -1012,6 +1012,128 @@ HmacSha256Final (
> > >    OUT     UINT8  *HmacValue
> > >    );
> > >
> > >
> > +//==============================================================
> > =======================
> > > +//    EVP (Envelope) Primitive
> > >
> > +//==============================================================
> > =======================
> > > +
> > > +/**
> > > +  Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
> > use.
> > > +
> > > +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> > > +           If the allocations fails, EvpMdNew() returns NULL.
> > > +
> > > +**/
> > > +VOID *
> > > +EFIAPI
> > > +EvpMdNew (
> > > +  VOID
> > > +  );
> > > +
> > > +/**
> > > +  Release the specified EVP_MD_CTX context.
> > > +
> > > +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be
> > released.
> > > +
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +EvpMdFree (
> > > +  IN  VOID  *EvpMdContext
> > > +  );
> > > +
> > > +/**
> > > +  Initializes user-supplied memory pointed to by EvpMdContext as
> > EVP_MD_CTX
> > > +  context for subsequent use.
> > > +
> > > +  If EvpMdContext is NULL, then return FALSE.
> > > +  If DigestName is NULL, then return FALSE.
> > > +
> > > +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being
> > initialized.
> > > +  @param[in]    DigestName    Pointer to the digest name.
> > > +
> > > +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> > > +  @retval FALSE  EVP_MD_CTX context initialization failed.
> > > +
> > > +**/
> > > +BOOLEAN
> > > +EFIAPI
> > > +EvpMdInit (
> > > +  OUT VOID          *EvpMdContext,
> > > +  IN  CONST CHAR8   *DigestName
> > > +  );
> >
> > (2) I suggest merging the New() and Init() functions into one -- we
> > should only have a New() function:
> >
> > VOID *
> > EFIAPI
> > EvpMdNew (
> >   IN  CONST CHAR8 *DigestName
> >   );
> >
> > And this function would collect the actions from both the current New()
> > and Init() functions.
> >
> > I'm proposing this because:
> >
> > (2a) Init() is not really useful. In theory we might be able to call
> > Init() right after Final(), but that seems like a very obscure use case.
> >
> > (2b) New() is useless without Init(); it makes no sense for a context to
> > exist, somewhere in the edk2 codebase, *between* New() and Init().
> >
> > (2c) Init() used to encourage a usage pattern (at least with HMACs)
> > where the caller would allocate its own context storage first. This
> > usage was fraught with a memory leak
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=2095#c7>. This isn't a
> > practical problem in this case, because we're -- correctly -- not
> > offering a GetContextSize() in this patch, but Init() is still a vestige
> > of a bad pattern.
> >
> > Basically a caller should go through New -> Update[n] -> Final -> Free,
> > possibly calling Duplicate before or after Update.
> >
> > ... I could even entertain merging "Final" into "Free", as there seems
> > to be no point for a context to exist, in edk2, between Final and Free.
> > That would make for New -> Update[n] -> Free.
> >
> > I understand that this would diverge from the current model of the
> > "digest function family", but I see it as an improvement.
> >
> >
> > > +
> > > +/**
> > > +  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.
> > > +
> > > +  @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.
> > > +
> > > +  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 Digest is NULL, 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
> > > +  );
> > > +
> > >
> > //===============================================================
> > ======================
> > >  //    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..a38c300eb8
> > > --- /dev/null
> > > +++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> > > @@ -0,0 +1,228 @@
> > > +/** @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.
> > > +
> > > +  @return  Pointer to the EVP_MD_CTX context that has been initialized.
> > > +           If the allocations fails, EvpMdNew() returns NULL.
> > > +
> > > +**/
> > > +VOID *
> > > +EFIAPI
> > > +EvpMdNew (
> > > +  VOID
> > > +  )
> > > +{
> > > +  //
> > > +  // Allocates & Initializes EVP_MD_CTX Context by OpenSSL
> > EVP_MD_CTX_new()
> > > +  //
> > > +  return (VOID *) EVP_MD_CTX_new ();
> > > +}
> >
> > (3) This (VOID*) cast is not needed, and makes the code harder to read,
> > in my opinion.
> >
> >
> > > +
> > > +/**
> > > +  Release the specified EVP_MD_CTX context.
> > > +
> > > +  @param[in]  EvpMdContext  Pointer to the EVP_MD_CTX context to be
> > released.
> > > +
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +EvpMdFree (
> > > +  IN  VOID  *EvpMdContext
> > > +  )
> > > +{
> > > +  //
> > > +  // Free OpenSSL EVP_MD_CTX Context
> > > +  //
> > > +  EVP_MD_CTX_free ((EVP_MD_CTX *)EvpMdContext);
> > > +}
> >
> > (4) Same as (3), just in the reverse direction -- I suggest dropping the
> > (EVP_MD_CTX *) cast.
> >
> > (Points (3) and (4) apply to some more locations in this patch; if you
> > agree with my comments, please review the rest for these superfluous
> casts.)
> >
> >
> > (5) If possible, I'd suggest appending at least one other patch to this
> > series, for putting the new interfaces to use at once. We should pick a
> > library or driver in edk2 that currently consumes Sha256Init() and its
> > friends, and convert those calls to EvpMd*().
> >
> > Thanks,
> > Laszlo
> >
> > > +
> > > +/**
> > > +  Initializes user-supplied memory pointed to by EvpMdContext as
> > EVP_MD_CTX
> > > +  context for subsequent use.
> > > +
> > > +  If EvpMdContext is NULL, then return FALSE.
> > > +  If DigestName is NULL, then return FALSE.
> > > +
> > > +  @param[out]   EvpMdContext  Pointer to EVP_MD_CTX context being
> > initialized.
> > > +  @param[in]    DigestName    Pointer to the digest name.
> > > +
> > > +  @retval TRUE   EVP_MD_CTX context initialization succeeded.
> > > +  @retval FALSE  EVP_MD_CTX context initialization failed.
> > > +
> > > +**/
> > > +BOOLEAN
> > > +EFIAPI
> > > +EvpMdInit (
> > > +  OUT VOID          *EvpMdContext,
> > > +  IN  CONST CHAR8   *DigestName
> > > +  )
> > > +{
> > > +  CONST EVP_MD    *Digest;
> > > +
> > > +  //
> > > +  // Check input parameters.
> > > +  //
> > > +  if (EvpMdContext == NULL || DigestName == NULL) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  //
> > > +  // OpenSSL EVP_MD Context Initialization
> > > +  //
> > > +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  Digest = EVP_get_digestbyname (DigestName);
> > > +  if (Digest == NULL) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  if (EVP_DigestInit_ex ((EVP_MD_CTX *)EvpMdContext, Digest, NULL) != 1)
> {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  return TRUE;
> > > +}
> > > +
> > > +/**
> > > +  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 ((EVP_MD_CTX *)NewEvpMdContext, (EVP_MD_CTX
> > *)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.
> > > +
> > > +  @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 ((EVP_MD_CTX *)EvpMdContext, Data, DataSize) !=
> 1)
> > {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  return TRUE;
> > > +}
> > > +
> > > +/**
> > > +  Completes computation of the EVP digest value.
> > > +
> > > +  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 Digest is NULL, 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;
> > > +
> > > +  //
> > > +  // Check input parameters.
> > > +  //
> > > +  if (EvpMdContext == NULL || DigestValue == NULL) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  //
> > > +  // OpenSSL EVP digest finalization
> > > +  //
> > > +  if (EVP_DigestFinal_ex ((EVP_MD_CTX *)EvpMdContext, DigestValue,
> > &Length) != 1) {
> > > +    return FALSE;
> > > +  }
> > > +  if (EVP_MD_CTX_reset ((EVP_MD_CTX *)EvpMdContext) != 1) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  return TRUE;
> > > +}
> > >


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

end of thread, other threads:[~2020-08-14 21:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-13  1:20 [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface Zurcher, Christopher J
2020-08-13  1:20 ` [PATCH 1/1] " Zurcher, Christopher J
2020-08-13  9:46   ` [edk2-devel] " Laszlo Ersek
2020-08-13  9:47     ` Laszlo Ersek
2020-08-13 14:38     ` Yao, Jiewen
2020-08-14 21:33       ` Zurcher, Christopher J

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