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....@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 (#64159): https://edk2.groups.io/g/devel/message/64159 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] -=-=-=-=-=-=-=-=-=-=-=-
