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; Thu, 09 May 2019 06:42:47 -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 AFF113084295; Thu, 9 May 2019 13:42:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-234.rdu2.redhat.com [10.10.120.234]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5C45E5D710; Thu, 9 May 2019 13:42:45 +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: Jian J Wang , Ting Ye References: <1557379429-7527-1-git-send-email-xiaoyux.lu@intel.com> <1557379429-7527-2-git-send-email-xiaoyux.lu@intel.com> From: "Laszlo Ersek" Message-ID: <0c5b5e95-cb2c-75af-a30b-015dac14b91c@redhat.com> Date: Thu, 9 May 2019 15:42:44 +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: <1557379429-7527-2-git-send-email-xiaoyux.lu@intel.com> 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]); Thu, 09 May 2019 13:42:46 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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; >