[edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl
Laszlo Ersek
lersek at redhat.com
Tue May 14 15:11:17 UTC 2019
On 05/14/19 14:41, Lu, XiaoyuX wrote:
> 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?
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 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 (#40608): https://edk2.groups.io/g/devel/message/40608
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