From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 17 May 2019 06:08:27 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D224AC00DDF9; Fri, 17 May 2019 13:08:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-94.rdu2.redhat.com [10.10.120.94]) by smtp.corp.redhat.com (Postfix) with ESMTP id CD3DB38E0E; Fri, 17 May 2019 13:08:18 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 0/7] CryptoPkg: Upgrade OpenSSL to 1.1.1b To: "Lu, XiaoyuX" , "devel@edk2.groups.io" Cc: "Wang, Jian J" , "Ye, Ting" , Ard Biesheuvel , Leif Lindholm References: <1557993298-22205-1-git-send-email-xiaoyux.lu@intel.com> <049e489c-b58f-0fc5-1c66-8ad920d93979@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 17 May 2019 15:08:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 17 May 2019 13:08:21 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/17/19 12:12, Lu, XiaoyuX wrote: > Hi, Lerszlo: well... I agree that my first name may not be trivial to spell, but you can always use the clipboard, whenever in doubt. For the record, it's "Laszlo". > > (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) See my response to Jian on this. > (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. Sorry, I was unclear: I meant a community-level, central CI. Not a personal one. And, the central CI is undergoing design discussions right now. Thanks Laszlo > -----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 > Cc: Wang, Jian J ; Ye, Ting ; Ard Biesheuvel ; Leif Lindholm > 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 >> Cc: Ting Ye >> >> 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 > > >