[edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
Zurcher, Christopher J
christopher.j.zurcher at intel.com
Fri Aug 14 21:33:55 UTC 2020
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 at intel.com>
> Sent: Thursday, August 13, 2020 07:38
> To: Laszlo Ersek <lersek at redhat.com>; devel at edk2.groups.io; Zurcher,
> Christopher J <christopher.j.zurcher at intel.com>
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX <xiaoyux.lu at 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 at redhat.com>
> > Sent: Thursday, August 13, 2020 5:47 PM
> > To: devel at edk2.groups.io; Zurcher, Christopher J
> > <christopher.j.zurcher at intel.com>
> > Cc: Yao, Jiewen <jiewen.yao at intel.com>; Wang, Jian J
> <jian.j.wang at intel.com>;
> > Lu, XiaoyuX <xiaoyux.lu at 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 at intel.com>
> > > Cc: Jian J Wang <jian.j.wang at intel.com>
> > > Cc: Xiaoyu Lu <xiaoyux.lu at intel.com>
> > > Signed-off-by: Christopher J Zurcher <christopher.j.zurcher at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list