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

["Zhang, Shenglei" <shenglei.zhang@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, October 22, 2019 12:29 AM
> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>; David Woodhouse <dwmw2@infradead.org>
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg: Upgrade OpenSSL to 1.1.1d
> 
> 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/R
> untimeDxe/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/Varia
> ble/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/Va
> riable/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/Openssl
> Lib/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/Openssl
> Lib/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/Opens
> slLib/OUTPUT/OpensslLib.lib
> >
> > "ar" cr \
> >
> Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/Opens
> slLib/OUTPUT/OpensslLib.lib \
> >
> @Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/Ope
> nsslLib/OUTPUT/object_files.lst
> 
> If I check the file
> 
> 
> Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/Opens
> slLib/OUTPUT/object_files.lst
> 
> I definitely see
> 
> 
> Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/Opens
> slLib/OUTPUT/openssl/crypto/dso/dso_openssl.obj
> 
> there. However, if I run
> 
>   nm
> Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/Opens
> slLib/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.i
> n
> > 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?

Actually... I didn’t. Because I treated it as a small change with little risk.
I found there is no change except the auto-generated things in OpensslLib.inf and OpensslLibCrypto.inf.

With the update in "process_files.pl" according to your comments, things can be generated into dso_conf.h,
where DSO_NONE is not absent now.
And, I test this change on a real platform, which can boot to Shell with this patch.
I 'll send a new patch for the change in "process_files.pl".

Thanks,
Shenglei

> 
> Laszlo