Re: [edk2-devel] [PATCH] CryptoPkg: Upgrade OpenSSL to 1.1.1d

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

On 10/21/19 10:06, Zhang, Shenglei wrote:
> Update openssl from 1.1.1b to 1.1.1d.
> Something needs to be noticed is that, there is a bug existing in the
> released 1_1_1d version(894da2fb7ed5d314ee5c2fc9fd2d9b8b74111596),
> which causes build failure. So we switch the code base to a usable
> version, which is 2 commits later than the stable tag.
> Now we use the version c3656cc594daac8167721dde7220f0e59ae146fc.
> This log is to fix the build failure.
> https://bugzilla.tianocore.org/show_bug.cgi?id=2226
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf   | 57 -------------------
>  .../Library/OpensslLib/OpensslLibCrypto.inf   | 49 ----------------
>  CryptoPkg/Library/OpensslLib/openssl          |  2 +-
>  3 files changed, 1 insertion(+), 107 deletions(-)

When I try to apply this patch manually, on top of current master
(91f98c908627), then "git am" fails.

However, if I try to reproduce this patch myself (advancing the
submodule to c3656cc594da, and then running "process_files.pl"), then
the result ("git diff") matches the code changes in the patch -- not
counting CRLF vs. LF, anyway.

(It seems like the "git am" failure is due to mixed line-endings within
the patch -- the submodule reference hunk uses LFs, not CRLFs. I can
live with that.)

Having to use openssl at c3656cc594da is unfortunate, but I think it's
justified.

Unfortunately, with this update, the following build command fails for
me (it may fail for other OVMF builds as well, this was simply my first
attempt):

  $ nice build \
      -a IA32 \
      -p OvmfPkg/OvmfPkgIa32.dsc \
      -t GCC48 \
      -b DEBUG \
      -D SMM_REQUIRE \
      -D SECURE_BOOT_ENABLE \
      -D NETWORK_IP6_ENABLE \
      -D NETWORK_TLS_ENABLE \
      -D NETWORK_HTTP_BOOT_ENABLE \
      -D E1000_ENABLE \
      -n 4 \
      --report-file=$HOME/tmp/build.ovmf.32.report \
      --log=$HOME/tmp/build.ovmf.32.log \
      --cmd-len=65536 \
      --genfds-multi-thread

The directly failing command is:

  "gcc"  \
    -o Build/OvmfIa32/DEBUG_GCC48/IA32/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm/DEBUG/VariableSmm.dll  \
    -nostdlib  \
    -Wl,-n,-q,--gc-sections  \
    -z common-page-size=0x20  \
    -Wl,--entry,_ModuleEntryPoint  \
    -u _ModuleEntryPoint  \
    -Wl,-Map,Build/OvmfIa32/DEBUG_GCC48/IA32/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm/DEBUG/VariableSmm.map,--whole-archive  \
    -Wl,-m,elf_i386,--oformat=elf32-i386  \
    -z common-page-size=0x1000  \
    -Wl,--start-group,@Build/OvmfIa32/DEBUG_GCC48/IA32/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm/OUTPUT/static_library_files.lst,--end-group  \
    -g  \
    -fshort-wchar  \
    -fno-builtin  \
    -fno-strict-aliasing  \
    -Wall  \
    -Werror  \
    -Wno-array-bounds  \
    -ffunction-sections  \
    -fdata-sections  \
    -include AutoGen.h  \
    -fno-common  \
    -DSTRING_ARRAY_NAME=VariableSmmStrings  \
    -m32  \
    -march=i586  \
    -malign-double  \
    -fno-stack-protector  \
    -D EFI32  \
    -fno-asynchronous-unwind-tables  \
    -Wno-address  \
    -Os  \
    -mno-mmx  \
    -mno-sse  \
    -D DISABLE_NEW_DEPRECATED_INTERFACES  \
    -Wl,--defsym=PECOFF_HEADER_SIZE=0x220  \
    -Wl,--script=BaseTools/Scripts/GccBase.lds

And the error message:

> Build/OvmfIa32/DEBUG_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib(dso_lib.obj): In function `DSO_new_method':
> CryptoPkg/Library/OpensslLib/openssl/crypto/dso/dso_lib.c:25: undefined reference to `DSO_METHOD_openssl'
> Build/OvmfIa32/DEBUG_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib(dso_lib.obj): In function `DSO_pathbyaddr':
> CryptoPkg/Library/OpensslLib/openssl/crypto/dso/dso_lib.c:314: undefined reference to `DSO_METHOD_openssl'

This is strange, because the missing function is provided by
"crypto/dso/dso_openssl.c", which is listed in the INF files.

Hmmm. I ran the following command too:

  $ build \
      -p CryptoPkg/CryptoPkg.dsc \
      -a IA32 \
      -b NOOPT \
      -t GCC48 \
      -m CryptoPkg/Library/OpensslLib/OpensslLib.inf

This compiles OK. The last commands are:

> rm -f Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib
>
> "ar" cr \
>   Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib \
>   @Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/object_files.lst

If I check the file

  Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/object_files.lst

I definitely see

  Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/openssl/crypto/dso/dso_openssl.obj

there. However, if I run

  nm Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib

then the DSO_METHOD_openssl() symbol is reported as undefined:

> dso_lib.obj:
>          U DSO_METHOD_openssl

In fact, if I run the "nm" command on "dso_openssl.obj" itself, the
output is totally empty! (No symbols in the object file.)

Ahh, I know what's up. See the source code in
"crypto/dso/dso_openssl.c":

> #ifdef DSO_NONE
>
> static DSO_METHOD dso_meth_null = {
>     "NULL shared library method"
> };
>
> DSO_METHOD *DSO_METHOD_openssl(void)
> {
>     return &dso_meth_null;
> }
> #endif

The #ifdef comes from OpenSSL commit 5fba3afad017 ("Rework DSO API
conditions and configuration option", 2019-04-10), which is part of
OpenSSL_1_1_1c. See the change (excerpt):

$ git show 5fba3afad017 -- \
    crypto/dso/dso_openssl.c \
    crypto/include/internal/dso_conf.h.in

> commit 5fba3afad01707f4a8856a35500de007a8a256ec
> Author: Richard Levitte <levitte@openssl.org>
> Date:   Mon Apr 1 06:40:33 2019 +0200
>
>     Rework DSO API conditions and configuration option
>
>     'no-dso' is meaningless, as it doesn't get any macro defined.
>     Therefore, we remove all checks of OPENSSL_NO_DSO.  However, there may
>     be some odd platforms with no DSO scheme.  For those, we generate the
>     internal macro DSO_NONE aand use it.
>
>     Reviewed-by: Paul Dale <paul.dale@oracle.com>
>     (Merged from https://github.com/openssl/openssl/pull/8622)
>
> diff --git a/crypto/dso/dso_openssl.c b/crypto/dso/dso_openssl.c
> index 6626331e9256..eeebd98087b4 100644
> --- a/crypto/dso/dso_openssl.c
> +++ b/crypto/dso/dso_openssl.c
> @@ -9,7 +9,7 @@
>
>  #include "dso_locl.h"
>
> -#if !defined(DSO_VMS) && !defined(DSO_DLCFN) && !defined(DSO_DL) && !defined(DSO_WIN32) && !defined(DSO_DLFCN)
> +#ifdef DSO_NONE
>
>  static DSO_METHOD dso_meth_null = {
>      "NULL shared library method"
> diff --git a/crypto/include/internal/dso_conf.h.in b/crypto/include/internal/dso_conf.h.in
> index d6e9d1b1baae..17fae7d8023a 100644
> --- a/crypto/include/internal/dso_conf.h.in
> +++ b/crypto/include/internal/dso_conf.h.in
> @@ -10,7 +10,6 @@
>
>  #ifndef HEADER_DSO_CONF_H
>  # define HEADER_DSO_CONF_H
> -{- output_off() if $disabled{dso} -}
>  {-  # The DSO code currently always implements all functions so that no
>      # applications will have to worry about that from a compilation point
>      # of view. However, the "method"s may return zero unless that platform
> @@ -18,6 +17,9 @@
>      # by a define "DSO_<name>" ... we translate the "dso_scheme" config
>      # string entry into using the following logic;
>      my $scheme = uc $target{dso_scheme};
> +    if (!$scheme) {
> +        $scheme = "NONE";
> +    }
>      my @macros = ( "DSO_$scheme" );
>      if ($scheme eq 'DLFCN') {
>          @macros = ( "DSO_DLFCN", "HAVE_DLFCN_H" );
> @@ -26,5 +28,4 @@
>      }
>      join("\n", map { "# define $_" } @macros); -}
>  # define DSO_EXTENSION "{- $target{dso_extension} -}"
> -{- output_on() if $disabled{dso} -}
>  #endif

Sure enough, "build.info" invokes the generator on this template file
too:

> DEPEND[include/openssl/opensslconf.h]=configdata.pm
> GENERATE[include/openssl/opensslconf.h]=include/openssl/opensslconf.h.in
> DEPEND[crypto/include/internal/bn_conf.h]=configdata.pm
> GENERATE[crypto/include/internal/bn_conf.h]=crypto/include/internal/bn_conf.h.in
> DEPEND[crypto/include/internal/dso_conf.h]=configdata.pm
> GENERATE[crypto/include/internal/dso_conf.h]=crypto/include/internal/dso_conf.h.in

Unfortunately, it seems like the DSO_NONE internal macro is *not*
generated, in our case.

... Ah. Our "process_files.pl" script manually generates
"opensslconf.h", from the configuration data. But, we have never done
the same for "dso_conf.h".

Thus far, we've gotten away with it, because the *absence* of all
DSO_xxx flags happened to do the right thing for us. But now, the "right
thing for us" actually depends on a new macro, DSO_NONE, and for getting
that, we need to invoke the generator on "dso_conf.h.in" too. Otherwise,
"crypto/dso/dso_openssl.c" gets pre-processed to an empty source file,
as a result of openssl commit 5fba3afad017.

Therefore, the bug is in "process_files.pl". Please invoke the generator
on "dso_conf.h.in" too, similarly to "opensslconf.h.in".

... And now I'm left with the question: did you test this patch at all,
with any real platform?

Laszlo