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 <ler...@redhat.com> > Sent: Thursday, August 13, 2020 5:47 PM > To: devel@edk2.groups.io; Zurcher, Christopher J > <christopher.j.zurc...@intel.com> > Cc: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > Lu, XiaoyuX <xiaoyux...@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....@intel.com> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Xiaoyu Lu <xiaoyux...@intel.com> > > Signed-off-by: Christopher J Zurcher <christopher.j.zurc...@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; > > +} > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64222): https://edk2.groups.io/g/devel/message/64222 Mute This Topic: https://groups.io/mt/76159838/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
