Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl

["Xiaoyu lu" <xiaoyux.lu@intel.com>]

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.

> (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 <xiaoyux.lu@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@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@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@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@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;
>