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; Mon, 13 May 2019 08:12:59 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D6C923084288; Mon, 13 May 2019 15:12:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-237.rdu2.redhat.com [10.10.123.237]) by smtp.corp.redhat.com (Postfix) with ESMTP id 921E95D71E; Mon, 13 May 2019 15:12:57 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl To: devel@edk2.groups.io, xiaoyux.lu@intel.com 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> From: "Laszlo Ersek" Message-ID: <33da2dd6-8252-0737-640d-5b618351e94e@redhat.com> Date: Mon, 13 May 2019 17:12:51 +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.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Mon, 13 May 2019 15:12:58 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 OPENSSL_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 accept 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. Please pick one of two: - file a new TianoCore BZ about cleaning up this technical debt, and paste the BZ URL into the code, as a comment - delay TianoCore BZ#1089 to the next edk2-stable release, and work with upstream OpenSSL to compile out parts of "randfile.c". Thanks Laszlo > >> (2b) Alternatively, I'm noticing that "rand" is just another module (similar to "store", see above). Assuming we really don't need RAND_* functions for anything in edk2: have we tried configuring OpenSSL, for the edk2 build, 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 unnecessary files in process_files.pl > > On 05/09/19 07:23, Xiaoyu lu wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1089 >> >> When running process_files.py to configure OpenSSL, we can exclude >> some unnecessary files. This can reduce porting time, compiling time >> 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_locl.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 OpenSSL_1_1_1b, to modify the "crypto/store/" subdirectory, was commit 71a5516dcc8a ("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} = [ >> "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} = [ "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", you 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 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. > >> 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", 2015-09-03, part of OpenSSL_1_1_0 --, Qin Long customized "crypto/rand/rand_unix.c". So that, when OPENSSL_SYS_UEFI was #defined, the real RAND_poll() function was replaced by a stub that 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 honors 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 client 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 OPENSSL_SYS_UEFI is defined. In other words, continue with Qin Long's approach from commit fb4844bbc62f. > > (2b) Alternatively, I'm noticing that "rand" is just another module (similar to "store", see above). Assuming we really don't need RAND_* functions for anything in edk2: have we tried configuring OpenSSL, for the edk2 build, 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 =~ "crypto/bio/b_print.c"; >> + >> + # No need to add unused files in UEFI. >> + # So it can reduce porting time, compile time, library size. >> + next if $s =~ "crypto/rand/randfile.c"; >> + next if $s =~ "crypto/store/"; >> + >> if ($product =~ "libssl") { >> push @sslfilelist, ' $(OPENSSL_PATH)/' . $s . "\r\n"; >> next; >> > > > >