From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
"Lu, Xiaoyu1" <xiaoyu1.lu@intel.com>,
"Jiang, Guomin" <guomin.jiang@intel.com>,
"Zurcher, Christopher" <christopher.zurcher@microsoft.com>,
Rebecca Cran <quic_rcran@quicinc.com>,
Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [Patch 00/12] CryptoPkg: Remove EC PCD and merge perf opt OpensslLibs
Date: Wed, 12 Oct 2022 01:24:30 +0000 [thread overview]
Message-ID: <CO1PR11MB4929E0BFE1A3B5BABDE1ED4FD2229@CO1PR11MB4929.namprd11.prod.outlook.com> (raw)
In-Reply-To: <MW4PR11MB587247AADED34E2C462B35048C229@MW4PR11MB5872.namprd11.prod.outlook.com>
Hi Jiewen,
Comments below.
Mike
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Tuesday, October 11, 2022 6:09 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Zurcher,
> Christopher <christopher.zurcher@microsoft.com>; Rebecca Cran <quic_rcran@quicinc.com>; Ard Biesheuvel <ardb@kernel.org>
> Subject: RE: [Patch 00/12] CryptoPkg: Remove EC PCD and merge perf opt OpensslLibs
>
> Thank you Mike.
>
> 1) I like the idea to combine multiple OpensslLibIA32/X64.inf into one OpensslLibAccel.inf.
> Also the cleanup looks good to me.
>
> 2) I also like the summary in readme in
> https://github.com/mdkinney/edk2/tree/CryptoPkg_RemoveEcPcd_MergeOptimizedOpensslLibs/CryptoPkg
> I notice some algorithms are listed Y(Deprecated) but N(Don't Use), such as Tdes, Arc4, Aes.Ecb*.
> But I don’t see the use case for those algorithms and I suggest a Y(Deprecated) have Y(Don't Use).
Good catch. I will fix.
>
> 3) About PcdOpensslEcEnabled
> I notice it is used in existing code -
> https://github.com/mdkinney/edk2/blob/CryptoPkg_RemoveEcPcd_MergeOptimizedOpensslLibs/CryptoPkg/Library/TlsLib/TlsConfig.c#L11
> 39
> Is this right way?
This was added since I started this work and was added back in by rebase. I will fix.
We can just remove the check for the PCD. If the OpensslLib instance does not include
SSL services, then the Null SSL services are present and the call to SSL_ctrl() will
return 0 which will force TlsSetEcCurve() to return EFI_UNSUPPORTED. It will also
ASSERT() informing the developer that a call to a service that depends on SSL was made
without SSL services available.
long
SSL_ctrl (
SSL *ssl,
int cmd,
long larg,
void *parg
)
{
ASSERT (FALSE);
return 0;
}
Likewise, if the OpensslLib instance does not support EC services, then the Null
EC services will be included and the call to EC_KEY_new_by_curve_name() will
return NULL which will force TlsSetEcCurve() to return EFI_UNSUPPORTED. It will also
ASSERT() informing the developer that a call to a service that depends on EC was made
without EC services available.
EC_KEY *
EC_KEY_new_by_curve_name (
int nid
)
{
ASSERT (FALSE);
return NULL;
}
>
> Thank you
> Yao, Jiewen
>
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Tuesday, October 11, 2022 11:04 PM
> > To: devel@edk2.groups.io
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Jiang,
> > Guomin <guomin.jiang@intel.com>; Zurcher, Christopher
> > <christopher.zurcher@microsoft.com>; Rebecca Cran
> > <quic_rcran@quicinc.com>; Ard Biesheuvel <ardb@kernel.org>
> > Subject: [Patch 00/12] CryptoPkg: Remove EC PCD and merge perf opt
> > OpensslLibs
> >
> > The recent addition of the Ecliptic Curve (EC) feature and the performance
> > optimization features increased the complexity for platforms to integrate
> > and enable these features. This series simplifies the platform configuration
> > as much as possible and improves the ability to manage the the size impact
> > of cryptographic services in each FW phase. A Readme.md is also added
> > that
> > provides an overview of the CryptoPkg design and features along with
> > platform
> > integration recommendations.
> >
> > This series also addresses private library class declarations missing from
> > CryptoPkg.dec and library instances not producing all the APIs defined
> > by the library classes. A review of the CryptoPkg EDK II meta data files
> > identified
> > a number of additional cleanups. The CryptoPkg.dsc file was also updated to
> > improve CI coverage for future CryptoPkg changes and identified some
> > unit test bug fixes.
> >
> > PR: https://github.com/tianocore/edk2/pull/3443
> > Branch:
> > https://github.com/mdkinney/edk2/tree/CryptoPkg_RemoveEcPcd_Merge
> > OptimizedOpensslLibs
> > Readme:
> > https://github.com/mdkinney/edk2/blob/CryptoPkg_RemoveEcPcd_Merge
> > OptimizedOpensslLibs/CryptoPkg/Readme.md
> >
> > Change Summary
> > ==============
> > * Document disabled/deprecated cryptographic services
> > * Add missing UNI files in BaseCryptLib
> > * Update BaseCryptLib internal functions to be STATIC and remove EFIAPI
> > * Add GLOBAL_REMOVE_IF_UNREFERENCED to BaseCryptLib global
> > variables
> > * Fix BaseCryptLib unit tests
> > * Cleanup BaseCryptLib and TlsLib INF files and
> > * Move SysCall/inet_pton.c from BaseCryptLib to TlsLib that uses it.
> > * Merge 4 performance optimized INFs into OpensslLib*Accel.inf
> > * Remove use of PcdOpensslEcEnabled and use OpensslLibFull*.inf instead
> > * Add OpensslLib and IntrinsicLib to CryptoPkg.dec as private library classes
> > * Update all OpensslLib instances to always produce all APIs in OpensslLib
> > class
> > * Move PrintLib dependency from OpensslLib INF files to BaseCryptLib INF
> > files
> > * Update CryptoPkg.dsc files to provide full CI test coverage across all the
> > supported combinations of OpensslLib, BaseCryptLib, and TlsLib instances.
> > * Remove PACKAGE profile from CryptoPkg.dsc and add
> > TARGET_UNIT_TESTS
> > profile. Adding TARGET_UNIT_TESTS profile is required to prevent a few
> > unit
> > test artifacts being included in non unit test builds of components.
> > * Add CryptoPkg Readme.md with overview and platform integration
> > details.
> > * Update host-based unit tests to always use OpensslLibFull.inf and add
> > unit
> > test coverage for OpensslLibFullAccel.inf.
> > * Add Readme.md with CryptoPkg overview and platform integration
> > documentation
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Xiaoyu Lu <xiaoyu1.lu@intel.com>
> > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > Cc: Christopher Zurcher <christopher.zurcher@microsoft.com>
> > Cc: Rebecca Cran <quic_rcran@quicinc.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > Michael D Kinney (12):
> > CryptoPkg: Document and disable deprecated crypto services
> > CryptoPkg/Library/BaseCryptLib: Add missing UNI file and fix format
> > CryptoPkg/Library/BaseCryptLib: Update internal functions/variables
> > CryptoPkg/Test/UnitTest/Library/BaseCryptLib: Unit test fixes
> > CryptoPkg/Library: Cleanup BaseCryptLib and TlsLib
> > CryptoPkg/Library/OpensslLib: Combine all performance optimized INFs
> > CryptoPkg/Library/OpensslLib: Produce consistent set of APIs
> > CryptoPkg/Library/OpensslLib: Remove PrintLib from INF files
> > CryptoPkg: Remove PcdOpensslEcEnabled from CryptoPkg.dec
> > CryptoPkg: Update DSC to improve CI test coverage
> > CryptoPkg: Fixed host-based unit tests
> > CryptoPkg: Add Readme.md
> >
> > CryptoPkg/CryptoPkg.ci.yaml | 11 +-
> > CryptoPkg/CryptoPkg.dec | 42 +-
> > CryptoPkg/CryptoPkg.dsc | 460 +++++++++---
> > .../Pcd/PcdCryptoServiceFamilyEnable.h | 122 +--
> > .../Library/BaseCryptLib/BaseCryptLib.inf | 10 +-
> > .../Library/BaseCryptLib/BaseCryptLib.uni | 2 -
> > .../Library/BaseCryptLib/Hmac/CryptHmac.c | 7 +
> > .../Library/BaseCryptLib/Kdf/CryptHkdf.c | 5 +-
> > .../Library/BaseCryptLib/PeiCryptLib.inf | 8 +-
> > .../Library/BaseCryptLib/PeiCryptLib.uni | 2 -
> > .../BaseCryptLib/Pk/CryptAuthenticode.c | 2 +-
> > .../BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c | 3 +-
> > .../BaseCryptLib/Pk/CryptPkcs7VerifyEku.c | 3 +
> > CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c | 44 +-
> > .../Library/BaseCryptLib/RuntimeCryptLib.inf | 9 +-
> > .../Library/BaseCryptLib/RuntimeCryptLib.uni | 2 -
> > .../Library/BaseCryptLib/SecCryptLib.inf | 13 +-
> > .../{SmmCryptLib.uni => SecCryptLib.uni} | 11 +-
> > .../Library/BaseCryptLib/SmmCryptLib.inf | 12 -
> > .../Library/BaseCryptLib/SmmCryptLib.uni | 2 -
> > .../BaseCryptLib/UnitTestHostBaseCryptLib.inf | 22 +-
> > .../Library/Include/openssl/opensslconf.h | 328 +++++++-
> > .../Include/openssl/opensslconf_generated.h | 333 ---------
> > CryptoPkg/Library/OpensslLib/EcSm2Null.c | 291 ++++++++
> > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 133 ++--
> > CryptoPkg/Library/OpensslLib/OpensslLib.uni | 10 +-
> > ...nsslLibIa32Gcc.inf => OpensslLibAccel.inf} | 189 +++--
> > .../Library/OpensslLib/OpensslLibAccel.uni | 14 +
> > .../OpensslLib/OpensslLibConstructor.c | 6 +-
> > .../Library/OpensslLib/OpensslLibCrypto.inf | 185 +++--
> > .../Library/OpensslLib/OpensslLibCrypto.uni | 11 +-
> > .../{OpensslLib.inf => OpensslLibFull.inf} | 143 ++--
> > .../{OpensslLib.uni => OpensslLibFull.uni} | 10 +-
> > ...sslLibIa32.inf => OpensslLibFullAccel.inf} | 192 +++--
> > .../OpensslLib/OpensslLibFullAccel.uni | 14 +
> > .../Library/OpensslLib/OpensslLibX64.inf | 706 ------------------
> > .../Library/OpensslLib/OpensslLibX64Gcc.inf | 706 ------------------
> > CryptoPkg/Library/OpensslLib/SslNull.c | 405 ++++++++++
> > .../SysCall/inet_pton.c | 0
> > CryptoPkg/Library/TlsLib/TlsConfig.c | 2 +-
> > CryptoPkg/Library/TlsLib/TlsLib.inf | 12 +-
> > CryptoPkg/Private/Library/IntrinsicLib.h | 16 +
> > CryptoPkg/Private/Library/OpensslLib.h | 14 +
> > CryptoPkg/Readme.md | 498 ++++++++++++
> > CryptoPkg/Test/CryptoPkgHostUnitTest.dsc | 17 +-
> > .../UnitTest/Library/BaseCryptLib/HmacTests.c | 17 +-
> > .../UnitTest/Library/BaseCryptLib/TSTests.c | 2 +-
> > .../TestBaseCryptLibHostAccel.inf | 55 ++
> > 48 files changed, 2667 insertions(+), 2434 deletions(-)
> > copy CryptoPkg/Library/BaseCryptLib/{SmmCryptLib.uni =>
> > SecCryptLib.uni} (74%)
> > delete mode 100644
> > CryptoPkg/Library/Include/openssl/opensslconf_generated.h
> > create mode 100644 CryptoPkg/Library/OpensslLib/EcSm2Null.c
> > rename CryptoPkg/Library/OpensslLib/{OpensslLibIa32Gcc.inf =>
> > OpensslLibAccel.inf} (79%)
> > create mode 100644 CryptoPkg/Library/OpensslLib/OpensslLibAccel.uni
> > copy CryptoPkg/Library/OpensslLib/{OpensslLib.inf => OpensslLibFull.inf}
> > (80%)
> > copy CryptoPkg/Library/OpensslLib/{OpensslLib.uni => OpensslLibFull.uni}
> > (56%)
> > rename CryptoPkg/Library/OpensslLib/{OpensslLibIa32.inf =>
> > OpensslLibFullAccel.inf} (79%)
> > create mode 100644
> > CryptoPkg/Library/OpensslLib/OpensslLibFullAccel.uni
> > delete mode 100644 CryptoPkg/Library/OpensslLib/OpensslLibX64.inf
> > delete mode 100644 CryptoPkg/Library/OpensslLib/OpensslLibX64Gcc.inf
> > create mode 100644 CryptoPkg/Library/OpensslLib/SslNull.c
> > rename CryptoPkg/Library/{BaseCryptLib => TlsLib}/SysCall/inet_pton.c
> > (100%)
> > create mode 100644 CryptoPkg/Private/Library/IntrinsicLib.h
> > create mode 100644 CryptoPkg/Private/Library/OpensslLib.h
> > create mode 100644 CryptoPkg/Readme.md
> > create mode 100644
> > CryptoPkg/Test/UnitTest/Library/BaseCryptLib/TestBaseCryptLibHostAccel.i
> > nf
> >
> > --
> > 2.37.1.windows.1
next prev parent reply other threads:[~2022-10-12 1:24 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-11 15:03 [Patch 00/12] CryptoPkg: Remove EC PCD and merge perf opt OpensslLibs Michael D Kinney
2022-10-11 15:03 ` [Patch 01/12] CryptoPkg: Document and disable deprecated crypto services Michael D Kinney
2022-10-11 15:03 ` [Patch 02/12] CryptoPkg/Library/BaseCryptLib: Add missing UNI file and fix format Michael D Kinney
2022-10-11 15:03 ` [Patch 03/12] CryptoPkg/Library/BaseCryptLib: Update internal functions/variables Michael D Kinney
2022-10-11 15:03 ` [Patch 04/12] CryptoPkg/Test/UnitTest/Library/BaseCryptLib: Unit test fixes Michael D Kinney
2022-10-11 15:03 ` [Patch 05/12] CryptoPkg/Library: Cleanup BaseCryptLib and TlsLib Michael D Kinney
2022-10-11 15:03 ` [Patch 06/12] CryptoPkg/Library/OpensslLib: Combine all performance optimized INFs Michael D Kinney
2022-10-11 23:20 ` [edk2-devel] " Christopher Zurcher
2022-10-11 23:58 ` Michael D Kinney
2022-10-11 15:03 ` [Patch 07/12] CryptoPkg/Library/OpensslLib: Produce consistent set of APIs Michael D Kinney
2022-10-11 15:03 ` [Patch 08/12] CryptoPkg/Library/OpensslLib: Remove PrintLib from INF files Michael D Kinney
2022-10-11 15:03 ` [Patch 09/12] CryptoPkg: Remove PcdOpensslEcEnabled from CryptoPkg.dec Michael D Kinney
2022-10-11 15:03 ` [Patch 10/12] CryptoPkg: Update DSC to improve CI test coverage Michael D Kinney
2022-10-11 15:03 ` [Patch 11/12] CryptoPkg: Fixed host-based unit tests Michael D Kinney
2022-10-11 15:03 ` [Patch 12/12] CryptoPkg: Add Readme.md Michael D Kinney
2022-10-12 1:08 ` [Patch 00/12] CryptoPkg: Remove EC PCD and merge perf opt OpensslLibs Yao, Jiewen
2022-10-12 1:24 ` Michael D Kinney [this message]
2022-10-12 1:36 ` Yao, Jiewen
2022-10-12 1:55 ` Michael D Kinney
2022-10-12 2:07 ` Yao, Jiewen
2022-10-12 2:23 ` Michael D Kinney
2022-10-12 8:33 ` Yao, Jiewen
[not found] ` <171D30322FF3DC63.20882@groups.io>
2022-10-12 2:12 ` [edk2-devel] " Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO1PR11MB4929E0BFE1A3B5BABDE1ED4FD2229@CO1PR11MB4929.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox