Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL to 1.1.1b

["Xiaoyu Lu" <xiaoyux.lu@intel.com>]

Hi, Lerszlo:

(1):

> Unfortunately, I've found another build issue with this series. (My apologies that I didn't discover it earlier.) It is reported in the 32-bit (ARM) build of the ArmVirtQemu platform:
> 
>   CryptoPkg/Library/OpensslLib/openssl/crypto/rand/drbg_lib.c:1028:
>   undefined reference to `__aeabi_ui2d'
> 

OpensslLib[Crypto].inf contains ArmSoftFloatLib as dependent library.

In ArmSoftFloatLib:

 softfloat-for-gcc.h|98| #define uint32_to_float64       __floatunsidf
 softfloat-for-gcc.h|222| #define __floatunsidf       __aeabi_ui2d

 softfloat-for-gcc.h|128| #define float64_to_uint32_round_to_zero     __fixunsdfsi
 softfloat-for-gcc.h|234| #define __fixunsdfsi        __aeabi_d2uiz

But *uint32_to_float64* and *float64_to_uint32_round_to_zero* aren't implemented in softfloat.c

If these two functions implement, the build will pass. (I use dummy functions and try)


(2):

>thus, preferably, a CryptoPkg patch series should be at least build tested (if not boot tested) for all arches, before being posted to the mailing list.

I should test ARM, since IA32 arch has Intrinsic problem(_ftol2). It is very likely that ARM arch does not support it either. 

>(Yes, CI would help a lot with such issues.)

Now I don't have a CI environment here. 
I will setup one for building OvmfPkg, ArmVirtPkg, EmulatorPkg.

Thanks,
Xiaoyu

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Friday, May 17, 2019 2:26 AM
To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL to 1.1.1b

Hi,

(+ Ard and Leif)

On 05/16/19 09:54, Xiaoyu lu wrote:
> This series is also available at:
> https://github.com/xiaoyuxlu/edk2/tree/bz_1089_upgrade_to_openssl_1_1_
> 1b_v4
> 
> Changes:
> 
> (1) CryptoPkgOpensslLib: Modify process_files.pl for  upgrading 
> OpenSSL
> 
> (2) CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl
>     crypto/store/* are excluded.
>     crypto/rand/randfile.c is excluded.
> 
> (3) CryptoPkg/IntrinsicLib: Fix possible unresolved external symbol 
> issue
> 
> (4) CryptoPkg/OpensslLib: Prepare for upgrading OpenSSL
>     Disable warnings for buiding OpenSSL_1_1_1b
> 
> (5) CryptoPkg/OpensslLib: Fix cross-build problem for AARCH64
> 
> (6) CryptoPkg: Upgrade OpenSSL to 1.1.1b
>     The biggest change is use TSC as entropy source
>     If TSC isn't avaiable, fallback to TimerLib(PerformanceCounter).
> 
> (7) CryptoPkg/BaseCryptLib: Make HMAC_CTX size backward compatible
> 
> 
> Verification done for this series:
> * Https boot in OvmfPkg.
> * BaseCrypt Library test. (Ovmf, EmulatorPkg)
> 
> Important notice:
> Nt32Pkg doesn't support TimerLib
>> TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemp
>> TimerLib|late.inf
> So it will failed in Nt32Pkg.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> 
> Laszlo Ersek (1):
>   CryptoPkg/OpensslLib: Fix cross-build problem for AARCH64
> 
> Xiaoyu Lu (6):
>   CryptoPkg/OpensslLib: Modify process_files.pl for upgrading OpenSSL
>   CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl
>   CryptoPkg/IntrinsicLib: Fix possible unresolved external symbol issue
>   CryptoPkg/OpensslLib: Prepare for upgrading OpenSSL
>   CryptoPkg: Upgrade OpenSSL to 1.1.1b
>   CryptoPkg/BaseCryptLib: Make HMAC_CTX size backward compatible
> 
>  CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf    |   4 +-
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf        |  76 ++++-
>  CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf  |  67 ++++-
>  CryptoPkg/Library/Include/CrtLibSupport.h          |  13 +-
>  CryptoPkg/Library/Include/openssl/opensslconf.h    |  54 +++-
>  CryptoPkg/Library/Include/sys/syscall.h            |  11 +
>  CryptoPkg/Library/OpensslLib/buildinf.h            |   2 +
>  CryptoPkg/Library/OpensslLib/rand_pool_noise.h     |  29 ++
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c |   8 +-
>  .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c      |   9 +-
>  .../Library/BaseCryptLib/Hmac/CryptHmacSha256.c    |   8 +-
>  CryptoPkg/Library/IntrinsicLib/Ia32/MathFtol.c     |  22 ++
>  CryptoPkg/Library/OpensslLib/ossl_store.c          |  17 ++
>  CryptoPkg/Library/OpensslLib/rand_pool.c           | 316 +++++++++++++++++++++
>  CryptoPkg/Library/OpensslLib/rand_pool_noise.c     |  29 ++
>  CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c |  43 +++
>  CryptoPkg/Library/OpensslLib/openssl               |   2 +-
>  CryptoPkg/Library/OpensslLib/process_files.pl      |  11 +-
>  18 files changed, 669 insertions(+), 52 deletions(-)  create mode 
> 100644 CryptoPkg/Library/Include/sys/syscall.h
>  create mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.h
>  create mode 100644 CryptoPkg/Library/IntrinsicLib/Ia32/MathFtol.c
>  create mode 100644 CryptoPkg/Library/OpensslLib/ossl_store.c
>  create mode 100644 CryptoPkg/Library/OpensslLib/rand_pool.c
>  create mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.c
>  create mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
> 

Unfortunately, I've found another build issue with this series. (My apologies that I didn't discover it earlier.) It is reported in the 32-bit (ARM) build of the ArmVirtQemu platform:

  CryptoPkg/Library/OpensslLib/openssl/crypto/rand/drbg_lib.c:1028:
  undefined reference to `__aeabi_ui2d'

The referenced line is from the drbg_add() function:

    if (buflen < seedlen || randomness < (double) seedlen) {

Beyond the failure to resolve the "__aeabi_ui2d" symbol, the edk2 coding style spec says, "Floating point operations are not recommended in UEFI firmware." (Even though the UEFI spec describes the required floating point environment for all architectures.)

So, I'm not sure what we should do here. If we think that floating point is plain evil in edk2, then we cannot rebase edk2 to OpenSSL-1.1.1b.

... Hmmm, this seems to be the 32-bit ARM variant of [PATCH v4 3/7]!

If we find floating point generally acceptable in edk2, then Ard and Leif could help us decide please whether this 32-bit ARM issue should be fixed during the feature freeze (when fixes are still allowed), or if it justifies postponing OpenSSL 1.1.1b to the next edk2 stable tag.

Again, I'm sorry that I found this only now -- but "CryptoPkg/CryptoPkg.dsc" is multi-arch:

  SUPPORTED_ARCHITECTURES        = IA32|X64|ARM|AARCH64

thus, preferably, a CryptoPkg patch series should be at least build tested (if not boot tested) for all arches, before being posted to the mailing list.

(Yes, CI would help a lot with such issues.)

Thanks
Laszlo