[edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl
Xiaoyu lu
xiaoyux.lu at intel.com
Tue May 14 12:41:17 UTC 2019
Hi Laszlo,
I think process_files.pl is used to control which of OpenSSL source files we need which we don't need.
If we have unwanted files, the effective way is exclude them directly in process_files.pl.
You can see process_files.pl
> 129 ┆ ┆ ┆ ┆ ┆ next if $s =~ "crypto/bio/b_print.c";
Qing Long also use this way to exclude unwanted file.
If the file (example: rand_unix.c) is used by OpenSSL internal, We can't exclude it in process_files.pl,
Than we consider submitting patches for OpenSSL.
What do you think?
Thanks,
Xiaoyu
-----Original Message-----
From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Monday, May 13, 2019 11:13 PM
To: devel at edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu at intel.com>
Cc: Wang, Jian J <jian.j.wang at intel.com>; Ye, Ting <ting.ye at intel.com>
Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl
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 at redhat.com]
> Sent: Thursday, May 9, 2019 9:43 PM
> To: devel at edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu at intel.com>
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Ye, Ting <ting.ye at intel.com>
> 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 <levitte at openssl.org>
>> 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 <jian.j.wang at intel.com>
>> Cc: Ting Ye <ting.ye at intel.com>
>> Signed-off-by: Xiaoyu Lu <xiaoyux.lu at intel.com>
>> ---
>> 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;
>>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40593): https://edk2.groups.io/g/devel/message/40593
Mute This Topic: https://groups.io/mt/31552209/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list