Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[James Bottomley]

On Fri, 2016-03-11 at 17:53 +, David Woodhouse wrote:
> On Fri, 2016-03-11 at 09:45 -0800, James Bottomley wrote:
> > 
> > > Did you *really* enable the building of openssl-1.0.2x/ssl/*.c in
> > > the
> > > above builds? Or are you talking about something different?
> > 
> > No, and since I'm not using it, separating it is fine with me.
> 
> And here's where I came in... is there any need for it to be
> separate?
> 
> If the objects corresponding to ssl/*.c are present in the library
> archive, doesn't that just mean that they'll get pulled in if they're
> *referenced*, and not if they're not? So why separate it out into
> OpensslTlsLib at all?

OK, I promise to wait until un jetlagged before answering next time. 
 However, my answer is no, combined is fine ... it's a libaray.

James



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 09:45 -0800, James Bottomley wrote:
> 
> > Did you *really* enable the building of openssl-1.0.2x/ssl/*.c in the
> > above builds? Or are you talking about something different?
> 
> No, and since I'm not using it, separating it is fine with me.

And here's where I came in... is there any need for it to be separate?

If the objects corresponding to ssl/*.c are present in the library
archive, doesn't that just mean that they'll get pulled in if they're
*referenced*, and not if they're not? So why separate it out into
OpensslTlsLib at all?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[James Bottomley]

On Fri, 2016-03-11 at 17:38 +, David Woodhouse wrote:
> On Fri, 2016-03-11 at 09:25 -0800, James Bottomley wrote:
> > 
> > > With the ssl/ directory enabled?
> > 
> > Yes, if you crack the package, this is the contents:
> > 
> > /usr/include/edk2
> > /usr/include/edk2/Base.h
> > /usr/include/edk2/Guid
> > /usr/include/edk2/Guid/GlobalVariable.h
> > /usr/include/edk2/Guid/ImageAuthentication.h
> > /usr/include/edk2/Library
> > /usr/include/edk2/Library/BaseCryptLib.h
> > /usr/include/edk2/ProcessorBind.h
> > /usr/include/edk2/Protocol
> > /usr/include/edk2/Protocol/Hash.h
> > /usr/include/edk2/Protocol/Pkcs7Verify.h
> > /usr/include/edk2/Uefi
> > /usr/include/edk2/Uefi/UefiBaseType.h
> > /usr/lib64/edk2
> > /usr/lib64/edk2/OpensslLib.lib
> > 
> > It's the OpensslLib.lib that allows you to link all openssl
> > functions
> > in EFI.  It's cheating quite a bit because the headers aren't
> > present,
> > so you use the Linux headers from openssl-devel when you compile.
> 
> That's dangerous and likely to break; various structures change.
> 
> But still, the OpensslLib in EDK2 only builds libcrypto; the contents
> of the crypto/ directory of OpenSSL.
> 
> The discussion here is about building libssl, from ssl/.
> 
> Jiaxin proposed building it as a separate library. I asked why not 
> just build it into the *same* OpensslLib.

Oh, right, sorry misunderstood.  It does look like my build is
currently without this component, since we mostly only care about the
pkcs7 and x509 functions.

> Did you *really* enable the building of openssl-1.0.2x/ssl/*.c in the
> above builds? Or are you talking about something different?

No, and since I'm not using it, separating it is fine with me.

James


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 09:25 -0800, James Bottomley wrote:
> 
> > With the ssl/ directory enabled?
> 
> Yes, if you crack the package, this is the contents:
> 
> /usr/include/edk2
> /usr/include/edk2/Base.h
> /usr/include/edk2/Guid
> /usr/include/edk2/Guid/GlobalVariable.h
> /usr/include/edk2/Guid/ImageAuthentication.h
> /usr/include/edk2/Library
> /usr/include/edk2/Library/BaseCryptLib.h
> /usr/include/edk2/ProcessorBind.h
> /usr/include/edk2/Protocol
> /usr/include/edk2/Protocol/Hash.h
> /usr/include/edk2/Protocol/Pkcs7Verify.h
> /usr/include/edk2/Uefi
> /usr/include/edk2/Uefi/UefiBaseType.h
> /usr/lib64/edk2
> /usr/lib64/edk2/OpensslLib.lib
> 
> It's the OpensslLib.lib that allows you to link all openssl functions
> in EFI.  It's cheating quite a bit because the headers aren't present,
> so you use the Linux headers from openssl-devel when you compile.

That's dangerous and likely to break; various structures change.

But still, the OpensslLib in EDK2 only builds libcrypto; the contents
of the crypto/ directory of OpenSSL.

The discussion here is about building libssl, from ssl/.

Jiaxin proposed building it as a separate library. I asked why not just
build it into the *same* OpensslLib.

Did you *really* enable the building of openssl-1.0.2x/ssl/*.c in the
above builds? Or are you talking about something different?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[James Bottomley]

On Fri, 2016-03-11 at 16:32 +, David Woodhouse wrote:
> On Fri, 2016-03-11 at 07:54 -0800, James Bottomley wrote:
> > 
> > I package it here:
> > 
> > https://build.opensuse.org/package/show/home:jejb1:UEFI/OVMF
> > 
> > in edk2-devel
> 
> With the ssl/ directory enabled?

Yes, if you crack the package, this is the contents:

/usr/include/edk2
/usr/include/edk2/Base.h
/usr/include/edk2/Guid
/usr/include/edk2/Guid/GlobalVariable.h
/usr/include/edk2/Guid/ImageAuthentication.h
/usr/include/edk2/Library
/usr/include/edk2/Library/BaseCryptLib.h
/usr/include/edk2/ProcessorBind.h
/usr/include/edk2/Protocol
/usr/include/edk2/Protocol/Hash.h
/usr/include/edk2/Protocol/Pkcs7Verify.h
/usr/include/edk2/Uefi
/usr/include/edk2/Uefi/UefiBaseType.h
/usr/lib64/edk2
/usr/lib64/edk2/OpensslLib.lib

It's the OpensslLib.lib that allows you to link all openssl functions
in EFI.  It's cheating quite a bit because the headers aren't present,
so you use the Linux headers from openssl-devel when you compile.

James


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 07:54 -0800, James Bottomley wrote:
> 
> I package it here:
> 
> https://build.opensuse.org/package/show/home:jejb1:UEFI/OVMF
> 
> in edk2-devel

With the ssl/ directory enabled?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[James Bottomley]

On Fri, 2016-03-11 at 09:00 +, David Woodhouse wrote:
> On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> > --- a/CryptoPkg/CryptoPkg.dsc
> > +++ b/CryptoPkg/CryptoPkg.dsc
> > @@ -48,10 +48,11 @@
> >   
> > UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriver
> > EntryPoint.inf
> >   
> > UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/
> > UefiApplicationEntryPoint.inf
> >  
> >IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> >OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > +  OpensslTlsLib|CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
> 
> One more thing... does this *need* to be a separate library?
> 
> It looks like the libraries are built into an archive and then linked
> statically. So only those objects which are *referenced* are actually
> pulled into the build. Which means that if we just *add* the ssl/
> directory to the OpensslLib build, it will only be pulled in if
> something *uses* it. Doesn't it?

I package it here:

https://build.opensuse.org/package/show/home:jejb1:UEFI/OVMF

in edk2-devel

I don't *need* to, but it's really useful having a single linkable sour
ce for an openssl EFI library rather than having to build it ourselves
(as shim currently does)

James


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 09:39 +, Long, Qin wrote:
> 
> > It looks like the libraries are built into an archive and then
> linked
> > statically. So only those objects which are *referenced* are
> actually
> > pulled into the build. Which means that if we just *add* the ssl/
> > directory to the OpensslLib build, it will only be pulled in if
> > something *uses* it. Doesn't it?
> > 
> 
> Yes, it's feasible to archive two libraries into one, and only
> referenced symbols will be included. 
> The current design (separated libraries) is try to keep the original
> openssl layout (libcrypto and libssl). Different library serve as
> different scopes. Of cause, the name of OpensslLib.inf looks
> confusing, which should be one crypto library only. 
> 
> I agree the proposal looks also valuable. We should ever discuss this
> internally. Let me try and get some size data for evaluations (I
> think the total symbols / functions in image still highly depend on
> the capabilities of the compiler / linker).

Yeah. With GCC we seem to have function granularity — if a function
isn't actually called, it gets completely dropped out of the image.

With MSVC it seems to be object file granularity. So if *one* function
in a given .obj file is called, that whole .obj file is included.

That's why we had additional problems with the MSVC build and needed
extra cleanups in the OpenSSL code (and I could reproduce them on Linux
by adding -fno-function-sections to the CFLAGS).

But I think that in both cases, we should have confidence that if you
don't *use* anything from the objects in the ssl/ directory, you won't
get anything else pulled in.

I'll try just fixing the process_files.sh script to include the ssl/*.c
files in the build. If I'm right, the resulting image will be
identical.

I'm slightly concerned by all the other duplication in the
OpensslTlsLib.inf file — the CFLAGS and other things. Merging them into
one, if it's technically feasible, does seem cleaner.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Long, Qin]

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Friday, March 11, 2016 1:00 AM
> To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-de...@ml01.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Long, Qin 
> <qin.l...@intel.com>
> Subject: Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 
> 'openssl\ssl'
> 
> On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> > --- a/CryptoPkg/CryptoPkg.dsc
> > +++ b/CryptoPkg/CryptoPkg.dsc
> > @@ -48,10 +48,11 @@
> >    
> > UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
> >    
> > UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
> >
> >    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> >    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > +  OpensslTlsLib|CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
> 
> One more thing... does this *need* to be a separate library?
> 
> It looks like the libraries are built into an archive and then linked
> statically. So only those objects which are *referenced* are actually
> pulled into the build. Which means that if we just *add* the ssl/
> directory to the OpensslLib build, it will only be pulled in if
> something *uses* it. Doesn't it?
> 

Yes, it's feasible to archive two libraries into one, and only referenced 
symbols will be included. 
The current design (separated libraries) is try to keep the original openssl 
layout (libcrypto and libssl). Different library serve as different scopes. Of 
cause, the name of OpensslLib.inf looks confusing, which should be one crypto 
library only. 

I agree the proposal looks also valuable. We should ever discuss this 
internally. Let me try and get some size data for evaluations (I think the 
total symbols / functions in image still highly depend on the capabilities of 
the compiler / linker). 

> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> --- a/CryptoPkg/CryptoPkg.dsc
> +++ b/CryptoPkg/CryptoPkg.dsc
> @@ -48,10 +48,11 @@
>    
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    
> UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>  
>    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> +  OpensslTlsLib|CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf

One more thing... does this *need* to be a separate library?

It looks like the libraries are built into an archive and then linked
statically. So only those objects which are *referenced* are actually
pulled into the build. Which means that if we just *add* the ssl/
directory to the OpensslLib build, it will only be pulled in if
something *uses* it. Doesn't it?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation




smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Wu, Jiaxin]

Thanks for comments. /Jiaxin

> -Original Message-
> From: Wu, Jiaxin
> Sent: Friday, March 11, 2016 10:09 AM
> To: David Woodhouse <dw...@infradead.org>; edk2-de...@ml01.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Long,
> Qin <qin.l...@intel.com>
> Subject: RE: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to
> enable 'openssl\ssl'
> 
> 
> 
> > -Original Message-
> > From: David Woodhouse [mailto:dw...@infradead.org]
> > Sent: Thursday, March 10, 2016 7:31 PM
> > To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-de...@ml01.01.org
> > Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>;
> > Long, Qin <qin.l...@intel.com>
> > Subject: Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to
> > enable 'openssl\ssl'
> >
> > On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> > > diff --git a/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni
> > b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni
> > > new file mode 100644
> > > index
> >
> ..384f0245db8d1d3a1d758f6db
> > 58f85f5fc155211
> > > GIT binary patch
> > > literal 1792
> > > zcmd6oNpBND5QXcE#D5q$7eI*(95^6^u<-yRLdNo
> > > z1hRUq>Q}F-Uyr|ity#?y+9Q66y|RhTY;I$_Z}-
> > @ht!o$TZI~@=Wh1+Edtz%VSZ}e7
> > >
> >
> z@RvPjZ){8J@H=H4#u^Bx%oF;V4LH@OU9+BnxOTLKpDVGH?5@D5?il=!OkTr
> > rO%4TY
> > >
> >
> zr_`q;n+G<uhm23{u|2V8cFa5@lak!#%yp*Vl=}TV6RTsFM?_OK3$N;!am#E(M
> > P69L
> >
> > These should be UTF-8 now, shouldn't they? It seems wrong to be adding
> > new UTF-16 files while other people are submitting patches to convert
> > *from* UTF-16 to UTF-8.
> >
> > --
> > David WoodhouseOpen Source Technology Centre
> > david.woodho...@intel.com  Intel Corporation

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Wu, Jiaxin]

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Thursday, March 10, 2016 7:31 PM
> To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-de...@ml01.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Long,
> Qin <qin.l...@intel.com>
> Subject: Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to
> enable 'openssl\ssl'
> 
> On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> > diff --git a/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni
> b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni
> > new file mode 100644
> > index
> ..384f0245db8d1d3a1d758f6db
> 58f85f5fc155211
> > GIT binary patch
> > literal 1792
> > zcmd6oNpBND5QXcE#D5q$7eI*(95^6^u<-yRLdNo
> > z1hRUq>Q}F-Uyr|ity#?y+9Q66y|RhTY;I$_Z}-
> @ht!o$TZI~@=Wh1+Edtz%VSZ}e7
> >
> z@RvPjZ){8J@H=H4#u^Bx%oF;V4LH@OU9+BnxOTLKpDVGH?5@D5?il=!OkTr
> rO%4TY
> >
> zr_`q;n+G<uhm23{u|2V8cFa5@lak!#%yp*Vl=}TV6RTsFM?_OK3$N;!am#E(M
> P69L
> 
> These should be UTF-8 now, shouldn't they? It seems wrong to be adding
> new UTF-16 files while other people are submitting patches to convert
> *from* UTF-16 to UTF-8.
> 
> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Wu, Jiaxin]

You are right. The change for EDKII_openssl patch is unreasonable. 

Actually, this change was based on the openssl version 1.0.2e. The issue has 
been fixed in the later openssl version. So, just ignore it. I will create 
another patch for whole 'openssl\ssl' feature requirement to resolve this patch 
conflict issue.

Thanks.
Jiaxin

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Thursday, March 10, 2016 6:43 PM
> To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-de...@ml01.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Long,
> Qin <qin.l...@intel.com>
> Subject: Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to
> enable 'openssl\ssl'
> 
> On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> >
> > diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> > index c42b776..f2d8f1a 100644
> > --- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> > +++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> > @@ -11,10 +11,19 @@ diff U3 crypto/bio/bio.h crypto/bio/bio.h
> >   BIO *BIO_new_fp(FILE *stream, int close_flag);
> >  +# ifndef OPENSSL_NO_FP_API
> >   #  define BIO_s_file_internal    BIO_s_file
> >   # endif
> >   BIO *BIO_new(BIO_METHOD *type);
> > +@@ -655,6 +655,8 @@
> > + BIO *BIO_new_file(const char *filename, const char *mode);
> > + BIO *BIO_new_fp(FILE *stream, int close_flag);
> > + #  define BIO_s_file_internal    BIO_s_file
> > ++# else
> > ++#  define BIO_s_file_internal()  NULL
> > + # endif
> > + BIO *BIO_new(BIO_METHOD *type);
> > + int BIO_set(BIO *a, BIO_METHOD *type);
> >  diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
> >  --- crypto/bio/bss_file.c  Thu Jan 28 21:38:30 2016
> >  +++ crypto/bio/bss_file.c  Wed Feb 17 16:01:02 2016
> >  @@ -467,6 +467,23 @@
> >   return (ret);
> 
> As a general rule, you should never make have been making changes to
> this OpenSSL patch without ensuring that a ticket is filed upstream.
> 
> As of this week, there is *nothing* in the EDKII_openssl patch which
> isn't a backport of a commit from OpenSSL 1.1. The patch is
> autogenerated from a 1.0.2+backports git tree.
> 
> Adding to it like this was *never* acceptable. Sure, you were only
> making it a little bit worse at a time, but please don't. It just isn't
> the way to do things.
> 
> In this case, perhaps the *only* thing missing was the fact that this
> should have been in its own separate commit, with a commit comment
> *identifying* the upstream ticket (and OpenSSL 1.1 commit) in which it
> was fixed. But that's important to get right too.
> 
> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Wu, Jiaxin]

Hi David,

Good point. I agree the change for 'time' should be standalone commit. 

This change is to avoid the potential system hang caused by the NULL 'time' 
parameter usage.

Thanks.
Jiaxin

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Thursday, March 10, 2016 6:39 PM
> To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-de...@ml01.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Long,
> Qin <qin.l...@intel.com>
> Subject: Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to
> enable 'openssl\ssl'
> 
> On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> >
> > diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
> b/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
> > index 6422d61..93e487d 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
> > @@ -1,10 +1,10 @@
> >  /** @file
> >    C Run-Time Libraries (CRT) Time Management Routines Wrapper
> Implementation
> >    for OpenSSL-based Cryptographic Library (used in DXE & RUNTIME).
> >
> > -Copyright (c) 2010 - 2011, Intel Corporation. All rights reserved.
> > +Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.
> >  This program and the accompanying materials
> >  are licensed and made available under the terms and conditions of the BSD
> License
> >  which accompanies this distribution.  The full text of the license may be
> found at
> >  http://opensource.org/licenses/bsd-license.php
> >
> > @@ -71,10 +71,11 @@ UINTN CumulativeDays[2][14] = {
> >  //  INTN *timer
> >  //  )
> >  time_t time (time_t *timer)
> >  {
> >    EFI_TIME  Time;
> > +  time_t    CalTime;
> >    UINTN Year;
> >
> >    //
> >    // Get the current time and date information
> >    //
> > @@ -82,26 +83,30 @@ time_t time (time_t *timer)
> >
> >    //
> >    // Years Handling
> >    // UTime should now be set to 00:00:00 on Jan 1 of the current year.
> >    //
> > -  for (Year = 1970, *timer = 0; Year != Time.Year; Year++) {
> > -    *timer = *timer + (time_t)(CumulativeDays[IsLeap(Year)][13] *
> SECSPERDAY);
> > +  for (Year = 1970, CalTime = 0; Year != Time.Year; Year++) {
> > +    CalTime = CalTime + (time_t)(CumulativeDays[IsLeap(Year)][13] *
> SECSPERDAY);
> >    }
> >
> >    //
> >    // Add in number of seconds for current Month, Day, Hour, Minute,
> Seconds, and TimeZone adjustment
> >    //
> > -  *timer = *timer +
> > -   (time_t)((Time.TimeZone != EFI_UNSPECIFIED_TIMEZONE) ?
> (Time.TimeZone * 60) : 0) +
> > -   (time_t)(CumulativeDays[IsLeap(Time.Year)][Time.Month] *
> SECSPERDAY) +
> > -   (time_t)(((Time.Day > 0) ? Time.Day - 1 : 0) * SECSPERDAY) +
> > -   (time_t)(Time.Hour * SECSPERHOUR) +
> > -   (time_t)(Time.Minute * 60) +
> > -   (time_t)Time.Second;
> > -
> > -  return *timer;
> > +  CalTime = CalTime +
> > +    (time_t)((Time.TimeZone != EFI_UNSPECIFIED_TIMEZONE) ?
> (Time.TimeZone * 60) : 0) +
> > +    (time_t)(CumulativeDays[IsLeap(Time.Year)][Time.Month] *
> SECSPERDAY) +
> > +    (time_t)(((Time.Day > 0) ? Time.Day - 1 : 0) * SECSPERDAY) +
> > +    (time_t)(Time.Hour * SECSPERHOUR) +
> > +    (time_t)(Time.Minute * 60) +
> > +    (time_t)Time.Second;
> > +
> > +  if (timer != NULL) {
> > +    *timer = CalTime;
> > +  }
> > +
> > +  return CalTime;
> >  }
> >
> >  //
> >  // Convert a time value from type time_t to struct tm.
> >  //
> 
> It looks like this should be a single standalone commit, with its own
> commit comment explaining the change.
> 
> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Long, Qin]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of David 
> Woodhouse
> Sent: Thursday, March 10, 2016 10:01 AM
> To: Long, Qin <qin.l...@intel.com>; Wu, Jiaxin <jiaxin...@intel.com>; 
> edk2-de...@ml01.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>
> Subject: Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 
> 'openssl\ssl'
> 
> On Thu, 2016-03-10 at 17:12 +, Long, Qin wrote:
> >
> > This patch series should be based on the old version, before the
> > back-porting of upstreaming patch was done.
> > We should have no need to add the extra patches on OpenSSL now for
> > OpensslTlsLib build now.
> 
> Do you have a simple test case for OpensslTlsLib? I have it building
> the library against 1.0.2, and I'll make sure it builds for 1.1 too
> (although today was the last day for making significant changes in 1.1
> so I hope we aren't missing anything!).
> 
> However, a simple UEFI application which can make a TLS connection
> would be useful... or at least a git tree I can pull the HTTPS work
> from; applying patches is kind of painful until we get the CRLF
> nonsense fixed.
> 

Exactly. 
Unfortunately, we have no this kind of app (like Cryptest) in hand. I ever did 
one app to validate the SSL handshaking by leveraging the memory BIO (no 
socket) mechanism to make sure which is workable in UEFI environment. OK, 
Jiaxin and I will summary some steps for HTTPS validations. 
And will also work out one simple validation application (such as Tlstest.efi) 
for this case later, which should be helpful. 

> --
> dwmw2

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Thu, 2016-03-10 at 17:12 +, Long, Qin wrote:
> 
> This patch series should be based on the old version, before the
> back-porting of upstreaming patch was done.
> We should have no need to add the extra patches on OpenSSL now for
> OpensslTlsLib build now.

Do you have a simple test case for OpensslTlsLib? I have it building
the library against 1.0.2, and I'll make sure it builds for 1.1 too
(although today was the last day for making significant changes in 1.1
so I hope we aren't missing anything!).

However, a simple UEFI application which can make a TLS connection
would be useful... or at least a git tree I can pull the HTTPS work
from; applying patches is kind of painful until we get the CRLF
nonsense fixed.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Long, Qin]

David,

This patch series should be based on the old version, before the back-porting 
of upstreaming patch was done.
We should have no need to add the extra patches on OpenSSL now for 
OpensslTlsLib build now.


Best Regards & Thanks,
LONG, Qin

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Thursday, March 10, 2016 2:47 AM
> To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-de...@ml01.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Long, Qin 
> <qin.l...@intel.com>
> Subject: Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 
> 'openssl\ssl'
> 
> On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> > diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> > b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> > index c0ccc0e..e68bfb8 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> > @@ -446,5 +446,10 @@ void syslog (int a, const char *c, ...)
> >
> >  ssize_t write (int f, const void *b, size_t l)
> >  {
> >    return 0;
> >  }
> > +
> > +int printf (char const *fmt, ...)
> > +{
> > +  return 0;
> > +}
> 
> I'm assuming this is to work around a stray printf() in OpenSSL code
> that we weren't building before?
> 
> The correct fix is to file a ticket upstream, submit a *fix* upstream
> to be included in OpenSSL, and then to add the corresponding backported
> patch to our EDKII_openssl patch.
> 
> Please don't add more workarounds like the above; we're trying to clean
> those up not accumulate more.
> 
> And again... even if this was the right thing to do, it lives in a
> *separate* standalone commit. It's OK if it's gratuitous, and the
> commit comment simply says "we *will* want this because...".
> 
> See some of the OpenSSL API cleanups, for example, which make things
> *ready* for OpenSSL 1.1 even while we're still using 1.0.2.
> 
> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni 
> b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni
> new file mode 100644
> index 
> ..384f0245db8d1d3a1d758f6db58f85f5fc155211
> GIT binary patch
> literal 1792
> zcmd6oNpBND5QXcE#D5q$7eI*(95^6^u<-yRLdNo
> z1hRUq>Q}F-Uyr|ity#?y+9Q66y|RhTY;I$_Z}-@ht!o$TZI~@=Wh1+Edtz%VSZ}e7
> z@RvPjZ){8J@H=H4#u^Bx%oF;V4LH@OU9+BnxOTLKpDVGH?5@D5?il=!OkTrrO%4TY
> zr_`q;n+G

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> index c0ccc0e..e68bfb8 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> @@ -446,5 +446,10 @@ void syslog (int a, const char *c, ...)
>  
>  ssize_t write (int f, const void *b, size_t l)
>  {
>    return 0;
>  }
> +
> +int printf (char const *fmt, ...)
> +{
> +  return 0;
> +}

I'm assuming this is to work around a stray printf() in OpenSSL code
that we weren't building before?

The correct fix is to file a ticket upstream, submit a *fix* upstream
to be included in OpenSSL, and then to add the corresponding backported
patch to our EDKII_openssl patch.

Please don't add more workarounds like the above; we're trying to clean
those up not accumulate more.

And again... even if this was the right thing to do, it lives in a
*separate* standalone commit. It's OK if it's gratuitous, and the
commit comment simply says "we *will* want this because...".

See some of the OpenSSL API cleanups, for example, which make things
*ready* for OpenSSL 1.1 even while we're still using 1.0.2.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf 
> b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
> new file mode 100644
> index 000..fbd2b08
> --- /dev/null
> +++ b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
> @@ -0,0 +1,110 @@
 ...
> +[Sources]
> +  $(OPENSSL_PATH)/e_os.h
> +  $(OPENSSL_PATH)/ssl/s2_meth.c
> +  $(OPENSSL_PATH)/ssl/s2_srvr.c
> +  $(OPENSSL_PATH)/ssl/s2_clnt.c
> +  $(OPENSSL_PATH)/ssl/s2_lib.c
> +  $(OPENSSL_PATH)/ssl/s2_enc.c

These file lists are auto-generated now. You'll need to extend the
process_files.sh script to do this for OpensslTlsLib.inf just like it
does for OpensslLib.inf.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> 
> diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch 
> b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> index c42b776..f2d8f1a 100644
> --- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> +++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> @@ -11,10 +11,19 @@ diff U3 crypto/bio/bio.h crypto/bio/bio.h
>   BIO *BIO_new_fp(FILE *stream, int close_flag);
>  +# ifndef OPENSSL_NO_FP_API
>   #  define BIO_s_file_internal    BIO_s_file
>   # endif
>   BIO *BIO_new(BIO_METHOD *type);
> +@@ -655,6 +655,8 @@
> + BIO *BIO_new_file(const char *filename, const char *mode);
> + BIO *BIO_new_fp(FILE *stream, int close_flag);
> + #  define BIO_s_file_internal    BIO_s_file
> ++# else
> ++#  define BIO_s_file_internal()  NULL
> + # endif
> + BIO *BIO_new(BIO_METHOD *type);
> + int BIO_set(BIO *a, BIO_METHOD *type);
>  diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
>  --- crypto/bio/bss_file.c  Thu Jan 28 21:38:30 2016
>  +++ crypto/bio/bss_file.c  Wed Feb 17 16:01:02 2016
>  @@ -467,6 +467,23 @@
>   return (ret);

As a general rule, you should never make have been making changes to
this OpenSSL patch without ensuring that a ticket is filed upstream.

As of this week, there is *nothing* in the EDKII_openssl patch which
isn't a backport of a commit from OpenSSL 1.1. The patch is
autogenerated from a 1.0.2+backports git tree.

Adding to it like this was *never* acceptable. Sure, you were only
making it a little bit worse at a time, but please don't. It just isn't
the way to do things.

In this case, perhaps the *only* thing missing was the fact that this
should have been in its own separate commit, with a commit comment
*identifying* the upstream ticket (and OpenSSL 1.1 commit) in which it
was fixed. But that's important to get right too.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[Jiaxin Wu]

This patch is used to add OpensslTlsLib module to enable
'openssl\ssl' function.

Cc: Long Qin 
Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
 CryptoPkg/CryptoPkg.dsc|   1 +
 CryptoPkg/Include/OpenSslSupport.h |  11 ++-
 .../Library/BaseCryptLib/SysCall/CrtWrapper.c  |   5 +
 .../Library/BaseCryptLib/SysCall/TimerWrapper.c|  29 +++---
 .../Library/OpensslLib/EDKII_openssl-1.0.2f.patch  |   9 ++
 CryptoPkg/Library/OpensslLib/Install.cmd   |   1 +
 CryptoPkg/Library/OpensslLib/Install.sh|   1 +
 CryptoPkg/Library/OpensslLib/OpensslLib.inf|   2 +-
 CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf | 110 +
 CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni | Bin 0 -> 1792 bytes
 10 files changed, 155 insertions(+), 14 deletions(-)
 create mode 100644 CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
 create mode 100644 CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni

diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
index 5ae0e67..bb7f082 100644
--- a/CryptoPkg/CryptoPkg.dsc
+++ b/CryptoPkg/CryptoPkg.dsc
@@ -48,10 +48,11 @@
   
UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
   
UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
 
   IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
   OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
+  OpensslTlsLib|CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
 
 [LibraryClasses.ARM, LibraryClasses.AARCH64]
   #
   # It is not possible to prevent the ARM compiler for generic intrinsic 
functions.
   # This library provides the instrinsic functions generate by a given 
compiler.
diff --git a/CryptoPkg/Include/OpenSslSupport.h 
b/CryptoPkg/Include/OpenSslSupport.h
index 239ae8b..13c73b5 100644
--- a/CryptoPkg/Include/OpenSslSupport.h
+++ b/CryptoPkg/Include/OpenSslSupport.h
@@ -1,9 +1,9 @@
 /** @file
   Root include file to support building OpenSSL Crypto Library.
 
-Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
 http://opensource.org/licenses/bsd-license.php
 
@@ -116,10 +116,12 @@ typedef UINT32 ino_t;
 typedef UINT32 dev_t;
 typedef UINT16 nlink_t;
 typedef intpid_t;
 typedef void   *DIR;
 typedef void   __sighandler_t (int);
+typedef UINT8  __uint8_t;
+typedef UINT8  sa_family_t;
 
 //
 // Structures from EFI Application Toolkit required to buiild Open SSL
 //
 struct tm {
@@ -170,10 +172,16 @@ struct stat {
   UINT32   st_gen;  /* file generation number */
   INT32st_lspare;
   INT64st_qspare[2];
 };
 
+struct sockaddr {
+  __uint8_t sa_len; /* total length */
+  sa_family_t sa_family;/* address family */
+  charsa_data[14];  /* actually longer; address value */
+};
+
 //
 // Externs from EFI Application Toolkit required to buiild Open SSL
 //
 extern int errno;
 
@@ -270,8 +278,9 @@ extern FILE  *stdout;
 #define strchr(str,ch)ScanMem8((VOID 
*)(str),AsciiStrSize(str),(UINT8)ch)
 #define abort()   ASSERT (FALSE)
 #define assert(expression)
 #define localtime(timer)  NULL
 #define gmtime_r(timer,result)(result = NULL)
+#define gettimeofday(tvp,tz)  do { (tvp)->tv_sec = time(NULL); 
(tvp)->tv_usec = 0; } while (0)
 #define atoi(nptr)AsciiStrDecimalToUintn(nptr)
 
 #endif
diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c 
b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
index c0ccc0e..e68bfb8 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
@@ -446,5 +446,10 @@ void syslog (int a, const char *c, ...)
 
 ssize_t write (int f, const void *b, size_t l)
 {
   return 0;
 }
+
+int printf (char const *fmt, ...)
+{
+  return 0;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c 
b/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
index 6422d61..93e487d 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
@@ -1,10 +1,10 @@
 /** @file
   C Run-Time Libraries (CRT) Time Management Routines Wrapper Implementation
   for OpenSSL-based Cryptographic Library (used in DXE & RUNTIME).
 
-Copyright (c) 2010 - 2011, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.
 This program and the