Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Mike,
I did make some attempts with it, but it doesn't work, two troubles here:
Bob and Yuwei, please point out if I'm wrong:
1. Using member of structure PCD in INF isn’t supported by Basetools currently,
At least it cannot be used as FeatureFlag Expression.
2. As far as I know, structure PCD actually is a const array in code,
I afraid it will not work fine with precompile but we do have this need:
#if !FixedPcdGetBool (PcdOpensslEcEnabled)
# ifndef OPENSSL_NO_EC
# define OPENSSL_NO_EC
# endif
#endif
This is really caused by the bad structure of openssl,
maybe we use more detailed comments to remind developers to sync the two PCDs?
Thanks,
Yi
-Original Message-
From: Kinney, Michael D
Sent: Friday, September 23, 2022 1:25 PM
To: Li, Yi1 ; devel@edk2.groups.io; Chen, Christine
; Feng, Bob C ; Kinney, Michael D
Cc: Yao, Jiewen ; Wang, Jian J ;
Lu, Xiaoyu1 ; Jiang, Guomin
Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Yi,
I agree there are some complex interactions in the opensll sources.
Since you are defining a family for EC, can we use the EC Family != 0 instead of
PcdOpensslEcEnabled and remove PcdOpensslEcEnabled.
I want to make sure developers do not run into strange build failures if
they do not keep the 2 different PCDs aligned. I prefer a single PCD
setting to enable use of EC services.
I also noticed that the use of a PCD expression in an INF to select source
files does not work if the PCD value is specified with the --pcd flag on
the build command line. This looks like a significant bug with the PCD
expression in an INF file. This also needs to be fixed.
Mike
> -Original Message-
> From: Li, Yi1
> Sent: Thursday, September 22, 2022 8:02 PM
> To: Kinney, Michael D ; devel@edk2.groups.io
> Cc: Yao, Jiewen ; Wang, Jian J ;
> Lu, Xiaoyu1 ; Jiang,
> Guomin
> Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
>
> Hi Mike,
>
> 1. Yes, it matches.
> By Intel side, 100+kb(20%+) FV size increase will be a big concern, please
> refer to another internal email.
>
> 2. Additional size is coming from modules may consumed EC APIs, eg. TLS PEM
> X509 ...
>
> If we added EC source to OpensslLib.inf and disabled macro OPENSSL_NO_EC,
> those modules will link EC APIs and increase binary
> size,
> This an example from x509/x_pubkey.c , other modules is similar:
> #ifndef OPENSSL_NO_EC
> EC_KEY *d2i_EC_PUBKEY(EC_KEY **a, const unsigned char **pp, long length)
> {
> EVP_PKEY *pkey;
> EC_KEY *key = NULL;
> // call EC functions
> }
> #endif
>
> If we added EC source to OpensslLib.inf and enable macro OPENSSL_NO_EC, EC
> module will throw build error,
> Since some EC internal APIs or structs have been disabled by OPENSSL_NO_EC
> but not another.
> This an example from ec/ec_local.h , other error is similar:
>
> #ifndef OPENSSL_NO_EC
> typedef struct ec_group_st EC_GROUP;
> typedef struct ec_point_st EC_POINT;
> #endif
>
> // but this function not been enclosed by OPENSSL_NO_EC, and will throw build
> error
> int (*point_set_Jprojective_coordinates_GFp) (const EC_GROUP *,
> EC_POINT *, const BIGNUM *x,
> const BIGNUM *y,
> const BIGNUM *z, BN_CTX *);
>
> To avoid this annoying openssl error, we introduced conditional EC.
>
> Thanks,
> Yi
>
> -Original Message-
> From: Kinney, Michael D
> Sent: Friday, September 23, 2022 6:47 AM
> To: Li, Yi1 ; devel@edk2.groups.io; Kinney, Michael D
>
> Cc: Yao, Jiewen ; Wang, Jian J ;
> Lu, Xiaoyu1 ; Jiang,
> Guomin
> Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
>
> Hi Yi,
>
> I agree EC is an important feature.
>
> I did some analysis of the size impact to the CryptoPkg modules on current
> trunk
> with EC on and off. Uncompressed size is important for PEI Phase. For DXE and
> SMM phase, the Crypto services can always be compressed. From the table
> below,
> building all the EC services in the OpensslLib has no size impact to the NONE
> profile and the MIN_PEI profile. It has ~105 KB impact to compressed DXE/SMM
> usages that may use the MIN_DXE_MIN_SMM or ALL profiles.
>
>Uncompressed LZMA Compressed
> CPU CRYPTO_SERVICESModule EC=FALSE EC=TRUE EC=FALSE EC=TRUE
> Increase
> === === ===
>
> IA32 NONE CryptoPei2153621568 0
> KB
> IA32 NONE CryptoDxe2163221696 0
> KB
> IA32 NONE Cr
Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Yi,
I agree there are some complex interactions in the opensll sources.
Since you are defining a family for EC, can we use the EC Family != 0 instead of
PcdOpensslEcEnabled and remove PcdOpensslEcEnabled.
I want to make sure developers do not run into strange build failures if
they do not keep the 2 different PCDs aligned. I prefer a single PCD
setting to enable use of EC services.
I also noticed that the use of a PCD expression in an INF to select source
files does not work if the PCD value is specified with the --pcd flag on
the build command line. This looks like a significant bug with the PCD
expression in an INF file. This also needs to be fixed.
Mike
> -Original Message-
> From: Li, Yi1
> Sent: Thursday, September 22, 2022 8:02 PM
> To: Kinney, Michael D ; devel@edk2.groups.io
> Cc: Yao, Jiewen ; Wang, Jian J ;
> Lu, Xiaoyu1 ; Jiang,
> Guomin
> Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
>
> Hi Mike,
>
> 1. Yes, it matches.
> By Intel side, 100+kb(20%+) FV size increase will be a big concern, please
> refer to another internal email.
>
> 2. Additional size is coming from modules may consumed EC APIs, eg. TLS PEM
> X509 ...
>
> If we added EC source to OpensslLib.inf and disabled macro OPENSSL_NO_EC,
> those modules will link EC APIs and increase binary
> size,
> This an example from x509/x_pubkey.c , other modules is similar:
> #ifndef OPENSSL_NO_EC
> EC_KEY *d2i_EC_PUBKEY(EC_KEY **a, const unsigned char **pp, long length)
> {
> EVP_PKEY *pkey;
> EC_KEY *key = NULL;
> // call EC functions
> }
> #endif
>
> If we added EC source to OpensslLib.inf and enable macro OPENSSL_NO_EC, EC
> module will throw build error,
> Since some EC internal APIs or structs have been disabled by OPENSSL_NO_EC
> but not another.
> This an example from ec/ec_local.h , other error is similar:
>
> #ifndef OPENSSL_NO_EC
> typedef struct ec_group_st EC_GROUP;
> typedef struct ec_point_st EC_POINT;
> #endif
>
> // but this function not been enclosed by OPENSSL_NO_EC, and will throw build
> error
> int (*point_set_Jprojective_coordinates_GFp) (const EC_GROUP *,
> EC_POINT *, const BIGNUM *x,
> const BIGNUM *y,
> const BIGNUM *z, BN_CTX *);
>
> To avoid this annoying openssl error, we introduced conditional EC.
>
> Thanks,
> Yi
>
> -Original Message-
> From: Kinney, Michael D
> Sent: Friday, September 23, 2022 6:47 AM
> To: Li, Yi1 ; devel@edk2.groups.io; Kinney, Michael D
>
> Cc: Yao, Jiewen ; Wang, Jian J ;
> Lu, Xiaoyu1 ; Jiang,
> Guomin
> Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
>
> Hi Yi,
>
> I agree EC is an important feature.
>
> I did some analysis of the size impact to the CryptoPkg modules on current
> trunk
> with EC on and off. Uncompressed size is important for PEI Phase. For DXE and
> SMM phase, the Crypto services can always be compressed. From the table
> below,
> building all the EC services in the OpensslLib has no size impact to the NONE
> profile and the MIN_PEI profile. It has ~105 KB impact to compressed DXE/SMM
> usages that may use the MIN_DXE_MIN_SMM or ALL profiles.
>
>Uncompressed LZMA Compressed
> CPU CRYPTO_SERVICESModule EC=FALSE EC=TRUE EC=FALSE EC=TRUE
> Increase
> === === ===
>
> IA32 NONE CryptoPei2153621568 0
> KB
> IA32 NONE CryptoDxe2163221696 0
> KB
> IA32 NONE CryptoSmm2297623072 0
> KB
> IA32 MIN_PEI CryptoPei 248992 249120 0
> KB
> IA32 MIN_DXE_MIN_SMM CryptoDxe 636672 829568288520 401034113
> KB
> IA32 MIN_DXE_MIN_SMM CryptoSmm 426048 601472191517 296022105
> KB
> IA32 ALL CryptoPei 423840 598976189047 293759104
> KB
> IA32 ALL CryptoDxe 645280 838144292955 405277113
> KB
> IA32 ALL CryptoSmm 441888 617184198779 303628105
> KB
> X64 NONE CryptoPei2963229664 0
> KB
> X64 NONE CryptoDxe2979229792 0
> KB
> X64 NONE CryptoSmm3129631296 0
> KB
> X64 MIN_PEI CryptoPei 310784 310848 0
> KB
> X64 MIN_DXE_MI
Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Mike,
1. Yes, it matches.
By Intel side, 100+kb(20%+) FV size increase will be a big concern, please
refer to another internal email.
2. Additional size is coming from modules may consumed EC APIs, eg. TLS PEM
X509 ...
If we added EC source to OpensslLib.inf and disabled macro OPENSSL_NO_EC, those
modules will link EC APIs and increase binary size,
This an example from x509/x_pubkey.c , other modules is similar:
#ifndef OPENSSL_NO_EC
EC_KEY *d2i_EC_PUBKEY(EC_KEY **a, const unsigned char **pp, long length)
{
EVP_PKEY *pkey;
EC_KEY *key = NULL;
// call EC functions
}
#endif
If we added EC source to OpensslLib.inf and enable macro OPENSSL_NO_EC, EC
module will throw build error,
Since some EC internal APIs or structs have been disabled by OPENSSL_NO_EC but
not another.
This an example from ec/ec_local.h , other error is similar:
#ifndef OPENSSL_NO_EC
typedef struct ec_group_st EC_GROUP;
typedef struct ec_point_st EC_POINT;
#endif
// but this function not been enclosed by OPENSSL_NO_EC, and will throw build
error
int (*point_set_Jprojective_coordinates_GFp) (const EC_GROUP *,
EC_POINT *, const BIGNUM *x,
const BIGNUM *y,
const BIGNUM *z, BN_CTX *);
To avoid this annoying openssl error, we introduced conditional EC.
Thanks,
Yi
-Original Message-
From: Kinney, Michael D
Sent: Friday, September 23, 2022 6:47 AM
To: Li, Yi1 ; devel@edk2.groups.io; Kinney, Michael D
Cc: Yao, Jiewen ; Wang, Jian J ;
Lu, Xiaoyu1 ; Jiang, Guomin
Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Yi,
I agree EC is an important feature.
I did some analysis of the size impact to the CryptoPkg modules on current trunk
with EC on and off. Uncompressed size is important for PEI Phase. For DXE and
SMM phase, the Crypto services can always be compressed. From the table below,
building all the EC services in the OpensslLib has no size impact to the NONE
profile and the MIN_PEI profile. It has ~105 KB impact to compressed DXE/SMM
usages that may use the MIN_DXE_MIN_SMM or ALL profiles.
Uncompressed LZMA Compressed
CPU CRYPTO_SERVICESModule EC=FALSE EC=TRUE EC=FALSE EC=TRUE Increase
=== === ===
IA32 NONE CryptoPei2153621568 0 KB
IA32 NONE CryptoDxe2163221696 0 KB
IA32 NONE CryptoSmm2297623072 0 KB
IA32 MIN_PEI CryptoPei 248992 249120 0 KB
IA32 MIN_DXE_MIN_SMM CryptoDxe 636672 829568288520 401034113 KB
IA32 MIN_DXE_MIN_SMM CryptoSmm 426048 601472191517 296022105 KB
IA32 ALL CryptoPei 423840 598976189047 293759104 KB
IA32 ALL CryptoDxe 645280 838144292955 405277113 KB
IA32 ALL CryptoSmm 441888 617184198779 303628105 KB
X64 NONE CryptoPei2963229664 0 KB
X64 NONE CryptoDxe2979229792 0 KB
X64 NONE CryptoSmm3129631296 0 KB
X64 MIN_PEI CryptoPei 310784 310848 0 KB
X64 MIN_DXE_MIN_SMM CryptoDxe 804288 1016256311436 426596115 KB
X64 MIN_DXE_MIN_SMM CryptoSmm 543776 733920204483 310775106 KB
X64 ALL CryptoPei 540384 730240202494 308467106 KB
X64 ALL CryptoDxe 815392 1027296316228 431321115 KB
X64 ALL CryptoSmm 563648 753696213488 319644106 KB
NOTE: Even if multiple modules in an FV use static linking of Crypto libs, if
the
entire FV is compressed, the total size impact is typically the size of a
single instance of a compressed CryptoLib. The sizes of the Crypto*
modules
in the table above should be a close approximation of the size impact to a
single FV.
Does this match your previous size analysis?
The critical issue to evaluate here is why adding the EC sources to
OpensllLib.inf
causes the modules that do not use any EC services to grow by ~105KB. Has any
detailed analysis of the final linked images been performed to see where this
additional size is coming from?
Thanks,
Mike
> -Original Message-
> From: Li, Yi1
> Sent: Thursday, September 22, 2022 5:54 AM
> To: Kinney, Michael D ; devel@edk2.groups.io;
> Kishore, Shelly
> Cc: Yao, Jiewen ; Wang, Jian J ;
> Lu, Xiaoyu1 ; Jiang,
> Guomin
> Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
>
> Hi Mike,
> I have did some POC that seems existed structured PCD is hard to control
&
Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Yi, I agree EC is an important feature. I did some analysis of the size impact to the CryptoPkg modules on current trunk with EC on and off. Uncompressed size is important for PEI Phase. For DXE and SMM phase, the Crypto services can always be compressed. From the table below, building all the EC services in the OpensslLib has no size impact to the NONE profile and the MIN_PEI profile. It has ~105 KB impact to compressed DXE/SMM usages that may use the MIN_DXE_MIN_SMM or ALL profiles. Uncompressed LZMA Compressed CPU CRYPTO_SERVICESModule EC=FALSE EC=TRUE EC=FALSE EC=TRUE Increase === === === IA32 NONE CryptoPei2153621568 0 KB IA32 NONE CryptoDxe2163221696 0 KB IA32 NONE CryptoSmm2297623072 0 KB IA32 MIN_PEI CryptoPei 248992 249120 0 KB IA32 MIN_DXE_MIN_SMM CryptoDxe 636672 829568288520 401034113 KB IA32 MIN_DXE_MIN_SMM CryptoSmm 426048 601472191517 296022105 KB IA32 ALL CryptoPei 423840 598976189047 293759104 KB IA32 ALL CryptoDxe 645280 838144292955 405277113 KB IA32 ALL CryptoSmm 441888 617184198779 303628105 KB X64 NONE CryptoPei2963229664 0 KB X64 NONE CryptoDxe2979229792 0 KB X64 NONE CryptoSmm3129631296 0 KB X64 MIN_PEI CryptoPei 310784 310848 0 KB X64 MIN_DXE_MIN_SMM CryptoDxe 804288 1016256311436 426596115 KB X64 MIN_DXE_MIN_SMM CryptoSmm 543776 733920204483 310775106 KB X64 ALL CryptoPei 540384 730240202494 308467106 KB X64 ALL CryptoDxe 815392 1027296316228 431321115 KB X64 ALL CryptoSmm 563648 753696213488 319644106 KB NOTE: Even if multiple modules in an FV use static linking of Crypto libs, if the entire FV is compressed, the total size impact is typically the size of a single instance of a compressed CryptoLib. The sizes of the Crypto* modules in the table above should be a close approximation of the size impact to a single FV. Does this match your previous size analysis? The critical issue to evaluate here is why adding the EC sources to OpensllLib.inf causes the modules that do not use any EC services to grow by ~105KB. Has any detailed analysis of the final linked images been performed to see where this additional size is coming from? Thanks, Mike > -Original Message- > From: Li, Yi1 > Sent: Thursday, September 22, 2022 5:54 AM > To: Kinney, Michael D ; devel@edk2.groups.io; > Kishore, Shelly > Cc: Yao, Jiewen ; Wang, Jian J ; > Lu, Xiaoyu1 ; Jiang, > Guomin > Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > Hi Mike, > I have did some POC that seems existed structured PCD is hard to control > binary size, > Here is the previous discussion for reference. > https://bugzilla.tianocore.org/show_bug.cgi?id=3679 > https://edk2.groups.io/g/devel/topic/86257810#81814 > https://bugzilla.tianocore.org/show_bug.cgi?id=1446 > > Anyway EC is an important feature which consumed by vary modern security > features such WPA3 , SPDM, TLS1.3 etc. > Hope it can be added to edk2, and I am glad to take the code and test work if > there are other ways to control the size. > > Thanks, > Yi > > -Original Message- > From: Kinney, Michael D > Sent: Thursday, September 22, 2022 11:56 AM > To: Li, Yi1 ; devel@edk2.groups.io; Kishore, Shelly > > Cc: Yao, Jiewen ; Wang, Jian J ; > Lu, Xiaoyu1 ; Jiang, > Guomin > Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > That change to OpensslLib.inf should not have been done either. > > Looks like this EC feature needs more evaluation to fit into the > structured PCD control of the lib sizes. > > Mike > > > -Original Message- > > From: Li, Yi1 > > Sent: Wednesday, September 21, 2022 7:16 PM > > To: Kinney, Michael D ; devel@edk2.groups.io > > Cc: Yao, Jiewen ; Wang, Jian J > > ; Lu, Xiaoyu1 ; Jiang, > > Guomin > > Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > > > Hi Mike, > > Thanks for review. > > > > Even PCD_CRYPTO_SERVICE_FAMILY_ENABLE is set to 0, CryptoEc.c will also be > > compiled and throw build error: > > > > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): &g
Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Mike, I have did some POC that seems existed structured PCD is hard to control binary size, Here is the previous discussion for reference. https://bugzilla.tianocore.org/show_bug.cgi?id=3679 https://edk2.groups.io/g/devel/topic/86257810#81814 https://bugzilla.tianocore.org/show_bug.cgi?id=1446 Anyway EC is an important feature which consumed by vary modern security features such WPA3 , SPDM, TLS1.3 etc. Hope it can be added to edk2, and I am glad to take the code and test work if there are other ways to control the size. Thanks, Yi -Original Message- From: Kinney, Michael D Sent: Thursday, September 22, 2022 11:56 AM To: Li, Yi1 ; devel@edk2.groups.io; Kishore, Shelly Cc: Yao, Jiewen ; Wang, Jian J ; Lu, Xiaoyu1 ; Jiang, Guomin Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support That change to OpensslLib.inf should not have been done either. Looks like this EC feature needs more evaluation to fit into the structured PCD control of the lib sizes. Mike > -Original Message- > From: Li, Yi1 > Sent: Wednesday, September 21, 2022 7:16 PM > To: Kinney, Michael D ; devel@edk2.groups.io > Cc: Yao, Jiewen ; Wang, Jian J ; > Lu, Xiaoyu1 ; Jiang, > Guomin > Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > Hi Mike, > Thanks for review. > > Even PCD_CRYPTO_SERVICE_FAMILY_ENABLE is set to 0, CryptoEc.c will also be > compiled and throw build error: > > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): > error C2220: the following warning is treated as > an error > 1 file(s) copied. > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): > warning C4013: 'EC_GROUP_new_by_curve_name' > undefined; assuming extern returning int > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): > warning C4047: 'return': 'void *' differs in > levels of indirection from 'int' > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(105): > warning C4013: 'EC_GROUP_get_curve' undefined; > assuming extern returning int > > I think the root cause is that we have enabled conditional ec in > OpensslLib.inf before by PcdOpensslEcEnabled, > https://github.com/tianocore/edk2/blob/2c17d676e402d75a3a674499342f7ddaccf387bd/CryptoPkg/Library/OpensslLib/OpensslLib.inf#L2 > 02-L238 > if PcdOpensslEcEnabled not true, all ec files will not be compiled. > This will save 200+kb memory on platforms which use dxe driver but do not > need ec feature. > > So I add this PCD to BaseCryptLib.inf also to avoid build error, Not sure if > there is any other way, other better ideas are > welcome. > > Thanks, > Yi > > -Original Message- > From: Kinney, Michael D > Sent: Thursday, September 22, 2022 12:22 AM > To: devel@edk2.groups.io; Li, Yi1 ; Kinney, Michael D > > Cc: Yao, Jiewen ; Wang, Jian J ; > Lu, Xiaoyu1 ; Jiang, > Guomin > Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > Comments embedded below. > > Mike > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of yi1 li > > Sent: Tuesday, September 20, 2022 9:55 PM > > To: devel@edk2.groups.io > > Cc: Li, Yi1 ; Yao, Jiewen ; Wang, > > Jian J ; Lu, Xiaoyu1 > > ; Jiang, Guomin > > Subject: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3828 > > > > This patch is used to add CryptEc library, which is wrapped > > over OpenSSL. > > > > Cc: Jiewen Yao > > Cc: Jian J Wang > > Cc: Xiaoyu Lu > > Cc: Guomin Jiang > > > > Signed-off-by: Yi Li > > --- > > CryptoPkg/Include/Library/BaseCryptLib.h | 424 ++ > > .../Library/BaseCryptLib/BaseCryptLib.inf | 2 + > > .../Library/BaseCryptLib/PeiCryptLib.inf | 1 + > > CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c | 765 ++ > > .../Library/BaseCryptLib/Pk/CryptEcNull.c | 496 > > .../Library/BaseCryptLib/SmmCryptLib.inf | 1 + > > .../BaseCryptLibNull/BaseCryptLibNull.inf | 1 + > > .../Library/BaseCryptLibNull/Pk/CryptEcNull.c | 496 > > 8 files changed, 2186 insertions(+) > > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c > > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEcNull.c > > create mode 100644 CryptoPkg/Library/BaseCryptLibNull/Pk/CryptEcNull.c > > > > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > > b/CryptoPkg/Include/Library/BaseCryptLib.h > > index b253923dd8..d74
Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
That change to OpensslLib.inf should not have been done either. Looks like this EC feature needs more evaluation to fit into the structured PCD control of the lib sizes. Mike > -Original Message- > From: Li, Yi1 > Sent: Wednesday, September 21, 2022 7:16 PM > To: Kinney, Michael D ; devel@edk2.groups.io > Cc: Yao, Jiewen ; Wang, Jian J ; > Lu, Xiaoyu1 ; Jiang, > Guomin > Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > Hi Mike, > Thanks for review. > > Even PCD_CRYPTO_SERVICE_FAMILY_ENABLE is set to 0, CryptoEc.c will also be > compiled and throw build error: > > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): > error C2220: the following warning is treated as > an error > 1 file(s) copied. > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): > warning C4013: 'EC_GROUP_new_by_curve_name' > undefined; assuming extern returning int > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): > warning C4047: 'return': 'void *' differs in > levels of indirection from 'int' > d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(105): > warning C4013: 'EC_GROUP_get_curve' undefined; > assuming extern returning int > > I think the root cause is that we have enabled conditional ec in > OpensslLib.inf before by PcdOpensslEcEnabled, > https://github.com/tianocore/edk2/blob/2c17d676e402d75a3a674499342f7ddaccf387bd/CryptoPkg/Library/OpensslLib/OpensslLib.inf#L2 > 02-L238 > if PcdOpensslEcEnabled not true, all ec files will not be compiled. > This will save 200+kb memory on platforms which use dxe driver but do not > need ec feature. > > So I add this PCD to BaseCryptLib.inf also to avoid build error, Not sure if > there is any other way, other better ideas are > welcome. > > Thanks, > Yi > > -Original Message- > From: Kinney, Michael D > Sent: Thursday, September 22, 2022 12:22 AM > To: devel@edk2.groups.io; Li, Yi1 ; Kinney, Michael D > > Cc: Yao, Jiewen ; Wang, Jian J ; > Lu, Xiaoyu1 ; Jiang, > Guomin > Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > Comments embedded below. > > Mike > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of yi1 li > > Sent: Tuesday, September 20, 2022 9:55 PM > > To: devel@edk2.groups.io > > Cc: Li, Yi1 ; Yao, Jiewen ; Wang, > > Jian J ; Lu, Xiaoyu1 > > ; Jiang, Guomin > > Subject: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3828 > > > > This patch is used to add CryptEc library, which is wrapped > > over OpenSSL. > > > > Cc: Jiewen Yao > > Cc: Jian J Wang > > Cc: Xiaoyu Lu > > Cc: Guomin Jiang > > > > Signed-off-by: Yi Li > > --- > > CryptoPkg/Include/Library/BaseCryptLib.h | 424 ++ > > .../Library/BaseCryptLib/BaseCryptLib.inf | 2 + > > .../Library/BaseCryptLib/PeiCryptLib.inf | 1 + > > CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c | 765 ++ > > .../Library/BaseCryptLib/Pk/CryptEcNull.c | 496 > > .../Library/BaseCryptLib/SmmCryptLib.inf | 1 + > > .../BaseCryptLibNull/BaseCryptLibNull.inf | 1 + > > .../Library/BaseCryptLibNull/Pk/CryptEcNull.c | 496 > > 8 files changed, 2186 insertions(+) > > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c > > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEcNull.c > > create mode 100644 CryptoPkg/Library/BaseCryptLibNull/Pk/CryptEcNull.c > > > > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > > b/CryptoPkg/Include/Library/BaseCryptLib.h > > index b253923dd8..d74fc21c1e 100644 > > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h > > @@ -14,6 +14,13 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > #include > > > > +#define CRYPTO_NID_NULL 0x > > + > > +// Key Exchange > > +#define CRYPTO_NID_SECP256R1 0x0204 > > +#define CRYPTO_NID_SECP384R1 0x0205 > > +#define CRYPTO_NID_SECP521R1 0x0206 > > + > > /// > > /// MD5 digest size in bytes > > /// > > @@ -2850,4 +2857,421 @@ BigNumAddMod ( > >OUT VOID *BnRes > >); > > > > +// > > = > > +//Basic Elliptic Curve Primitives > > +// > > =
Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Hi Mike, Thanks for review. Even PCD_CRYPTO_SERVICE_FAMILY_ENABLE is set to 0, CryptoEc.c will also be compiled and throw build error: d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): error C2220: the following warning is treated as an error 1 file(s) copied. d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): warning C4013: 'EC_GROUP_new_by_curve_name' undefined; assuming extern returning int d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(77): warning C4047: 'return': 'void *' differs in levels of indirection from 'int' d:\workspace\tianocore\edk2\CryptoPkg\Library\BaseCryptLib\Pk\CryptEc.c(105): warning C4013: 'EC_GROUP_get_curve' undefined; assuming extern returning int I think the root cause is that we have enabled conditional ec in OpensslLib.inf before by PcdOpensslEcEnabled, https://github.com/tianocore/edk2/blob/2c17d676e402d75a3a674499342f7ddaccf387bd/CryptoPkg/Library/OpensslLib/OpensslLib.inf#L202-L238 if PcdOpensslEcEnabled not true, all ec files will not be compiled. This will save 200+kb memory on platforms which use dxe driver but do not need ec feature. So I add this PCD to BaseCryptLib.inf also to avoid build error, Not sure if there is any other way, other better ideas are welcome. Thanks, Yi -Original Message- From: Kinney, Michael D Sent: Thursday, September 22, 2022 12:22 AM To: devel@edk2.groups.io; Li, Yi1 ; Kinney, Michael D Cc: Yao, Jiewen ; Wang, Jian J ; Lu, Xiaoyu1 ; Jiang, Guomin Subject: RE: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support Comments embedded below. Mike > -Original Message- > From: devel@edk2.groups.io On Behalf Of yi1 li > Sent: Tuesday, September 20, 2022 9:55 PM > To: devel@edk2.groups.io > Cc: Li, Yi1 ; Yao, Jiewen ; Wang, > Jian J ; Lu, Xiaoyu1 > ; Jiang, Guomin > Subject: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3828 > > This patch is used to add CryptEc library, which is wrapped > over OpenSSL. > > Cc: Jiewen Yao > Cc: Jian J Wang > Cc: Xiaoyu Lu > Cc: Guomin Jiang > > Signed-off-by: Yi Li > --- > CryptoPkg/Include/Library/BaseCryptLib.h | 424 ++ > .../Library/BaseCryptLib/BaseCryptLib.inf | 2 + > .../Library/BaseCryptLib/PeiCryptLib.inf | 1 + > CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c | 765 ++ > .../Library/BaseCryptLib/Pk/CryptEcNull.c | 496 > .../Library/BaseCryptLib/SmmCryptLib.inf | 1 + > .../BaseCryptLibNull/BaseCryptLibNull.inf | 1 + > .../Library/BaseCryptLibNull/Pk/CryptEcNull.c | 496 > 8 files changed, 2186 insertions(+) > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEcNull.c > create mode 100644 CryptoPkg/Library/BaseCryptLibNull/Pk/CryptEcNull.c > > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > b/CryptoPkg/Include/Library/BaseCryptLib.h > index b253923dd8..d74fc21c1e 100644 > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h > @@ -14,6 +14,13 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include > > +#define CRYPTO_NID_NULL 0x > + > +// Key Exchange > +#define CRYPTO_NID_SECP256R1 0x0204 > +#define CRYPTO_NID_SECP384R1 0x0205 > +#define CRYPTO_NID_SECP521R1 0x0206 > + > /// > /// MD5 digest size in bytes > /// > @@ -2850,4 +2857,421 @@ BigNumAddMod ( >OUT VOID *BnRes >); > > +// > = > +//Basic Elliptic Curve Primitives > +// > = > + > +/** > + Initialize new opaque EcGroup object. This object represents an EC curve > and > + and is used for calculation within this group. This object should be freed > + using EcGroupFree() function. > + > + @param[in] CryptoNid Identifying number for the ECC curve (Defined in > + BaseCryptLib.h). > + > + @retval EcGroup object On success. > + @retval NULLOn failure. > +**/ > +VOID * > +EFIAPI > +EcGroupInit ( > + IN UINTN CryptoNid > + ); > + > +/** > + Get EC curve parameters. While elliptic curve equation is Y^2 mod P = (X^3 > + AX + B) Mod P. > + This function will set the provided Big Number objects to the > corresponding > + values. The caller needs to make sure all the "out" BigNumber parameters > + are properly initialized. > + > + @param[in] EcGroupEC group
Re: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support
Comments embedded below. Mike > -Original Message- > From: devel@edk2.groups.io On Behalf Of yi1 li > Sent: Tuesday, September 20, 2022 9:55 PM > To: devel@edk2.groups.io > Cc: Li, Yi1 ; Yao, Jiewen ; Wang, > Jian J ; Lu, Xiaoyu1 > ; Jiang, Guomin > Subject: [edk2-devel] [PATCH V2 1/3] CryptoPkg: Add EC support > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3828 > > This patch is used to add CryptEc library, which is wrapped > over OpenSSL. > > Cc: Jiewen Yao > Cc: Jian J Wang > Cc: Xiaoyu Lu > Cc: Guomin Jiang > > Signed-off-by: Yi Li > --- > CryptoPkg/Include/Library/BaseCryptLib.h | 424 ++ > .../Library/BaseCryptLib/BaseCryptLib.inf | 2 + > .../Library/BaseCryptLib/PeiCryptLib.inf | 1 + > CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c | 765 ++ > .../Library/BaseCryptLib/Pk/CryptEcNull.c | 496 > .../Library/BaseCryptLib/SmmCryptLib.inf | 1 + > .../BaseCryptLibNull/BaseCryptLibNull.inf | 1 + > .../Library/BaseCryptLibNull/Pk/CryptEcNull.c | 496 > 8 files changed, 2186 insertions(+) > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c > create mode 100644 CryptoPkg/Library/BaseCryptLib/Pk/CryptEcNull.c > create mode 100644 CryptoPkg/Library/BaseCryptLibNull/Pk/CryptEcNull.c > > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > b/CryptoPkg/Include/Library/BaseCryptLib.h > index b253923dd8..d74fc21c1e 100644 > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > +++ b/CryptoPkg/Include/Library/BaseCryptLib.h > @@ -14,6 +14,13 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include > > +#define CRYPTO_NID_NULL 0x > + > +// Key Exchange > +#define CRYPTO_NID_SECP256R1 0x0204 > +#define CRYPTO_NID_SECP384R1 0x0205 > +#define CRYPTO_NID_SECP521R1 0x0206 > + > /// > /// MD5 digest size in bytes > /// > @@ -2850,4 +2857,421 @@ BigNumAddMod ( >OUT VOID *BnRes >); > > +// > = > +//Basic Elliptic Curve Primitives > +// > = > + > +/** > + Initialize new opaque EcGroup object. This object represents an EC curve > and > + and is used for calculation within this group. This object should be freed > + using EcGroupFree() function. > + > + @param[in] CryptoNid Identifying number for the ECC curve (Defined in > + BaseCryptLib.h). > + > + @retval EcGroup object On success. > + @retval NULLOn failure. > +**/ > +VOID * > +EFIAPI > +EcGroupInit ( > + IN UINTN CryptoNid > + ); > + > +/** > + Get EC curve parameters. While elliptic curve equation is Y^2 mod P = (X^3 > + AX + B) Mod P. > + This function will set the provided Big Number objects to the > corresponding > + values. The caller needs to make sure all the "out" BigNumber parameters > + are properly initialized. > + > + @param[in] EcGroupEC group object. > + @param[out] BnPrimeGroup prime number. > + @param[out] BnAA coefficient. > + @param[out] BnBB coefficient. > + @param[in] BnCtx BN context. > + > + @retval TRUE On success. > + @retval FALSE Otherwise. > +**/ > +BOOLEAN > +EFIAPI > +EcGroupGetCurve ( > + IN CONST VOID *EcGroup, > + OUT VOID *BnPrime, > + OUT VOID *BnA, > + OUT VOID *BnB, > + IN VOID*BnCtx > + ); > + > +/** > + Get EC group order. > + This function will set the provided Big Number object to the corresponding > + value. The caller needs to make sure that the "out" BigNumber parameter > + is properly initialized. > + > + @param[in] EcGroup EC group object. > + @param[out] BnOrder Group prime number. > + > + @retval TRUE On success. > + @retval FALSE Otherwise. > +**/ > +BOOLEAN > +EFIAPI > +EcGroupGetOrder ( > + IN VOID *EcGroup, > + OUT VOID *BnOrder > + ); > + > +/** > + Free previously allocated EC group object using EcGroupInit(). > + > + @param[in] EcGroup EC group object to free. > +**/ > +VOID > +EFIAPI > +EcGroupFree ( > + IN VOID *EcGroup > + ); > + > +/** > + Initialize new opaque EC Point object. This object represents an EC point > + within the given EC group (curve). > + > + @param[in] EC Group, properly initialized using EcGroupInit(). > + > + @retval EC Point object On success. > + @retval NULL On failure. > +**/ > +VOID * > +EFIAPI > +EcPointInit ( > + IN CONST VOID *EcGroup > + ); > + > +/** > + Free previously allocated EC Point object using EcPointInit(). > + > + @param[in] EcPoint EC Point to free. > + @param[in] Clear TRUE iff the memory should be cleared. > +**/ > +VOID > +EFIAPI > +EcPointDeInit ( > + IN VOID *EcPoint, > + IN BOOLEAN Clear > + ); > + > +/** > + Get EC point affine (x,y) coordinates. > + This function will set the provided Big Number objects
