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....@intel.com> > Sent: Thursday, August 13, 2020 07:38 > To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; Zurcher, > Christopher J <christopher.j.zurc...@intel.com> > Cc: 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 > > 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 (#64295): https://edk2.groups.io/g/devel/message/64295 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] -=-=-=-=-=-=-=-=-=-=-=-
