[edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.

Gerd Hoffmann posted 5 patches 1 year, 11 months ago
Failed in applying to current master (apply log)
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(-)
[edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Gerd Hoffmann 1 year, 11 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Yao, Jiewen 1 year, 11 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Gerd Hoffmann 1 year, 10 months ago
  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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Gerd Hoffmann 1 year, 10 months ago
  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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Yao, Jiewen 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Gerd Hoffmann 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Yao, Jiewen 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Gerd Hoffmann 1 year, 10 months ago
  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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by James Bottomley 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Yao, Jiewen 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by James Bottomley 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Gerd Hoffmann 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by James Bottomley 1 year, 10 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/5] CryptoPkg/openssl: enable EC unconditionally.
Posted by Yao, Jiewen 1 year, 10 months ago
> 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]
-=-=-=-=-=-=-=-=-=-=-=-