[edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support

Gerd Hoffmann posted 11 patches 1 year, 2 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
.../Include/Dsc/CryptoServicePcd.all.dsc.inc  | 29 +++++++
.../Dsc/CryptoServicePcd.min_dxe_smm.dsc.inc  | 35 +++++++++
.../Dsc/CryptoServicePcd.min_pei.dsc.inc      | 20 +++++
.../Include/Dsc/OvmfCryptoComponents.dsc.inc  | 41 ++++++++++
OvmfPkg/Include/Dsc/OvmfCryptoDefines.dsc.inc |  5 ++
OvmfPkg/Include/Dsc/OvmfCryptoLibs.dsc.inc    | 57 ++++++++++++++
CryptoPkg/CryptoPkg.dsc                       | 78 +------------------
OvmfPkg/AmdSev/AmdSevX64.dsc                  | 11 ++-
OvmfPkg/IntelTdx/IntelTdxX64.dsc              | 15 ++--
OvmfPkg/Microvm/MicrovmX64.dsc                | 22 +++---
OvmfPkg/OvmfPkgIa32.dsc                       | 20 ++---
OvmfPkg/OvmfPkgIa32X64.dsc                    | 20 ++---
OvmfPkg/OvmfPkgX64.dsc                        | 20 ++---
OvmfPkg/AmdSev/AmdSevX64.fdf                  |  6 ++
OvmfPkg/IntelTdx/IntelTdxX64.fdf              |  5 ++
OvmfPkg/Microvm/MicrovmX64.fdf                |  7 ++
OvmfPkg/OvmfPkgIa32.fdf                       |  6 ++
OvmfPkg/OvmfPkgIa32X64.fdf                    |  6 ++
OvmfPkg/OvmfPkgX64.fdf                        |  6 ++
OvmfPkg/Include/Fdf/OvmfCryptoDxeSmm.fdf.inc  | 12 +++
OvmfPkg/Include/Fdf/OvmfCryptoPei.fdf.inc     |  9 +++
.../.azurepipelines/Ubuntu-GCC5.yml           |  7 ++
22 files changed, 300 insertions(+), 137 deletions(-)
create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.all.dsc.inc
create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.min_dxe_smm.dsc.inc
create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.min_pei.dsc.inc
create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoDefines.dsc.inc
create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoLibs.dsc.inc
create mode 100644 OvmfPkg/Include/Fdf/OvmfCryptoDxeSmm.fdf.inc
create mode 100644 OvmfPkg/Include/Fdf/OvmfCryptoPei.fdf.inc
[edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Gerd Hoffmann 1 year, 2 months ago

Gerd Hoffmann (11):
  CryptoPkg: move Driver PCD configs to include files
  OvmfPkg: add OvmfCryptoLibs.dsc.inc
  OvmfPkg: OvmfPkgX64: use Crypto Libs include
  OvmfPkg: Add Crypto driver support, add more OvmfCrypto*.inc files.
  OvmfPkg: OvmfPkgX64: use new Crypto support includes
  OvmfPkg: add OVMF_X64_CRYPTO_DRIVER test case
  OvmfPkg: OvmfPkgIa32X64: use crypto includes
  OvmfPkg: OvmfPkgIa32: use crypto includes
  OvmfPkg: Microvm: use crypto includes
  OvmfPkg: IntelTdx: use crypto includes
  OvmfPkg: AmdSev: use crypto includes

 .../Include/Dsc/CryptoServicePcd.all.dsc.inc  | 29 +++++++
 .../Dsc/CryptoServicePcd.min_dxe_smm.dsc.inc  | 35 +++++++++
 .../Dsc/CryptoServicePcd.min_pei.dsc.inc      | 20 +++++
 .../Include/Dsc/OvmfCryptoComponents.dsc.inc  | 41 ++++++++++
 OvmfPkg/Include/Dsc/OvmfCryptoDefines.dsc.inc |  5 ++
 OvmfPkg/Include/Dsc/OvmfCryptoLibs.dsc.inc    | 57 ++++++++++++++
 CryptoPkg/CryptoPkg.dsc                       | 78 +------------------
 OvmfPkg/AmdSev/AmdSevX64.dsc                  | 11 ++-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc              | 15 ++--
 OvmfPkg/Microvm/MicrovmX64.dsc                | 22 +++---
 OvmfPkg/OvmfPkgIa32.dsc                       | 20 ++---
 OvmfPkg/OvmfPkgIa32X64.dsc                    | 20 ++---
 OvmfPkg/OvmfPkgX64.dsc                        | 20 ++---
 OvmfPkg/AmdSev/AmdSevX64.fdf                  |  6 ++
 OvmfPkg/IntelTdx/IntelTdxX64.fdf              |  5 ++
 OvmfPkg/Microvm/MicrovmX64.fdf                |  7 ++
 OvmfPkg/OvmfPkgIa32.fdf                       |  6 ++
 OvmfPkg/OvmfPkgIa32X64.fdf                    |  6 ++
 OvmfPkg/OvmfPkgX64.fdf                        |  6 ++
 OvmfPkg/Include/Fdf/OvmfCryptoDxeSmm.fdf.inc  | 12 +++
 OvmfPkg/Include/Fdf/OvmfCryptoPei.fdf.inc     |  9 +++
 .../.azurepipelines/Ubuntu-GCC5.yml           |  7 ++
 22 files changed, 300 insertions(+), 137 deletions(-)
 create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.all.dsc.inc
 create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.min_dxe_smm.dsc.inc
 create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.min_pei.dsc.inc
 create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
 create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoDefines.dsc.inc
 create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoLibs.dsc.inc
 create mode 100644 OvmfPkg/Include/Fdf/OvmfCryptoDxeSmm.fdf.inc
 create mode 100644 OvmfPkg/Include/Fdf/OvmfCryptoPei.fdf.inc

-- 
2.39.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99585): https://edk2.groups.io/g/devel/message/99585
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Ard Biesheuvel 1 year, 2 months ago
On Fri, 3 Feb 2023 at 14:28, Gerd Hoffmann <kraxel@redhat.com> wrote:
>

What is the point of this series? If we are trying to deduplicate
crypto code by moving it into a dedicated driver, can we please just
do that unconditionally, instead of doubling the size of the
validation matrix again? Or are there reasons why one might avoid this
crypto driver approach?


>
>
> Gerd Hoffmann (11):
>   CryptoPkg: move Driver PCD configs to include files
>   OvmfPkg: add OvmfCryptoLibs.dsc.inc
>   OvmfPkg: OvmfPkgX64: use Crypto Libs include
>   OvmfPkg: Add Crypto driver support, add more OvmfCrypto*.inc files.
>   OvmfPkg: OvmfPkgX64: use new Crypto support includes
>   OvmfPkg: add OVMF_X64_CRYPTO_DRIVER test case
>   OvmfPkg: OvmfPkgIa32X64: use crypto includes
>   OvmfPkg: OvmfPkgIa32: use crypto includes
>   OvmfPkg: Microvm: use crypto includes
>   OvmfPkg: IntelTdx: use crypto includes
>   OvmfPkg: AmdSev: use crypto includes
>
>  .../Include/Dsc/CryptoServicePcd.all.dsc.inc  | 29 +++++++
>  .../Dsc/CryptoServicePcd.min_dxe_smm.dsc.inc  | 35 +++++++++
>  .../Dsc/CryptoServicePcd.min_pei.dsc.inc      | 20 +++++
>  .../Include/Dsc/OvmfCryptoComponents.dsc.inc  | 41 ++++++++++
>  OvmfPkg/Include/Dsc/OvmfCryptoDefines.dsc.inc |  5 ++
>  OvmfPkg/Include/Dsc/OvmfCryptoLibs.dsc.inc    | 57 ++++++++++++++
>  CryptoPkg/CryptoPkg.dsc                       | 78 +------------------
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  | 11 ++-
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc              | 15 ++--
>  OvmfPkg/Microvm/MicrovmX64.dsc                | 22 +++---
>  OvmfPkg/OvmfPkgIa32.dsc                       | 20 ++---
>  OvmfPkg/OvmfPkgIa32X64.dsc                    | 20 ++---
>  OvmfPkg/OvmfPkgX64.dsc                        | 20 ++---
>  OvmfPkg/AmdSev/AmdSevX64.fdf                  |  6 ++
>  OvmfPkg/IntelTdx/IntelTdxX64.fdf              |  5 ++
>  OvmfPkg/Microvm/MicrovmX64.fdf                |  7 ++
>  OvmfPkg/OvmfPkgIa32.fdf                       |  6 ++
>  OvmfPkg/OvmfPkgIa32X64.fdf                    |  6 ++
>  OvmfPkg/OvmfPkgX64.fdf                        |  6 ++
>  OvmfPkg/Include/Fdf/OvmfCryptoDxeSmm.fdf.inc  | 12 +++
>  OvmfPkg/Include/Fdf/OvmfCryptoPei.fdf.inc     |  9 +++
>  .../.azurepipelines/Ubuntu-GCC5.yml           |  7 ++
>  22 files changed, 300 insertions(+), 137 deletions(-)
>  create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.all.dsc.inc
>  create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.min_dxe_smm.dsc.inc
>  create mode 100644 CryptoPkg/Include/Dsc/CryptoServicePcd.min_pei.dsc.inc
>  create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
>  create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoDefines.dsc.inc
>  create mode 100644 OvmfPkg/Include/Dsc/OvmfCryptoLibs.dsc.inc
>  create mode 100644 OvmfPkg/Include/Fdf/OvmfCryptoDxeSmm.fdf.inc
>  create mode 100644 OvmfPkg/Include/Fdf/OvmfCryptoPei.fdf.inc
>
> --
> 2.39.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99597): https://edk2.groups.io/g/devel/message/99597
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Gerd Hoffmann 1 year, 2 months ago
On Fri, Feb 03, 2023 at 02:33:07PM +0100, Ard Biesheuvel wrote:
> What is the point of this series? If we are trying to deduplicate
> crypto code by moving it into a dedicated driver, can we please just
> do that unconditionally, instead of doubling the size of the
> validation matrix again? Or are there reasons why one might avoid this
> crypto driver approach?

Unfortunately it is not a clear size win everywhere.

PEI jumps up in size even though I'm using the min_pei config for
CryptoPei, seems it *still* has way too much bits compiled in
(didn't look into tweaking the config yet, hints are welcome).

-   17530 TcgPei
+   17146 TcgPei
+   34362 Tcg2Pei
-   51066 Tcg2Pei
+  333950 CryptoPei

SMM doesn't change much (slight increase):

+  106662 VariableSmm
-  540818 VariableSmm
+  479374 CryptoSmm

DXE is a clear win, three users go from > 400k to < 100k which easily
compensates for the almost 700k crypto driver:

+   17326 TlsDxe
-   19494 TcgDxe
+   19450 TcgDxe
+   36682 SecurityStubDxe
+   54630 Tcg2Dxe
-   68498 Tcg2Dxe
+   78898 SecureBootConfigDxe
+  121190 IScsiDxe
-  125174 IScsiDxe
-  404574 SecureBootConfigDxe
-  479414 SecurityStubDxe
-  667006 TlsDxe
+  696298 CryptoDxe

Overall it should still be a (small) win even without looking at why PEI
is so big.

If there are no objections I happily drop the USE_CRYPTO_DRIVER option
and switch over to the crypto driver unconditionally.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99604): https://edk2.groups.io/g/devel/message/99604
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Ard Biesheuvel 1 year, 2 months ago
On Fri, 3 Feb 2023 at 16:37, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Feb 03, 2023 at 02:33:07PM +0100, Ard Biesheuvel wrote:
> > What is the point of this series? If we are trying to deduplicate
> > crypto code by moving it into a dedicated driver, can we please just
> > do that unconditionally, instead of doubling the size of the
> > validation matrix again? Or are there reasons why one might avoid this
> > crypto driver approach?
>
> Unfortunately it is not a clear size win everywhere.
>
> PEI jumps up in size even though I'm using the min_pei config for
> CryptoPei, seems it *still* has way too much bits compiled in
> (didn't look into tweaking the config yet, hints are welcome).
>
> -   17530 TcgPei
> +   17146 TcgPei
> +   34362 Tcg2Pei
> -   51066 Tcg2Pei
> +  333950 CryptoPei
>

Why would we use this for PEI if the size increases?

> SMM doesn't change much (slight increase):
>
> +  106662 VariableSmm
> -  540818 VariableSmm
> +  479374 CryptoSmm
>
> DXE is a clear win, three users go from > 400k to < 100k which easily
> compensates for the almost 700k crypto driver:
>
> +   17326 TlsDxe
> -   19494 TcgDxe
> +   19450 TcgDxe
> +   36682 SecurityStubDxe
> +   54630 Tcg2Dxe
> -   68498 Tcg2Dxe
> +   78898 SecureBootConfigDxe
> +  121190 IScsiDxe
> -  125174 IScsiDxe
> -  404574 SecureBootConfigDxe
> -  479414 SecurityStubDxe
> -  667006 TlsDxe
> +  696298 CryptoDxe
>
> Overall it should still be a (small) win even without looking at why PEI
> is so big.
>
> If there are no objections I happily drop the USE_CRYPTO_DRIVER option
> and switch over to the crypto driver unconditionally.
>

Yeah, I'd prefer that.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99607): https://edk2.groups.io/g/devel/message/99607
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Gerd Hoffmann 1 year, 2 months ago
  Hi,

> > Unfortunately it is not a clear size win everywhere.
> >
> > PEI jumps up in size even though I'm using the min_pei config for
> > CryptoPei, seems it *still* has way too much bits compiled in
> > (didn't look into tweaking the config yet, hints are welcome).
> >
> > -   17530 TcgPei
> > +   17146 TcgPei
> > +   34362 Tcg2Pei
> > -   51066 Tcg2Pei
> > +  333950 CryptoPei
> 
> Why would we use this for PEI if the size increases?

When using the crypto driver I'd prefer to do it everywhere and
don't mix+match things.

Background is that I'm hoping the crypto driver abstraction can also
help to have alternative drivers using other crypto libraries without
creating a huge mess in CryptoPkg.  Specifically add openssl-3 as an
option.  openssl-11 goes EOL later this year (Nov IIRC).  Switch to
openssl-3 unconditionally has been vetoed by Intel due to the size
increase v3 brings.  So I'm looking for options here ...

> > If there are no objections I happily drop the USE_CRYPTO_DRIVER option
> > and switch over to the crypto driver unconditionally.
> 
> Yeah, I'd prefer that.

Noted for v2 next week.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99611): https://edk2.groups.io/g/devel/message/99611
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Ard Biesheuvel 1 year, 2 months ago
On Fri, 3 Feb 2023 at 17:28, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Unfortunately it is not a clear size win everywhere.
> > >
> > > PEI jumps up in size even though I'm using the min_pei config for
> > > CryptoPei, seems it *still* has way too much bits compiled in
> > > (didn't look into tweaking the config yet, hints are welcome).
> > >
> > > -   17530 TcgPei
> > > +   17146 TcgPei
> > > +   34362 Tcg2Pei
> > > -   51066 Tcg2Pei
> > > +  333950 CryptoPei
> >
> > Why would we use this for PEI if the size increases?
>
> When using the crypto driver I'd prefer to do it everywhere and
> don't mix+match things.
>
> Background is that I'm hoping the crypto driver abstraction can also
> help to have alternative drivers using other crypto libraries without
> creating a huge mess in CryptoPkg.  Specifically add openssl-3 as an
> option.  openssl-11 goes EOL later this year (Nov IIRC).  Switch to
> openssl-3 unconditionally has been vetoed by Intel due to the size
> increase v3 brings.  So I'm looking for options here ...
>

I agree that this is a good idea in principle. However, the TPM code
probably just uses a few flavors of SHA and nothing else, and this is
not the part of CryptoPkg that I'd be concerned about. Most of the
issues with OpenSSL are in the TLS part of the library, with the
insanely complex ASN.1 parsing and X.509 handling etc etc.

Could we build CryptoPei with fewer algorithms built into it?

> > > If there are no objections I happily drop the USE_CRYPTO_DRIVER option
> > > and switch over to the crypto driver unconditionally.
> >
> > Yeah, I'd prefer that.
>
> Noted for v2 next week.
>
> take care,
>   Gerd
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99634): https://edk2.groups.io/g/devel/message/99634
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Gerd Hoffmann 1 year, 2 months ago
> > > > PEI jumps up in size even though I'm using the min_pei config for
> > > > CryptoPei, seems it *still* has way too much bits compiled in
> > > > (didn't look into tweaking the config yet, hints are welcome).
> > > >
> > > > +  333950 CryptoPei
> > >
> > > Why would we use this for PEI if the size increases?

> Could we build CryptoPei with fewer algorithms built into it?

Patch attached below brings it down to

  211582 CryptoPei

Which still is quite big for some reason ...

take care,
  Gerd

commit a0ecb20af423d4b97fd008ac05807c46dcad3a53
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Mon Feb 6 08:52:41 2023 +0100

    pei needs hashes only

diff --git a/CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc b/CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc
new file mode 100644
index 000000000000..1ead17340b6c
--- /dev/null
+++ b/CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc
@@ -0,0 +1,10 @@
+##
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.HmacSha256.Family               | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.HmacSha384.Family               | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family                     | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family                   | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha384.Family                   | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha512.Family                   | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sm3.Family                      | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
diff --git a/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc b/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
index e34444dde470..3ab90d7718f5 100644
--- a/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
+++ b/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
@@ -12,7 +12,8 @@
       TlsLib|CryptoPkg/Library/TlsLibNull/TlsLibNull.inf
       OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
     <PcdsFixedAtBuild>
-!include CryptoPkg/Include/Dsc/CryptoServicePcd.min_pei.dsc.inc
+#!include CryptoPkg/Include/Dsc/CryptoServicePcd.min_pei.dsc.inc
+!include CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc
   }
 
   CryptoPkg/Driver/CryptoSmm.inf {



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99653): https://edk2.groups.io/g/devel/message/99653
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Li, Yi 1 year, 2 months ago
Add -DOPENSSL_NO_AUTOALGINIT flag will reduce PEI size by ~60KB, based on patch you attached.

This flag will break PKCS7, Authenticode and Ts, but will be fine if only used in PEI builds.

Regards,
Yi

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
Sent: Monday, February 6, 2023 4:21 PM
To: Ard Biesheuvel <ardb@kernel.org>
Cc: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Michael Roth <michael.roth@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Pawel Polawski <ppolawsk@redhat.com>; Oliver Steffen <osteffen@redhat.com>; Tom Lendacky <thomas.lendacky@amd.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Aktas, Erdem <erdemaktas@google.com>; Jiang, Guomin <guomin.jiang@intel.com>; James Bottomley <jejb@linux.ibm.com>
Subject: Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support

> > > > PEI jumps up in size even though I'm using the min_pei config 
> > > > for CryptoPei, seems it *still* has way too much bits compiled 
> > > > in (didn't look into tweaking the config yet, hints are welcome).
> > > >
> > > > +  333950 CryptoPei
> > >
> > > Why would we use this for PEI if the size increases?

> Could we build CryptoPei with fewer algorithms built into it?

Patch attached below brings it down to

  211582 CryptoPei

Which still is quite big for some reason ...

take care,
  Gerd

commit a0ecb20af423d4b97fd008ac05807c46dcad3a53
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Mon Feb 6 08:52:41 2023 +0100

    pei needs hashes only

diff --git a/CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc b/CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc
new file mode 100644
index 000000000000..1ead17340b6c
--- /dev/null
+++ b/CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc
@@ -0,0 +1,10 @@
+##
+#  SPDX-License-Identifier: BSD-2-Clause-Patent ##
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.HmacSha256.Family               | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.HmacSha384.Family               | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family                     | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family                   | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha384.Family                   | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha512.Family                   | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
+  gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sm3.Family                      | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
diff --git a/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc b/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
index e34444dde470..3ab90d7718f5 100644
--- a/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
+++ b/OvmfPkg/Include/Dsc/OvmfCryptoComponents.dsc.inc
@@ -12,7 +12,8 @@
       TlsLib|CryptoPkg/Library/TlsLibNull/TlsLibNull.inf
       OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
     <PcdsFixedAtBuild>
-!include CryptoPkg/Include/Dsc/CryptoServicePcd.min_pei.dsc.inc
+#!include CryptoPkg/Include/Dsc/CryptoServicePcd.min_pei.dsc.inc
+!include CryptoPkg/Include/Dsc/CryptoServicePcd.hash_only.dsc.inc
   }
 
   CryptoPkg/Driver/CryptoSmm.inf {








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99717): https://edk2.groups.io/g/devel/message/99717
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Pedro Falcato 1 year, 2 months ago
On Fri, Feb 3, 2023 at 4:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Unfortunately it is not a clear size win everywhere.
> > >
> > > PEI jumps up in size even though I'm using the min_pei config for
> > > CryptoPei, seems it *still* has way too much bits compiled in
> > > (didn't look into tweaking the config yet, hints are welcome).
> > >
> > > -   17530 TcgPei
> > > +   17146 TcgPei
> > > +   34362 Tcg2Pei
> > > -   51066 Tcg2Pei
> > > +  333950 CryptoPei
> >
> > Why would we use this for PEI if the size increases?
>
> When using the crypto driver I'd prefer to do it everywhere and
> don't mix+match things.
>
> Background is that I'm hoping the crypto driver abstraction can also
> help to have alternative drivers using other crypto libraries without
> creating a huge mess in CryptoPkg.  Specifically add openssl-3 as an
> option.  openssl-11 goes EOL later this year (Nov IIRC).  Switch to
> openssl-3 unconditionally has been vetoed by Intel due to the size
> increase v3 brings.  So I'm looking for options here ...

Seriously?

Intel is blocking UP TO DATE NOT VULNERABLE OPENSSL because it doesn't
fit their flash due to all the cra- value add?
This is insane by many standards. Your freaking *CRYPTO LIBRARY* goes
EOL and people are still concerned about size.

Stellar job, Intel. Hopefully everyone gets their horrific custom
network stack heartbled to death. Or someone finds yet another Secure
Boot exploit.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99617): https://edk2.groups.io/g/devel/message/99617
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Ard Biesheuvel 1 year, 2 months ago
On Fri, 3 Feb 2023 at 20:45, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Fri, Feb 3, 2023 at 4:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > Unfortunately it is not a clear size win everywhere.
> > > >
> > > > PEI jumps up in size even though I'm using the min_pei config for
> > > > CryptoPei, seems it *still* has way too much bits compiled in
> > > > (didn't look into tweaking the config yet, hints are welcome).
> > > >
> > > > -   17530 TcgPei
> > > > +   17146 TcgPei
> > > > +   34362 Tcg2Pei
> > > > -   51066 Tcg2Pei
> > > > +  333950 CryptoPei
> > >
> > > Why would we use this for PEI if the size increases?
> >
> > When using the crypto driver I'd prefer to do it everywhere and
> > don't mix+match things.
> >
> > Background is that I'm hoping the crypto driver abstraction can also
> > help to have alternative drivers using other crypto libraries without
> > creating a huge mess in CryptoPkg.  Specifically add openssl-3 as an
> > option.  openssl-11 goes EOL later this year (Nov IIRC).  Switch to
> > openssl-3 unconditionally has been vetoed by Intel due to the size
> > increase v3 brings.  So I'm looking for options here ...
>
> Seriously?
>
> Intel is blocking UP TO DATE NOT VULNERABLE OPENSSL because it doesn't
> fit their flash due to all the cra- value add?
> This is insane by many standards. Your freaking *CRYPTO LIBRARY* goes
> EOL and people are still concerned about size.
>
> Stellar job, Intel. Hopefully everyone gets their horrific custom
> network stack heartbled to death. Or someone finds yet another Secure
> Boot exploit.
>

This is uncalled for. Please keep it civil and on topic. You (nor I)
have any context about this, and if you want to start a shouting match
on a public mailing list, I suggest you first get informed about what
the actual reasoning is behind such a decision (which, according to
the above, is the decision to keep OpenSSL 1.1 and 3 available side by
side). And please start another thread for this - I have no interest
in being part of this type of discussion.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99620): https://edk2.groups.io/g/devel/message/99620
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Marvin Häuser 1 year, 2 months ago
Hi Ard,

While I agree the tone is a bit irritating, I am not sure what kind of context you expect there to be. The library is nearing EOL and usage beyond EOL is unacceptable. It will take significant time to solve the related issues, test them, have them merged, and for them to trickle down the IBV chains.

OpenSSL is quite "big" in general and many consider it to not be a good choice for embedded usage. Do you know of any discussion regarding alternatives? I've heard folks use libsodium or mbedtls outside edk2, but don't have any experience with either. (Not necessarily looking to *start* a discussion, but mostly references / reading material, if you have any.)

Best regards,
Marvin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99622): https://edk2.groups.io/g/devel/message/99622
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Ard Biesheuvel 1 year, 2 months ago
On Sat, 4 Feb 2023 at 02:13, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi Ard,
>
> While I agree the tone is a bit irritating, I am not sure what kind of context you expect there to be. The library is nearing EOL and usage beyond EOL is unacceptable. It will take significant time to solve the related issues, test them, have them merged, and for them to trickle down the IBV chains.
>
> OpenSSL is quite "big" in general and many consider it to not be a good choice for embedded usage. Do you know of any discussion regarding alternatives? I've heard folks use libsodium or mbedtls outside edk2, but don't have any experience with either. (Not necessarily looking to *start* a discussion, but mostly references / reading material, if you have any.)
>

Again, I don't have the full context here, so with that in mind:

Open source is about the freedom to use the code base in any way you
like. Surely, Intel (as a collaborator in Tianocore) is entitled to
express a desire to retain the OpenSSL 1.1 version of CryptoPkg as an
option while we move it to OpenSSL 3? It is not even important how
they actually intend to use it, that is really their business.

Of course, if you *buy* from Intel, you have all reason to be annoyed
if their products are based on outdated crypto software. But that
doesn't mean it is up to the community to take away their ability to
do so.

Most Intel based consumer products don't have firmware that is
supplied by Intel directly, and the IBVs have their own forks anyway,
so it is not even clear to me who would be affected by this.

As for the use of mbetls or other [better] TLS libraries: I'd be all
for that, but I'm not sure how much work those libraries need to be
usable in the context of EDK2. IIRC, some changes went upstream into
OpenSSL for the UEFI execution context, and we'd probably need to do
the same for mbedtls.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99633): https://edk2.groups.io/g/devel/message/99633
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Marvin Häuser 1 year, 2 months ago
> On 4. Feb 2023, at 09:05, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Sat, 4 Feb 2023 at 02:13, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Hi Ard,
>> 
>> While I agree the tone is a bit irritating, I am not sure what kind of context you expect there to be. The library is nearing EOL and usage beyond EOL is unacceptable. It will take significant time to solve the related issues, test them, have them merged, and for them to trickle down the IBV chains.
>> 
>> OpenSSL is quite "big" in general and many consider it to not be a good choice for embedded usage. Do you know of any discussion regarding alternatives? I've heard folks use libsodium or mbedtls outside edk2, but don't have any experience with either. (Not necessarily looking to *start* a discussion, but mostly references / reading material, if you have any.)
>> 
> 
> Again, I don't have the full context here, so with that in mind:
> 
> Open source is about the freedom to use the code base in any way you
> like.

This is a point that can trivially be driven ad absurdum, so I’ll not press further. I think everyone agrees to *an extent* and *nobody* will agree to the full extent.

> Surely, Intel (as a collaborator in Tianocore) is entitled to
> express a desire to retain the OpenSSL 1.1 version of CryptoPkg as an
> option while we move it to OpenSSL 3? It is not even important how
> they actually intend to use it, that is really their business.
> 
> Of course, if you *buy* from Intel, you have all reason to be annoyed
> if their products are based on outdated crypto software. But that
> doesn't mean it is up to the community to take away their ability to
> do so.

Not if they drive the deprecation at a reasonable schedule. Regarding which, just in: https://edk2.groups.io/g/devel/message/99638
*Thank you*, Jiewen!

> 
> Most Intel based consumer products don't have firmware that is
> supplied by Intel directly, and the IBVs have their own forks anyway,
> so it is not even clear to me who would be affected by this.

In my experience, things like security get little attention and thus, at least at first, forks will use whatever upstream uses. :(

> 
> As for the use of mbetls or other [better] TLS libraries: I'd be all
> for that, but I'm not sure how much work those libraries need to be
> usable in the context of EDK2. IIRC, some changes went upstream into
> OpenSSL for the UEFI execution context, and we'd probably need to do
> the same for mbedtls.

Not so sure, the big issue with OpenSSL is its embedded-unfriendly design. The biggest reason I could think of would be the ConvertPointer() theatre. I know for a fact that some folks use mbedtls even for UEFI purposes, but not whether that’s for RT code. It’s all closed source, so… :(

Best regards,
Marvin

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99639): https://edk2.groups.io/g/devel/message/99639
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Pedro Falcato 1 year, 2 months ago
On Fri, Feb 3, 2023 at 11:25 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 3 Feb 2023 at 20:45, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > On Fri, Feb 3, 2023 at 4:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > > Unfortunately it is not a clear size win everywhere.
> > > > >
> > > > > PEI jumps up in size even though I'm using the min_pei config for
> > > > > CryptoPei, seems it *still* has way too much bits compiled in
> > > > > (didn't look into tweaking the config yet, hints are welcome).
> > > > >
> > > > > -   17530 TcgPei
> > > > > +   17146 TcgPei
> > > > > +   34362 Tcg2Pei
> > > > > -   51066 Tcg2Pei
> > > > > +  333950 CryptoPei
> > > >
> > > > Why would we use this for PEI if the size increases?
> > >
> > > When using the crypto driver I'd prefer to do it everywhere and
> > > don't mix+match things.
> > >
> > > Background is that I'm hoping the crypto driver abstraction can also
> > > help to have alternative drivers using other crypto libraries without
> > > creating a huge mess in CryptoPkg.  Specifically add openssl-3 as an
> > > option.  openssl-11 goes EOL later this year (Nov IIRC).  Switch to
> > > openssl-3 unconditionally has been vetoed by Intel due to the size
> > > increase v3 brings.  So I'm looking for options here ...
> >
> > Seriously?
> >
> > Intel is blocking UP TO DATE NOT VULNERABLE OPENSSL because it doesn't
> > fit their flash due to all the cra- value add?
> > This is insane by many standards. Your freaking *CRYPTO LIBRARY* goes
> > EOL and people are still concerned about size.
> >
> > Stellar job, Intel. Hopefully everyone gets their horrific custom
> > network stack heartbled to death. Or someone finds yet another Secure
> > Boot exploit.
> >
>
> This is uncalled for. Please keep it civil and on topic. You (nor I)
> have any context about this, and if you want to start a shouting match
> on a public mailing list, I suggest you first get informed about what
> the actual reasoning is behind such a decision (which, according to
> the above, is the decision to keep OpenSSL 1.1 and 3 available side by
> side). And please start another thread for this - I have no interest
> in being part of this type of discussion.

Sorry everyone, that was a ...passionate speech.
I recognize I'm on the wrong here.

Vendors and CryptoPkg people, please consider upgrading to OpenSSL 3.
1.1 is going EOL and security
for crypto related activities (especially for a project like OpenSSL
with such a CVE-full life) should be paramount.
Surely there are other ways you can cut on flash space.

</discussion>

As for the patches themselves, big +1 if they help decouple TLS
libraries. I've been thinking about trying another TLS lib
like mbedtls ever since the problems with OpenSSL and compiler
intrinsics came along, some time ago. Probably smaller too.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99621): https://edk2.groups.io/g/devel/message/99621
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 00/11] OvmfPkg: add Crypto Driver support
Posted by Ard Biesheuvel 1 year, 2 months ago
On Sat, 4 Feb 2023 at 02:08, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Fri, Feb 3, 2023 at 11:25 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 3 Feb 2023 at 20:45, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > >
> > > On Fri, Feb 3, 2023 at 4:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > >
> > > >   Hi,
> > > >
> > > > > > Unfortunately it is not a clear size win everywhere.
> > > > > >
> > > > > > PEI jumps up in size even though I'm using the min_pei config for
> > > > > > CryptoPei, seems it *still* has way too much bits compiled in
> > > > > > (didn't look into tweaking the config yet, hints are welcome).
> > > > > >
> > > > > > -   17530 TcgPei
> > > > > > +   17146 TcgPei
> > > > > > +   34362 Tcg2Pei
> > > > > > -   51066 Tcg2Pei
> > > > > > +  333950 CryptoPei
> > > > >
> > > > > Why would we use this for PEI if the size increases?
> > > >
> > > > When using the crypto driver I'd prefer to do it everywhere and
> > > > don't mix+match things.
> > > >
> > > > Background is that I'm hoping the crypto driver abstraction can also
> > > > help to have alternative drivers using other crypto libraries without
> > > > creating a huge mess in CryptoPkg.  Specifically add openssl-3 as an
> > > > option.  openssl-11 goes EOL later this year (Nov IIRC).  Switch to
> > > > openssl-3 unconditionally has been vetoed by Intel due to the size
> > > > increase v3 brings.  So I'm looking for options here ...
> > >
> > > Seriously?
> > >
> > > Intel is blocking UP TO DATE NOT VULNERABLE OPENSSL because it doesn't
> > > fit their flash due to all the cra- value add?
> > > This is insane by many standards. Your freaking *CRYPTO LIBRARY* goes
> > > EOL and people are still concerned about size.
> > >
> > > Stellar job, Intel. Hopefully everyone gets their horrific custom
> > > network stack heartbled to death. Or someone finds yet another Secure
> > > Boot exploit.
> > >
> >
> > This is uncalled for. Please keep it civil and on topic. You (nor I)
> > have any context about this, and if you want to start a shouting match
> > on a public mailing list, I suggest you first get informed about what
> > the actual reasoning is behind such a decision (which, according to
> > the above, is the decision to keep OpenSSL 1.1 and 3 available side by
> > side). And please start another thread for this - I have no interest
> > in being part of this type of discussion.
>
> Sorry everyone, that was a ...passionate speech.
> I recognize I'm on the wrong here.
>

Thanks, much appreciated.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99632): https://edk2.groups.io/g/devel/message/99632
Mute This Topic: https://groups.io/mt/96722233/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-