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

["Zurcher, Christopher J" <christopher.j.zurcher@intel.com>]

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.

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