CryptoPkg/CryptoPkg.dec | 4 - OvmfPkg/OvmfPkgIa32.fdf | 6 +- OvmfPkg/OvmfPkgIa32X64.fdf | 6 +- OvmfPkg/OvmfPkgX64.fdf | 6 +- .../Library/BaseCryptLib/BaseCryptLib.inf | 3 - .../Library/BaseCryptLib/PeiCryptLib.inf | 3 - .../Library/BaseCryptLib/RuntimeCryptLib.inf | 3 - .../Library/BaseCryptLib/SmmCryptLib.inf | 3 - .../BaseCryptLib/UnitTestHostBaseCryptLib.inf | 3 - CryptoPkg/Library/OpensslLib/OpensslLib.inf | 99 ++++---- .../Library/OpensslLib/OpensslLibCrypto.inf | 99 ++++---- CryptoPkg/Library/TlsLib/TlsLib.inf | 3 - CryptoPkg/Library/Include/crypto/dso_conf.h | 7 +- .../Library/Include/openssl/opensslconf.h | 240 ++++++++---------- CryptoPkg/CryptoPkg.ci.yaml | 10 + 15 files changed, 234 insertions(+), 261 deletions(-)
Re-opening the elliptic curves debate after running into the recent openssl changes. The current implementation is IMHO rather messy. It adds manual changes to a auto-generated files, which will make any updates a rather hard and error-prone process. I see two possible options how we can move forward: (1) Drop the idea to make EC configurable and just enable it unconditionally. I think long-term there is no way around this anyway as EC is a hard requirement for TLS 1.3. (2) Keep the EC config option, but update process_files.pl to automatically add the PcdEcEnabled config option handling to the files it generates. This patch set does (1). It also tweaks ovmf firmware volumes to make CI tests pass and it also excludes generated files from codestyle checks. take care, Gerd Gerd Hoffmann (5): Revert "CryptoPkg: Declare PcdEcEnabled in Library consuming OpensslLib" Revert "CryptoPkg: Make EC source file config-able" OvmfPkg: make DXEFV larger CryptoPkg/openssl: update generated files CryptoPkg/openssl: disable codestyle checks for generated files CryptoPkg/CryptoPkg.dec | 4 - OvmfPkg/OvmfPkgIa32.fdf | 6 +- OvmfPkg/OvmfPkgIa32X64.fdf | 6 +- OvmfPkg/OvmfPkgX64.fdf | 6 +- .../Library/BaseCryptLib/BaseCryptLib.inf | 3 - .../Library/BaseCryptLib/PeiCryptLib.inf | 3 - .../Library/BaseCryptLib/RuntimeCryptLib.inf | 3 - .../Library/BaseCryptLib/SmmCryptLib.inf | 3 - .../BaseCryptLib/UnitTestHostBaseCryptLib.inf | 3 - CryptoPkg/Library/OpensslLib/OpensslLib.inf | 99 ++++---- .../Library/OpensslLib/OpensslLibCrypto.inf | 99 ++++---- CryptoPkg/Library/TlsLib/TlsLib.inf | 3 - CryptoPkg/Library/Include/crypto/dso_conf.h | 7 +- .../Library/Include/openssl/opensslconf.h | 240 ++++++++---------- CryptoPkg/CryptoPkg.ci.yaml | 10 + 15 files changed, 234 insertions(+), 261 deletions(-) -- 2.35.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89457): https://edk2.groups.io/g/devel/message/89457 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Gerd Thanks for the patch. Some initial thought: I have no concern on OVMF package update. We can update if we want. However, I do have concern for crypto package to enable ECC *unconditionally*. I am not convinced that "EC is hard requirement for EDKII" just because "EC is a hard requirement for TLS 1.3". My reason below: A) TLS1.3 is only for DXE, but enabling ECC unconditionally may impact PEI/DXE. (Unless size of PEI/SMM is unchanged). B) TLS1.3 is only for special feature such as HTTPS boot, WIFI TLS-EAP. But not all platform requires HTTPS boot or WIFI TLS-EAP. C) TLS1.3 is not a mandatory requirement. TLS1.2 can still be used. It would be great if you can consider the option 2) below. I am in holiday now. And I am starting collecting feedback from Intel platform BIOS team. I will give official feedback after 1 week. Thank you Yao Jiewen > -----Original Message----- > From: Gerd Hoffmann <kraxel@redhat.com> > Sent: Monday, May 2, 2022 6:35 PM > To: devel@edk2.groups.io > Cc: Pawel Polawski <ppolawsk@redhat.com>; Li, Yi1 <yi1.li@intel.com>; Yao, > Jiewen <jiewen.yao@intel.com>; Oliver Steffen <osteffen@redhat.com>; Wang, > Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; > Jiang, Guomin <guomin.jiang@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; > Justen, Jordan L <jordan.l.justen@intel.com>; Gerd Hoffmann > <kraxel@redhat.com> > Subject: [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally. > > Re-opening the elliptic curves debate after running into the recent > openssl changes. The current implementation is IMHO rather messy. > It adds manual changes to a auto-generated files, which will make > any updates a rather hard and error-prone process. > > I see two possible options how we can move forward: > > (1) Drop the idea to make EC configurable and just enable it > unconditionally. I think long-term there is no way around > this anyway as EC is a hard requirement for TLS 1.3. > (2) Keep the EC config option, but update process_files.pl to > automatically add the PcdEcEnabled config option handling > to the files it generates. > > This patch set does (1). It also tweaks ovmf firmware volumes > to make CI tests pass and it also excludes generated files from > codestyle checks. > > take care, > Gerd > > Gerd Hoffmann (5): > Revert "CryptoPkg: Declare PcdEcEnabled in Library consuming > OpensslLib" > Revert "CryptoPkg: Make EC source file config-able" > OvmfPkg: make DXEFV larger > CryptoPkg/openssl: update generated files > CryptoPkg/openssl: disable codestyle checks for generated files > > CryptoPkg/CryptoPkg.dec | 4 - > OvmfPkg/OvmfPkgIa32.fdf | 6 +- > OvmfPkg/OvmfPkgIa32X64.fdf | 6 +- > OvmfPkg/OvmfPkgX64.fdf | 6 +- > .../Library/BaseCryptLib/BaseCryptLib.inf | 3 - > .../Library/BaseCryptLib/PeiCryptLib.inf | 3 - > .../Library/BaseCryptLib/RuntimeCryptLib.inf | 3 - > .../Library/BaseCryptLib/SmmCryptLib.inf | 3 - > .../BaseCryptLib/UnitTestHostBaseCryptLib.inf | 3 - > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 99 ++++---- > .../Library/OpensslLib/OpensslLibCrypto.inf | 99 ++++---- > CryptoPkg/Library/TlsLib/TlsLib.inf | 3 - > CryptoPkg/Library/Include/crypto/dso_conf.h | 7 +- > .../Library/Include/openssl/opensslconf.h | 240 ++++++++---------- > CryptoPkg/CryptoPkg.ci.yaml | 10 + > 15 files changed, 234 insertions(+), 261 deletions(-) > > -- > 2.35.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89483): https://edk2.groups.io/g/devel/message/89483 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi, > However, I do have concern for crypto package to enable ECC *unconditionally*. > I am not convinced that "EC is hard requirement for EDKII" just because "EC is a hard requirement for TLS 1.3". My reason below: > A) TLS1.3 is only for DXE, but enabling ECC unconditionally may impact PEI/DXE. (Unless size of PEI/SMM is unchanged). Well, the PcdEcEnabled switch we have in the tree right now enables or disables EC for everybody, it doesn't support enabling EC for DXE only. In we want change that we'll need two different *.inf files I guess, one for openssl with ec and one for openssl without ec. I'll check the effect on image sizes. > C) TLS1.3 is not a mandatory requirement. TLS1.2 can still be used. Yes, today this isn't much of a problem. But I expect that will change in the future as browsers fade out support for older TLS versions to improve security. Recent firefox versions have TLS 1.0 and 1.1 disabled by default. So while this isn't urgent it is still something we should consider and keep on our radar. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89528): https://edk2.groups.io/g/devel/message/89528 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi, > > I am not convinced that "EC is hard requirement for EDKII" just because "EC is a hard requirement for TLS 1.3". My reason below: > > A) TLS1.3 is only for DXE, but enabling ECC unconditionally may impact PEI/DXE. (Unless size of PEI/SMM is unchanged). > > Well, the PcdEcEnabled switch we have in the tree right now enables or > disables EC for everybody, it doesn't support enabling EC for DXE only. > > In we want change that we'll need two different *.inf files I guess, > one for openssl with ec and one for openssl without ec. > > I'll check the effect on image sizes. Here we go: --- master.stats 2022-05-05 10:05:03.791368600 +0200 +++ openssl-ec.stats 2022-05-05 10:35:44.429412053 +0200 @@ -137,8 +137,8 @@ 124410 BdsDxe 145534 DxeCore 148078 UiApp - 400158 SecureBootConfigDxe - 472950 SecurityStubDxe - 532626 VariableSmm - 658174 TlsDxe + 575390 SecureBootConfigDxe + 643062 SecurityStubDxe + 700562 VariableSmm + 847422 TlsDxe 946646 Shell So no effect on PEI size but SMM is affected. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89531): https://edk2.groups.io/g/devel/message/89531 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Thank you Gerd. I collected feedback from Intel BIOS team, both client and server, both old platform and new platform. In general, the new platform will leave enough space for crypto improvement. Size is not a big issue. The delta is acceptable. However, the old launched platforms only has limited flash space. This patch will break the current build because of size increase. Option (1) is not acceptable. In conclusion: For OvmfPkg update: Acked-by: Jiewen Yao <Jiewen.yao@intel.com> For SecurityPkg update: I recommend we consider option (2). (1) Drop the idea to make EC configurable and just enable it unconditionally. I think long-term there is no way around this anyway as EC is a hard requirement for TLS 1.3. (2) Keep the EC config option, but update process_files.pl to automatically add the PcdEcEnabled config option handling to the files it generates. Thank you Yao Jiewen > -----Original Message----- > From: Gerd Hoffmann <kraxel@redhat.com> > Sent: Thursday, May 5, 2022 5:16 PM > To: devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Pawel Polawski > <ppolawsk@redhat.com>; Li, Yi1 <yi1.li@intel.com>; Oliver Steffen > <osteffen@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard Biesheuvel > <ardb+tianocore@kernel.org>; Jiang, Guomin <guomin.jiang@intel.com>; Lu, > Xiaoyu1 <xiaoyu1.lu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com> > Subject: Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC > unconditionally. > > Hi, > > > > I am not convinced that "EC is hard requirement for EDKII" just because "EC > is a hard requirement for TLS 1.3". My reason below: > > > A) TLS1.3 is only for DXE, but enabling ECC unconditionally may impact > PEI/DXE. (Unless size of PEI/SMM is unchanged). > > > > Well, the PcdEcEnabled switch we have in the tree right now enables or > > disables EC for everybody, it doesn't support enabling EC for DXE only. > > > > In we want change that we'll need two different *.inf files I guess, > > one for openssl with ec and one for openssl without ec. > > > > I'll check the effect on image sizes. > > Here we go: > > --- master.stats 2022-05-05 10:05:03.791368600 +0200 > +++ openssl-ec.stats 2022-05-05 10:35:44.429412053 +0200 > @@ -137,8 +137,8 @@ > 124410 BdsDxe > 145534 DxeCore > 148078 UiApp > - 400158 SecureBootConfigDxe > - 472950 SecurityStubDxe > - 532626 VariableSmm > - 658174 TlsDxe > + 575390 SecureBootConfigDxe > + 643062 SecurityStubDxe > + 700562 VariableSmm > + 847422 TlsDxe > 946646 Shell > > So no effect on PEI size but SMM is affected. > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89589): https://edk2.groups.io/g/devel/message/89589 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, May 09, 2022 at 01:38:35AM +0000, Yao, Jiewen wrote: > Thank you Gerd. > > I collected feedback from Intel BIOS team, both client and server, both old platform and new platform. > > In general, the new platform will leave enough space for crypto improvement. Size is not a big issue. The delta is acceptable. > > However, the old launched platforms only has limited flash space. This patch will break the current build because of size increase. Option (1) is not acceptable. Hmm. Does that mean the old platform (what is "old" here btw?) wouldn't be able to do the switch to openssl3 either? take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89602): https://edk2.groups.io/g/devel/message/89602 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Old == the launched platform, or the platform will be launched shortly where the flash size and layout are locked. It is huge risk to change the layout suddenly. And it is not practical to change the flash size. (E.g. How can you change your flash size on your laptop? ) New platform usually does not have such constrain, because it may include new feature and have more size, and the layout can be tuned later. Talking about OPENSSL3.0. First, I support the OPENSSL 3.0 enabling plan, because we should do that before OPENSSL 1.1 end of support. You did a great job to enable OPENSSL3.0 in https://github.com/kraxel/edk2/tree/openssl3. I do appreciate that effort. However, we also have size concern on OPENSSL3.0, according to the data you provided. If we switch OPENSSL 1.1 to OPENSSL 3.0 immediately, then many platforms will be broken due to size issue. It is not practical. I would recommend in this way: 1) Please keep the good work to enable OPENSSL3.0 in your personal branch. 2) If you have some way to control the size, then do it. If there is no much size difference by default, then you can submit to EDKII directly. 3) If there is significant size difference, we need figure out a way to resolve it. As temporary step, you may choose post OPENSSL3.0 to https://github.com/tianocore/edk2-staging, which is an official location for broader evaluation, collaboration and enhancement. 4) As enhancement, the basic idea is to make the library configurable. As such, if the old platform does not new functionality, it can still live with OPENSSL3.0. The line is : same feature ==> same size (or minor reasonable increase), new feature ==> more size. Thank you Yao Jiewen > -----Original Message----- > From: Gerd Hoffmann <kraxel@redhat.com> > Sent: Monday, May 9, 2022 5:45 PM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com> > Cc: Pawel Polawski <ppolawsk@redhat.com>; Li, Yi1 <yi1.li@intel.com>; Oliver > Steffen <osteffen@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard > Biesheuvel <ardb+tianocore@kernel.org>; Jiang, Guomin > <guomin.jiang@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Justen, Jordan > L <jordan.l.justen@intel.com> > Subject: Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC > unconditionally. > > On Mon, May 09, 2022 at 01:38:35AM +0000, Yao, Jiewen wrote: > > Thank you Gerd. > > > > I collected feedback from Intel BIOS team, both client and server, both old > platform and new platform. > > > > In general, the new platform will leave enough space for crypto improvement. > Size is not a big issue. The delta is acceptable. > > > > However, the old launched platforms only has limited flash space. This patch > will break the current build because of size increase. Option (1) is not acceptable. > > Hmm. Does that mean the old platform (what is "old" here btw?) wouldn't > be able to do the switch to openssl3 either? > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89607): https://edk2.groups.io/g/devel/message/89607 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi, > Old == the launched platform, or the platform will be launched shortly > where the flash size and layout are locked. So everything you can buy today. > It is huge risk to change the layout suddenly. And it is not practical > to change the flash size. (E.g. How can you change your flash size on > your laptop? ) Sure. > However, we also have size concern on OPENSSL3.0, according to the data you provided. Yes. > 1) Please keep the good work to enable OPENSSL3.0 in your personal branch. > 2) If you have some way to control the size, then do it. If there is > no much size difference by default, then you can submit to EDKII > directly. I suspect I wouldn't get it down to 1.1.1 levels even if I find some ways to make it smaller than it is in my branch today. The code for the new "provider" concept simply needs space and I think it also makes LTO optimization less effective. Maybe creating our own crypto providers which include only the algorithms actually needed by edk2 gets the size down a bit. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89609): https://edk2.groups.io/g/devel/message/89609 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 2022-05-09 at 13:27 +0200, Gerd Hoffmann wrote: [...] > > 1) Please keep the good work to enable OPENSSL3.0 in your personal > > branch. > > 2) If you have some way to control the size, then do it. If there > > is no much size difference by default, then you can submit to EDKII > > directly. > > I suspect I wouldn't get it down to 1.1.1 levels even if I find some > ways to make it smaller than it is in my branch today. The code for > the new "provider" concept simply needs space and I think it also > makes LTO optimization less effective. Having just looked into converting engine code to provider code, I would concur with this. The design of providers, with their many to many functional mappings, seems designed to promote code bloat. > Maybe creating our own crypto providers which include only the > algorithms actually needed by edk2 gets the size down a bit. What about switching to a different crypto backend? Since we don't expose any openssl APIs at all and we wrapper everything we do expose, it should be possible to switch to one of the non-openssl (or forked from openssl) variants that value size, like mbedtls or boringssl? James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89610): https://edk2.groups.io/g/devel/message/89610 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
It is possible to switch to other crypt lib. For example, the *mbedtls* version POC can be found at https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg The advantage is: the size is much smaller. The disadvantage is: some required functions are not available, such as PKCS7. Thank you Yao Jiewen > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of James > Bottomley > Sent: Monday, May 9, 2022 7:48 PM > To: devel@edk2.groups.io; kraxel@redhat.com; Yao, Jiewen > <jiewen.yao@intel.com> > Cc: Pawel Polawski <ppolawsk@redhat.com>; Li, Yi1 <yi1.li@intel.com>; Oliver > Steffen <osteffen@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard > Biesheuvel <ardb+tianocore@kernel.org>; Jiang, Guomin > <guomin.jiang@intel.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Justen, Jordan > L <jordan.l.justen@intel.com> > Subject: Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC > unconditionally. > > On Mon, 2022-05-09 at 13:27 +0200, Gerd Hoffmann wrote: > [...] > > > 1) Please keep the good work to enable OPENSSL3.0 in your personal > > > branch. > > > 2) If you have some way to control the size, then do it. If there > > > is no much size difference by default, then you can submit to EDKII > > > directly. > > > > I suspect I wouldn't get it down to 1.1.1 levels even if I find some > > ways to make it smaller than it is in my branch today. The code for > > the new "provider" concept simply needs space and I think it also > > makes LTO optimization less effective. > > Having just looked into converting engine code to provider code, I > would concur with this. The design of providers, with their many to > many functional mappings, seems designed to promote code bloat. > > > Maybe creating our own crypto providers which include only the > > algorithms actually needed by edk2 gets the size down a bit. > > What about switching to a different crypto backend? Since we don't > expose any openssl APIs at all and we wrapper everything we do expose, > it should be possible to switch to one of the non-openssl (or forked > from openssl) variants that value size, like mbedtls or boringssl? > > James > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89612): https://edk2.groups.io/g/devel/message/89612 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 2022-05-09 at 12:03 +0000, Yao, Jiewen wrote: > It is possible to switch to other crypt lib. > > For example, the *mbedtls* version POC can be found at > https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg > The advantage is: the size is much smaller. > The disadvantage is: some required functions are not available, such > as PKCS7. Perhaps as a first step, we should look at our options. I would say missing functionality is problematic, but not necessarily a killer: we'd have to help the chosen project develop the capability and figure out how to maintain the fork while it was going upstream. PKCS#7 is pretty huge, though, it's the entire Cryptographic Message Syntax so I think us having to develop that for mbedtls makes that one a non starter. Other libraries could be: wolfssl gnutls boringssl LibreSSL They all seem to do pkcs#7. James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89615): https://edk2.groups.io/g/devel/message/89615 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, May 09, 2022 at 09:41:02AM -0400, James Bottomley wrote: > On Mon, 2022-05-09 at 12:03 +0000, Yao, Jiewen wrote: > > It is possible to switch to other crypt lib. > > > > For example, the *mbedtls* version POC can be found at > > https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg > > The advantage is: the size is much smaller. > > The disadvantage is: some required functions are not available, such > > as PKCS7. > > Perhaps as a first step, we should look at our options. I would say > missing functionality is problematic, but not necessarily a killer: > we'd have to help the chosen project develop the capability and figure > out how to maintain the fork while it was going upstream. I don't feel like entering the business of maintaining a tls library ... > Other libraries could be: > > wolfssl Hmm? Apparently no git repository? > gnutls Might be a issue license-wise. > boringssl Looks like an option worth investigating. The "designed to meet Google's needs" and "not intended for general use" notes in the toplevel README don't look that great though. Might turn out to be be difficult to get changes needed for edk2 merged (hasn't been a problem so far for me with openssl). > LibreSSL There was some hype around it after it was forked from openssl in the heartbleed aftermath. More recent news are less enthusiastic: https://lwn.net/Articles/841664/ Another possible option would be to add openssl3 as alternative OpensslLib implementation, so platforms can pick the one or the other depending on size constrains. I've also experimented a bit with CryptoPkg/Driver. It's not a clear win, at least for OVMF. PEI FV is larger in any case. Seems LTO works very well for the few hashes needed by TPM support code, and so the overhead added by using the crypto service protocol instead of direct linking is much larger than the savings by sharing code. DXE FV is smaller in the builds with secure boot and smm support, seems with the large tls codebase included we have enough wins by sharing the crypto code then, so the protocol overhead is worth the effort. I'm wondering where the crypto algorithm selection in CryptoPkg/CryptoPkg.dsc comes from though, specifically for MIN_DXE_MIN_SMM. Why is the crypto feature selection identical for DXE and SMM? Specifically why TLS is enabled for SMM? take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89651): https://edk2.groups.io/g/devel/message/89651 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 2022-05-10 at 12:40 +0200, Gerd Hoffmann wrote: > On Mon, May 09, 2022 at 09:41:02AM -0400, James Bottomley wrote: > > On Mon, 2022-05-09 at 12:03 +0000, Yao, Jiewen wrote: > > > It is possible to switch to other crypt lib. > > > > > > For example, the *mbedtls* version POC can be found at > > > https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg > > > The advantage is: the size is much smaller. > > > The disadvantage is: some required functions are not available, > > > such as PKCS7. > > > > Perhaps as a first step, we should look at our options. I would > > say missing functionality is problematic, but not necessarily a > > killer: we'd have to help the chosen project develop the capability > > and figure out how to maintain the fork while it was going > > upstream. > > I don't feel like entering the business of maintaining a tls > library ... Me neither, but we already maintain some exceptions like the logic to break the X509 chain for UEFI, so if we had to tinker around the edges, I think it's feasible. > > Other libraries could be: > > > > wolfssl > > Hmm? Apparently no git repository? https://github.com/wolfSSL/wolfssl > > gnutls > > Might be a issue license-wise. It's LGPL and our use case entirely embeds it so we're using it within the licence terms. Since we're effectively linking statically, it provides a slight problem for distributions because they need to facilitate relinking, but that's just a nasty mechanical problem > > > boringssl > > Looks like an option worth investigating. > > The "designed to meet Google's needs" and "not intended for general > use" notes in the toplevel README don't look that great > though. Might turnons out to be be difficult to get changes needed > for edk2 merged (hasn't been a problem so far for me with openssl). Right, boringssl is effectively Google's fork of openssl for android which they did because they could never get the openssl people to accept their patches or pay attention to the embedded bloat problem (which is currently our problem). > > LibreSSL > > There was some hype around it after it was forked from openssl in the > heartbleed aftermath. More recent news are less enthusiastic: > https://lwn.net/Articles/841664/ Yes, I'm not hugely enthused about LibreSSL, but I think we do need to list all the alternatives. > Another possible option would be to add openssl3 as alternative > OpensslLib implementation, so platforms can pick the one or the > other depending on size constrains. Really, no, we can't. That would leave the space constrained use case non functional when openssl 1 goes EOL. We have to make openssl 3 work for everything or consider a new crypto provider. > I've also experimented a bit with CryptoPkg/Driver. It's not a > clear win, at least for OVMF. > > PEI FV is larger in any case. Seems LTO works very well for the > few hashes needed by TPM support code, and so the overhead added > by using the crypto service protocol instead of direct linking is > much larger than the savings by sharing code. > > DXE FV is smaller in the builds with secure boot and smm support, > seems with the large tls codebase included we have enough wins by > sharing the crypto code then, so the protocol overhead is worth > the effort. > > I'm wondering where the crypto algorithm selection in > CryptoPkg/CryptoPkg.dsc comes from though, specifically for > MIN_DXE_MIN_SMM. Why is the crypto feature selection identical > for DXE and SMM? Specifically why TLS is enabled for SMM? I think the idea was that using a static openssl library you could link the various algorithm providers with it and make small pieces, but that didn't work out well for openssl which has a massive startup requirement. No idea why SMM would require TLS ... I can look at the code. James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89662): https://edk2.groups.io/g/devel/message/89662 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> I'm wondering where the crypto algorithm selection in > CryptoPkg/CryptoPkg.dsc comes from though, specifically for > MIN_DXE_MIN_SMM. Why is the crypto feature selection identical > for DXE and SMM? Specifically why TLS is enabled for SMM? [Jiewen] So far, I don't know if any SMM feature requires TLS. I guess we may win the flash size by creating identical binary for CryptoDxe and CryptoSmm *with compression*. But I don't have data and I am not sure. Just guess. You may have a try to remove TLS for SMM and check the final compressed FV size. > -----Original Message----- > From: kraxel@redhat.com <kraxel@redhat.com> > Sent: Tuesday, May 10, 2022 6:40 PM > To: James Bottomley <James.Bottomley@hansenpartnership.com> > Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Pawel > Polawski <ppolawsk@redhat.com>; Li, Yi1 <yi1.li@intel.com>; Oliver Steffen > <osteffen@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard Biesheuvel > <ardb+tianocore@kernel.org>; Jiang, Guomin <guomin.jiang@intel.com>; Lu, > Xiaoyu1 <xiaoyu1.lu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com> > Subject: Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC > unconditionally. > > On Mon, May 09, 2022 at 09:41:02AM -0400, James Bottomley wrote: > > On Mon, 2022-05-09 at 12:03 +0000, Yao, Jiewen wrote: > > > It is possible to switch to other crypt lib. > > > > > > For example, the *mbedtls* version POC can be found at > > > https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg > > > The advantage is: the size is much smaller. > > > The disadvantage is: some required functions are not available, such > > > as PKCS7. > > > > Perhaps as a first step, we should look at our options. I would say > > missing functionality is problematic, but not necessarily a killer: > > we'd have to help the chosen project develop the capability and figure > > out how to maintain the fork while it was going upstream. > > I don't feel like entering the business of maintaining a tls > library ... > > > Other libraries could be: > > > > wolfssl > > Hmm? Apparently no git repository? > > > gnutls > > Might be a issue license-wise. > > > boringssl > > Looks like an option worth investigating. > > The "designed to meet Google's needs" and "not intended for general use" > notes in the toplevel README don't look that great though. Might turn > out to be be difficult to get changes needed for edk2 merged (hasn't > been a problem so far for me with openssl). > > > LibreSSL > > There was some hype around it after it was forked from openssl in the > heartbleed aftermath. More recent news are less enthusiastic: > https://lwn.net/Articles/841664/ > > Another possible option would be to add openssl3 as alternative > OpensslLib implementation, so platforms can pick the one or the > other depending on size constrains. > > > I've also experimented a bit with CryptoPkg/Driver. It's not a > clear win, at least for OVMF. > > PEI FV is larger in any case. Seems LTO works very well for the > few hashes needed by TPM support code, and so the overhead added > by using the crypto service protocol instead of direct linking is > much larger than the savings by sharing code. > > DXE FV is smaller in the builds with secure boot and smm support, > seems with the large tls codebase included we have enough wins by > sharing the crypto code then, so the protocol overhead is worth > the effort. > > I'm wondering where the crypto algorithm selection in > CryptoPkg/CryptoPkg.dsc comes from though, specifically for > MIN_DXE_MIN_SMM. Why is the crypto feature selection identical > for DXE and SMM? Specifically why TLS is enabled for SMM? > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89653): https://edk2.groups.io/g/devel/message/89653 Mute This Topic: https://groups.io/mt/90832153/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.