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

["Laszlo Ersek" <lersek@redhat.com>]

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;
>