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; Tue, 14 May 2019 08:11:20 -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 4CE4065998; Tue, 14 May 2019 15:11:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-233.rdu2.redhat.com [10.10.120.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id D26691A7D3; Tue, 14 May 2019 15:11:18 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl To: "Lu, XiaoyuX" , "devel@edk2.groups.io" Cc: "Wang, Jian J" , "Ye, Ting" References: <1557379429-7527-1-git-send-email-xiaoyux.lu@intel.com> <1557379429-7527-2-git-send-email-xiaoyux.lu@intel.com> <0c5b5e95-cb2c-75af-a30b-015dac14b91c@redhat.com> <33da2dd6-8252-0737-640d-5b618351e94e@redhat.com> From: "Laszlo Ersek" Message-ID: <81f99569-5a7e-5085-1ad3-c0801bc725a9@redhat.com> Date: Tue, 14 May 2019 17:11:17 +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.38]); Tue, 14 May 2019 15:11:20 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 05/14/19 14:41, Lu, XiaoyuX wrote: > Hi Laszlo,=20 > =09I think process_files.pl is used to control which of OpenSSL source f= iles we need which we don't need. > =09If we have unwanted files, the effective way is exclude them directly= in process_files.pl. > You can see process_files.pl > =09 > =09> 129 =E2=94=86 =E2=94=86 =E2=94=86 =E2=94=86 =E2=94=86 next if $s = = =3D~ "crypto/bio/b_print.c"; >=20 > =09Qing Long also use this way to exclude unwanted file. >=20 > =09If the file (example: rand_unix.c) is used by OpenSSL internal, We ca= n't exclude it in process_files.pl,=20 > =09Than we consider submitting patches for OpenSSL. >=20 > =09What do you think? I agree to excluding "rand_unix.c" similarly to "b_print.c"; that is, with "next if" in the "process_files.pl" script. Thanks Laszlo > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of La= szlo Ersek > Sent: Monday, May 13, 2019 11:13 PM > To: devel@edk2.groups.io; Lu, XiaoyuX > Cc: Wang, Jian J ; Ye, Ting > Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude u= nnecessary files in process_files.pl >=20 > On 05/10/19 10:51, Xiaoyu lu wrote: >> Hi, Laszlo: >> >> Thank you for your time. >> >> I try the method you mentioned. >> >>> (1) Therefore, the right thing to do here is to add "no-store" to the = above list, in my opinion. Can you try that, please? >>> >>> And, this change should be a standalone patch, similarly to patch v2 1= /6 in this series. >> >> (1) OpenSSL configure script don't support no-store option. >> It will lead to configure error. >> >> Unsupported options: no-store >> >>> (2a) Therefore, we should modify the "randfile.c" source file, with an= upstream OpenSSL contribution, to hide the function definitions, when OPEN= SSL_SYS_UEFI is defined. In other words, continue with Qin Long's approach = from commit fb4844bbc62f. >> >> I think this is the best way. But the openssl community takes time to a= ccept the patch. >> I just let OpenSSL work for UEFI. So UEFI can use the new algorithm in = OpenSSL_1_1_1. >> I am willing to continue to modify this later. >=20 > Please pick one of two: >=20 > - file a new TianoCore BZ about cleaning up this technical debt, and pas= te the BZ URL into the code, as a comment >=20 > - delay TianoCore BZ#1089 to the next edk2-stable release, and work with= upstream OpenSSL to compile out parts of "randfile.c". >=20 > Thanks > Laszlo >=20 >> >>> (2b) Alternatively, I'm noticing that "rand" is just another module (s= imilar to "store", see above). Assuming we really don't need RAND_* functio= ns for anything in edk2: have we tried configuring OpenSSL, for the edk2 bu= ild, with the "no-rand" parameter? >> >> (2) I'm afraid not. Same as (1) >> >> ***** Unsupported options: no-rand >> >> Thanks, >> Xiaoyu. >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, May 9, 2019 9:43 PM >> To: devel@edk2.groups.io; Lu, XiaoyuX >> Cc: Wang, Jian J ; Ye, Ting >> Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude= =20 >> unnecessary files in process_files.pl >> >> On 05/09/19 07:23, Xiaoyu lu wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1089 >>> >>> When running process_files.py to configure OpenSSL, we can exclude=20 >>> some unnecessary files. This can reduce porting time, compiling time= =20 >>> and library size. >> >> Indeed. >> >>> OpenSSL_1_1_1(1708e3e85b4a8) add a STORE module (crypto/store/*). >> >> This statement is incorrect (or, minimally, inexact). According to the = following command: >> >> $ git log --oneline --reverse OpenSSL_1_1_1b -- crypto/store/ \ >> | head -n 1 >> >> the first OpenSSL commit that added files to crypto/store/ was: >> >>> commit a5db6fa5760f21d16d59e025e930c02456e00fef >>> Author: Richard Levitte >>> Date: Thu May 1 03:53:12 2003 +0000 >>> >>> Define a STORE type. For documentation, read the entry in CHANGES= , >>> crypto/store/README, crypto/store/store.h and crypto/store/str_loc= l.h. >> >> This commit goes back to 2003, and is part of releae OpenSSL_0_9_7d. >> >> Instead, let's check what the following command reports: >> >> $ git log --oneline --reverse \ >> OpenSSL_1_1_0j..OpenSSL_1_1_1b -- crypto/store/ \ >> | head -1 >> >> It states that the first commit after OpenSSL_1_1_0j, but not after Ope= nSSL_1_1_1b, to modify the "crypto/store/" subdirectory, was commit 71a5516= dcc8a ("Add the STORE module", 2017-06-29). >> >> If we investigate that commit: >> >> $ git show --stat 71a5516dcc8a >> >> we see that the commit modifies the Configure script: >> >>> Configure | 2 +- >> >> So let's check that part of the diff in detail: >> >> $ git show 71a5516dcc8a -- Configure >> >> And we get: >> >>> diff --git a/Configure b/Configure >>> index 2eacb2312e34..e302a58abb71 100755 >>> --- a/Configure >>> +++ b/Configure >>> @@ -310,7 +310,7 @@ $config{sdirs} =3D [ >>> "bn", "ec", "rsa", "dsa", "dh", "dso", "engine", >>> "buffer", "bio", "stack", "lhash", "rand", "err", >>> "evp", "asn1", "pem", "x509", "x509v3", "conf", "txt_db", "pkcs7"= , >>> "pkcs12", "comp", "ocsp", "ui", >>> - "cms", "ts", "srp", "cmac", "ct", "async", "kdf" >>> + "cms", "ts", "srp", "cmac", "ct", "async", "kdf", "store" >>> ]; >>> # test/ subdirectories to build >>> $config{tdirs} =3D [ "ossl_shim" ]; >> >> We can see that the "store" module is added after modules such as "cms"= , "ts", "srp", and so on. >> >> Now, if you look at "CryptoPkg/Library/OpensslLib/process_files.pl", yo= u find (with edk2 master being at commit 49693202ec9c): >> >> 49 "./Configure", >> 50 "UEFI", >> 51 "no-afalgeng", >> 52 "no-asm", >> 53 "no-async", <---- disables "async" >> 54 "no-autoalginit", >> 55 "no-autoerrinit", >> 56 "no-bf", >> 57 "no-blake2", >> 58 "no-camellia", >> 59 "no-capieng", >> 60 "no-cast", >> 61 "no-chacha", >> 62 "no-cms", <---- disables "cms" >> 63 "no-ct", <---- disables "ct" >> 64 "no-deprecated", >> 65 "no-dgram", >> 66 "no-dsa", >> 67 "no-dynamic-engine", >> 68 "no-ec", >> 69 "no-ec2m", >> 70 "no-engine", >> 71 "no-err", >> 72 "no-filenames", >> 73 "no-gost", >> 74 "no-hw", >> 75 "no-idea", >> 76 "no-mdc2", >> 77 "no-pic", >> 78 "no-ocb", >> 79 "no-poly1305", >> 80 "no-posix-io", >> 81 "no-rc2", >> 82 "no-rfc3779", >> 83 "no-rmd160", >> 84 "no-scrypt", >> 85 "no-seed", >> 86 "no-sock", >> 87 "no-srp", <---- disables "srp" >> 88 "no-ssl", >> 89 "no-stdio", >> 90 "no-threads", >> 91 "no-ts", <---- disables "ts" >> 92 "no-ui", >> 93 "no-whirlpool" >> >> (1) Therefore, the right thing to do here is to add "no-store" to the a= bove list, in my opinion. Can you try that, please? >> >> And, this change should be a standalone patch, similarly to patch v2 1/= 6 in this series. >> >>> But UEFI don't use them. So exclude these files. >> >>> This file, crypto/rand/randfile.c, have been modified between >>> OpenSSL_1_1_0j(74f2d9c1ec5f5) and OpenSSL_1_1_1b(50eaac9f33376672). >>> It requires more crt runtime support. But UEFI don't use it. >>> So exclude the file. >> >> I think I disagree with this approach. >> >> In OpenSSL commit fb4844bbc62f -- "Add UEFI flag for rand build",=20 >> 2015-09-03, part of OpenSSL_1_1_0 --, Qin Long customized=20 >> "crypto/rand/rand_unix.c". So that, when OPENSSL_SYS_UEFI was=20 >> #defined, the real RAND_poll() function was replaced by a stub that=20 >> would always report failure. (So this was a safe stub.) >> >> In OpenSSL commit 8389ec4b4950 -- "Add --with-rand-seed", 2017-07-22 --= , the feature test itself has been reworked (see the previous patch in this= series). However, it remains the case that "rand_unix.c" consumes and hono= rs the OPENSSL_SYS_UEFI macro. >> >> So, let's check the "randfile.c" file. It defines three functions: >> - RAND_load_file >> - RAND_write_file >> - RAND_file_name >> >> Nothing inside the OpenSSL library calls them (they exist purely for cl= ient code), and nothing in edk2 calls them either. >> >> (2a) Therefore, we should modify the "randfile.c" source file, with an = upstream OpenSSL contribution, to hide the function definitions, when OPENS= SL_SYS_UEFI is defined. In other words, continue with Qin Long's approach f= rom commit fb4844bbc62f. >> >> (2b) Alternatively, I'm noticing that "rand" is just another module (si= milar to "store", see above). Assuming we really don't need RAND_* function= s for anything in edk2: have we tried configuring OpenSSL, for the edk2 bui= ld, with the "no-rand" parameter? >> >> Thanks, >> Laszlo >> >>> >>> Cc: Jian J Wang >>> Cc: Ting Ye >>> Signed-off-by: Xiaoyu Lu >>> --- >>> CryptoPkg/Library/OpensslLib/process_files.pl | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl >>> b/CryptoPkg/Library/OpensslLib/process_files.pl >>> index 6c136cc..e277108 100755 >>> --- a/CryptoPkg/Library/OpensslLib/process_files.pl >>> +++ b/CryptoPkg/Library/OpensslLib/process_files.pl >>> @@ -127,6 +127,12 @@ foreach my $product ((@{$unified_info{libraries}}= , >>> foreach my $s (@{$unified_info{sources}->{$o}}) { >>> next if ($unified_info{generate}->{$s}); >>> next if $s =3D~ "crypto/bio/b_print.c"; >>> + >>> + # No need to add unused files in UEFI. >>> + # So it can reduce porting time, compile time, library si= ze. >>> + next if $s =3D~ "crypto/rand/randfile.c"; >>> + next if $s =3D~ "crypto/store/"; >>> + >>> if ($product =3D~ "libssl") { >>> push @sslfilelist, ' $(OPENSSL_PATH)/' . $s . "\r\n"= ; >>> next; >>> >> >> >> >> >=20 >=20 >=20 >=20