Re: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

["Yao, Jiewen" <jiewen.yao@intel.com>]

Comments below:

> -----Original Message-----
> From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> Sent: Tuesday, September 15, 2020 10:54 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: RE: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest
> interface
> 
> Replies inline
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Monday, September 14, 2020 18:22
> > To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
> > devel@edk2.groups.io
> > Cc: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>;
> > Lu, XiaoyuX <xiaoyux.lu@intel.com>
> > Subject: RE: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope)
> Digest
> > interface
> >
> > Hi Zurcher:
> > Thanks for your work.
> > 1) Please share with us what unit test you have done for all new APIs.
> 
> I unit tested both the native and Crypto Service implementations through the
> modified Hash2DxeCrypto protocol.
> I tested the Init/Update/Final flow as well as the HashAll function.
> 
> >
> > 2) Please add comment on what is the valid DigestName in EvpMdInit().
> > Otherwise, people will have no idea on that.
> 
> I will add valid options in a comment.
> I have to send another patch anyway to add a file in my commit (missed the
> second copy of CryptEvpMdNull.c in the NullLib folder).
> 
> >
> > 3) I assume the size will be unchanged if a module does not use the new
> EVPMD
> > API, such as UEFI secure boot, TCG trusted boot. Please double confirm if
> > that is right understanding.
> 
> Yes, if a module does not call the EVPMD API, it should not grow in size.
> The Crypto Service build output CryptoDxe.efi grew less than 1% after enabling
> the EvpMd function family through PcdCryptoServiceFamilyEnable.
> I suspect this is because the HmacSha256 Family was already enabled, and inside
> OpenSSL the HMAC functions are wrappers for EVP functions.
> So even with library-mode BaseCryptLib, any module that already calls the
> HMAC functions should not see any size change by adding EVP.
> 
> >
> > Hi all:
> > I would like collect feedback on below:
> > -- "I replaced the MD5 and SHAx functions with EVP functions in
> > Hash2DxeCrypto, and it grew from ~26k to ~253k."
> >
> > If there is negative size impact for the platform BIOS that is using
> > Hash2DxeCrypto, please share with the community.
> 
> The size change in Hash2DxeCrypto was seen while using the library-mode
> BaseCryptLib implementation, not the Crypto Services driver.
> We cannot move to OpenSSL 3 without replacing all low-level algorithm
> functions with EVP calls, so platforms using Hash2DxeCrypto will have to eat the
> size increase eventually.
> For platforms using Hash2DxeCrypto, moving to the Crypto Services model
> should help offset this increase.

[Jiewen] I think we need evaluate the size impact to decide if/when/how to move to OpenSSL 3 later.
We can cross the bridge when we come to it.


> 
> Thanks,
> Christopher Zurcher
> 
> >
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> > > Sent: Tuesday, September 15, 2020 8:58 AM
> > > To: devel@edk2.groups.io
> > > Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> > > Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> > > Subject: [PATCH v2 0/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest
> > > interface
> > >
> > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545
> > >
> > > V2 changes:
> > > Added NullLib implementation
> > > Added Crypto Service implementation
> > > Rebased Hash2DxeCrypto to use EVP interface instead of low-level functions
> > > Removed unnecessary casts
> > > Added "HashAll" utility function
> > > Merged "New" and "Init" functions as well as "Final" and "Free" functions
> > >   Retained "Init/Update/Final" naming instead of "New/Update/Free" as this
> > >   conforms with common usage
> > >
> > > 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.
> > >
> > > References:
> > >   https://www.openssl.org/docs/manmaster/man7/evp.html
> > >   https://www.openssl.org/docs/manmaster/man3/SHA256_Init.html
> > >
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > 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 (3):
> > >   CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
> > >   CryptoPkg: Add EVP to Crypto Service driver interface
> > >   SecurityPkg/Hash2DxeCrypto: Rebase Hash2DxeCrypto onto the EVP
> > >     interface
> > >
> > >  CryptoPkg/CryptoPkg.dsc                                 |   3 +
> > >  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/Library/BaseCryptLibNull/BaseCryptLibNull.inf |   1 +
> > >  CryptoPkg/Include/Library/BaseCryptLib.h                | 125 +++++++
> > >  CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h    |  10 +
> > >  CryptoPkg/Private/Protocol/Crypto.h                     | 127 +++++++
> > >  SecurityPkg/Hash2DxeCrypto/Driver.h                     |   1 -
> > >  CryptoPkg/Driver/Crypto.c                               | 148 ++++++++-
> > >  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c         | 253
> > ++++++++++++++
> > >  CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c     | 124 +++++++
> > >  CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c  | 140 ++++++++
> > >  SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c             | 345 ++----------
> > --------
> > >  15 files changed, 965 insertions(+), 316 deletions(-)
> > >  create mode 100644 CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
> > >  create mode 100644
> CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
> > >
> > > --
> > > 2.28.0.windows.1