[edk2] [PATCH] CryptoPkg: OpensslLib: reintroduce EFIAPI for ERR_add_error

Post by Laszlo Ersek
Git commit f93f78ea70 (SVN r17633), with subject "CryptoPkg: Update
openssl patch file from 0.9.8zf to 1.0.2c", replaced the file
CryptoPkg/Library/OpensslLib/EDKII_openssl-0.9.8zf.patch
with
CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
In the process, two hunks were lost that used to add EFIAPI to the
declaration of the variadic function ERR_add_error_data().
The VA_START() macro, from "MdePkg/Include/Base.h", expands to an
EFIAPI-dependent implementation when
!defined(__CC_ARM) && (!defined(__GNUC__) ||
defined(NO_BUILTIN_VA_FUNCS))
Under such circumstances, the va_start() macro invocation in
ERR_add_error_data() -- which is translated to VA_START() by
"CryptoPkg/Include/OpenSslSupport.h" -- results in EFIAPI-dependent code,
but callers of the function pass the arguments incorrectly, because the
declaration doesn't state EFIAPI.
This leads to crashes when ERR_add_error_vdata(), called by
ERR_add_error_data(), tries to access the arguments forwarded to it.
Restore the missing hunk from before SVN r17633.
Contributed-under: TianoCore Contribution Agreement 1.0

Yeah, EFIAPI to get the right calling conventions on x86_64 is
definitely the right thing here. Is this what was breaking shim for
you?

It's mildly annoying that EFIAPI is pretty much 100% about ABI calling
conventions and has nothing to do with actual API, but that ship has
long past sailed.

Post by Laszlo Ersek
---
.../Library/OpensslLib/EDKII_openssl-1.0.2c.patch | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
index 54e14d8..0d9575e 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
@@ -344,3 +344,37 @@ diff U3 crypto/opensslconf.h crypto/opensslconf.h
#endif
#if defined(HEADER_RC4_LOCL_H) && !defined(CONFIG_HEADER_RC4_LOCL_H)
+diff U3 crypto/err/err.c crypto/err/err.c
+--- crypto/err/err.c
++++ crypto/err/err.c
+ es->err_data_flags[i] = flags;
+ }
+
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...)
++#else
+ void ERR_add_error_data(int num, ...)
++#endif
+ {
+ va_list args;
+ va_start(args, num);
+diff U3 crypto/err/err.h crypto/err/err.h
+--- crypto/err/err.h
++++ crypto/err/err.h
+ # ifndef OPENSSL_NO_BIO
+ void ERR_print_errors(BIO *bp);
+ # endif
++
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...);
++#else
+ void ERR_add_error_data(int num, ...);
++#endif
++
+ void ERR_add_error_vdata(int num, va_list args);
+ void ERR_load_strings(int lib, ERR_STRING_DATA str[]);
+ void ERR_unload_strings(int lib, ERR_STRING_DATA str[]);
--
1.8.3.1

--
Peter

Peter Jones

2015-06-23 14:00:54 UTC

Post by Peter Jones
Yeah, EFIAPI to get the right calling conventions on x86_64 is
definitely the right thing here. Is this what was breaking shim for
you?

I see from your other mail that it is.

--
Peter

Laszlo Ersek

2015-06-23 14:01:45 UTC

Yeah, EFIAPI to get the right calling conventions on x86_64 is
definitely the right thing here. Is this what was breaking shim for
you?

Yes. The verification of shim, as the first ever signed binary loaded
during boot, consists of several steps, and one of those steps checks DB
for a direct signature (or cert). The very first attempt to validate the
binary fails -- of course a later, alternative, step *would* succeed --,
but when that first attempt fails, the OpenSSL code that tries to store
an error message, using the function being fixed here, blows up.

So everything is fine on my end with this patch in place. The error
message is now successfully stored (and then thrown away silently), and
shim is accepted / verified same as before in the later step.

Post by Peter Jones
It's mildly annoying that EFIAPI is pretty much 100% about ABI calling
conventions and has nothing to do with actual API, but that ship has
long past sailed.

Thanks! :)
Laszlo

David Woodhouse

2015-07-22 21:07:33 UTC

Yeah, EFIAPI to get the right calling conventions on x86_64 is
definitely the right thing here. Is this what was breaking shim for
you?
It's mildly annoying that EFIAPI is pretty much 100% about ABI calling
conventions and has nothing to do with actual API, but that ship has
long past sailed.

Alternatively (as I baulk at trying to send that particular one-liner
upstream to OpenSSL), would it be possible to simply *avoid* defining
NO_BUILTIN_VA_FUNCS for the OpenSSL part of the build?

--
David Woodhouse Open Source Technology Centre
***@intel.com Intel Corporation

Laszlo Ersek

2015-07-22 22:44:45 UTC

Yeah, EFIAPI to get the right calling conventions on x86_64 is
definitely the right thing here. Is this what was breaking shim for
you?
It's mildly annoying that EFIAPI is pretty much 100% about ABI calling
conventions and has nothing to do with actual API, but that ship has
long past sailed.

Alternatively (as I baulk at trying to send that particular one-liner
upstream to OpenSSL), would it be possible to simply *avoid* defining
NO_BUILTIN_VA_FUNCS for the OpenSSL part of the build?

I "guess" it should work at the moment (and -UNO_BUILTIN_VA_FUNCS would
be easy enough to add under [BuildOptions] in
"CryptoPkg/Library/OpensslLib/OpensslLib.inf").

But, as soon as a "genuinely" EFIAPI function with a variable argument
list appeared in "OpensslLib.inf", things would break again.

Any particular reason for disliking these hunks of
"EDKII_openssl-1.0.2d.patch" (over the other hunks)?

... The best thing would be to avoid NO_BUILTIN_VA_FUNCS *completely*.

Post by David Woodhouse
typedef __builtin_va_list VA_LIST;
#define VA_START(Marker, Parameter) __builtin_va_start (Marker, Parameter)
#define VA_ARG(Marker, TYPE) ((sizeof (TYPE) < sizeof (UINTN)) ? (TYPE)(__builtin_va_arg (Marker, UINTN)) : (TYPE)(__builtin_va_arg (Marker, TYPE)))
#define VA_END(Marker) __builtin_va_end (Marker)
#define VA_COPY(Dest, Start) __builtin_va_copy (Dest, Start)

Unfortunately, this is good for *non*-EFIAPI functions only. For EFIAPI
functions, you'd have to define them as __builtin_ms_va* (note the _ms).
Which, in turn, would be good only for EFIAPI functions. (See
"/usr/lib/gcc/*/*/include/cross-stdarg.h". See also
<https://bugzilla.redhat.com/show_bug.cgi?id=871889#c3>.)

So, this issue is not fixable globally under gcc. With gcc (compiling
for x86_64), EFIAPI and non-EFIAPI differ from each other, and the VA_*
macros would have to expand to different gcc builtins dependent *on the
function*, not some global (or even source file level) #define. And,
since the UEFI spec declares a number of functions that take variable
arguments, like InstallMultipleProtocolInterfaces(), we are forced to
stick with EFIAPI as default, and that compels all other varargs
functions (that use the VA_* macros internally) to be EFIAPI too. This
could only be avoided if __builtin_va* considered the containing
function's calling convention automatically, but that's not going to
happen (see bug 871889 again).

... Rather than undefining NO_BUILTIN_VA_FUNCS for openssl, we should
instead remove

Post by David Woodhouse
//
// Map all va_xxxx elements to VA_xxx defined in MdePkg/Include/Base.h
//
#if !defined(__CC_ARM) // if va_list is not already defined
#define va_list VA_LIST
#define va_arg VA_ARG
#define va_start VA_START
#define va_end VA_END

from "CryptoPkg/Include/OpenSslSupport.h" (in the gcc case). Because,
without the VA_* macros (of which OpenSSL itself contains no instances),
there would be no requirement for EFIAPI either. In other words, the
EFIAPI hunks in "EDKII_openssl-1.0.2d.patch" are a solution to a problem
that "CryptoPkg/Include/OpenSslSupport.h" creates.

Thanks
Laszlo

------------------------------------------------------------------------------

David Woodhouse

2015-07-22 23:06:01 UTC

Post by Laszlo Ersek
I "guess" it should work at the moment (and -UNO_BUILTIN_VA_FUNCS would
be easy enough to add under [BuildOptions] in
"CryptoPkg/Library/OpensslLib/OpensslLib.inf").
But, as soon as a "genuinely" EFIAPI function with a variable argument
list appeared in "OpensslLib.inf", things would break again.

I'm slightly confused. Why does it *only* matter for varargs functions?
I thought that on some platforms, the difference between the ELF ABI
and the MS ABI extended to putting arguments in *completely* different
registers? Why doesn't that bite us?

Post by Laszlo Ersek
Any particular reason for disliking these hunks of
"EDKII_openssl-1.0.2d.patch" (over the other hunks)?

Oh, I dislike lots of other bits of that too. I'm just working through
them all separately. Are you on the openssl-dev mailing list? :)

Post by Laszlo Ersek
So, this issue is not fixable globally under gcc. With gcc (compiling
for x86_64), EFIAPI and non-EFIAPI differ from each other,..

Well, why don't we just add -mabi=ms to the compiler flags? Then EFIAPI
in the source code could be a no-op, right?

Alternatively, why in $DEITY's name doesn't GCC give us a set of
builtin functions which actually DTRT according to whether *this*
particular function is being built with the MS ABI or not? Is there a
PR for that already?

--
dwmw2

Scott Duplichan

2015-07-23 00:54:10 UTC

David Woodhouse [mailto:***@infradead.org] wrote:

]Sent: Wednesday, July 22, 2015 06:06 PM
]To: Laszlo Ersek <***@redhat.com>
]Cc: edk2-***@lists.sourceforge.net; ***@host178.hostmonster.com
]Subject: Re: [edk2] [PATCH] CryptoPkg: OpensslLib: reintroduce EFIAPI for ERR_add_error_data()
]
]On Thu, 2015-07-23 at 00:44 +0200, Laszlo Ersek wrote:
]> I "guess" it should work at the moment (and -UNO_BUILTIN_VA_FUNCS would
]> be easy enough to add under [BuildOptions] in
]> "CryptoPkg/Library/OpensslLib/OpensslLib.inf").
]>
]> But, as soon as a "genuinely" EFIAPI function with a variable argument
]> list appeared in "OpensslLib.inf", things would break again.
]
]I'm slightly confused. Why does it *only* matter for varargs functions?
]I thought that on some platforms, the difference between the ELF ABI
]and the MS ABI extended to putting arguments in *completely* different
]registers? Why doesn't that bite us?
]
]> Any particular reason for disliking these hunks of
]> "EDKII_openssl-1.0.2d.patch" (over the other hunks)?
]
]Oh, I dislike lots of other bits of that too. I'm just working through
]them all separately. Are you on the openssl-dev mailing list? :)
]
]> So, this issue is not fixable globally under gcc. With gcc (compiling
]> for x86_64), EFIAPI and non-EFIAPI differ from each other,..
]
]Well, why don't we just add -mabi=ms to the compiler flags? Then EFIAPI
]in the source code could be a no-op, right?

A good question. It was discussed in a 2014 thread. Search for:
"Why not just change the ABI on all functions?"
https://www.mail-archive.com/edk2-***@lists.sourceforge.net/msg10480.html

Adding -mabi=ms results in considerable code size reduction too.

Thanks,
Scott

]Alternatively, why in $DEITY's name doesn't GCC give us a set of
]builtin functions which actually DTRT according to whether *this*
]particular function is being built with the MS ABI or not? Is there a
]PR for that already?
]
]--
]dwmw2

------------------------------------------------------------------------------

Laszlo Ersek

2015-07-23 10:31:42 UTC

It does not bite us because the calling convention is part of the
function's prototype (the declaration in the header file), and the compiler:
- adheres to the calling convention in callers -- arguments are
marshalled accordingly,
- enforces that calling convention when it sees the definition of the
function -- if the definition doesn't match the declaration, the
build breaks with an error. If the definition matches the
declaration, then the arguments are de-marshalled accoringly.

Difference with varargs functions:
- the compiler *does not* enforce in the function definition that the
VA_* macros actually match the calling convention posited in the
function declaration. Arguments are de-marshalled manually, and if we
mess up, the compiler doesn't catch it, we just crash.

Post by Laszlo Ersek
Any particular reason for disliking these hunks of
"EDKII_openssl-1.0.2d.patch" (over the other hunks)?

Oh, I dislike lots of other bits of that too. I'm just working through
them all separately. Are you on the openssl-dev mailing list? :)

Nope. :)

Post by Laszlo Ersek
So, this issue is not fixable globally under gcc. With gcc (compiling
for x86_64), EFIAPI and non-EFIAPI differ from each other,..

Well, why don't we just add -mabi=ms to the compiler flags? Then EFIAPI
in the source code could be a no-op, right?

Yes, I think EFIAPI would be a no-op then. I don't know why we don't set
-mabi=ms globally.

Hm, wait, I do know. Because gcc-4.4 doesn't support it. See the docs:

https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/i386-and-x86_002d64-Options.html

The section "Intel 386 and AMD x86-64 Options" does not list "-mabi=".
Apparently, the "-mabi=(ms|sysv)" option was introduced in gcc-4.5.

... Hm, that might be a glitch in the gcc-4.4 documentation. I just
checked my Fedora 13 VM (with gcc-4.4), and it does know about -mabi=ms.
So, I don't know.

Post by David Woodhouse
Alternatively, why in $DEITY's name doesn't GCC give us a set of
builtin functions which actually DTRT according to whether *this*
particular function is being built with the MS ABI or not?

Yes, exactly, why does it not? I agree with your sentiment 100%.

Post by David Woodhouse
Is there a
PR for that already?

I don't know. I can only refer you (again) to:
<https://bugzilla.redhat.com/show_bug.cgi?id=871889>. Jakub and Kai said
(in 2012) that it was a user error to expect the same builtins (or
macros) to DTRT regardless of the calling convention. They said the
programmer had to choose the right builtins manually.

And, if I check the "stdarg.h" and "cross-stdarg.h" files on my RHEL-7.1
system, under "/usr/lib/gcc/x86_64-redhat-linux/4.8.2/include", I still see:
- "stdarg.h" is bound to the SYSV ABI
- "cross-stdarg.h" is also bound to the SYSV ABI, for !__x86_64__
- "cross-stdarg.h" allows the programmer to manually choose between
SYSV and MS ABI compatible builtins for __x86_64__

So, as far as RHEL-7.1 can be considered "recent", I'd say those
bugzilla comments are the state of the art even today.

In any case, if there was such a gcc PR, we'd still have to stick with
the compatibility cruft in edk2, for old gcc versions' sake.

Thanks
Laszlo

------------------------------------------------------------------------------

Laszlo Ersek

2015-07-23 10:40:29 UTC

Post by David Woodhouse
Well, why don't we just add -mabi=ms to the compiler flags? Then EFIAPI
in the source code could be a no-op, right?

Yes, I think EFIAPI would be a no-op then. I don't know why we don't set
-mabi=ms globally.
https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/i386-and-x86_002d64-Options.html
The section "Intel 386 and AMD x86-64 Options" does not list "-mabi=".
Apparently, the "-mabi=(ms|sysv)" option was introduced in gcc-4.5.
... Hm, that might be a glitch in the gcc-4.4 documentation. I just
checked my Fedora 13 VM (with gcc-4.4), and it does know about -mabi=ms.
So, I don't know.

Sigh. My test was wrong. This is what I checked:

[***@edk2-gcc44-fc13 ~]# gcc -mabi=ms
gcc: no input files

That's a good result, right? Nope. Compare:

[***@edk2-gcc44-fc13 ~]# gcc -mabi=foobar
gcc: no input files

As I said, "sigh". The diagnostic output cannot be trusted here; the
results are undistinguishable between "ms" and "foobar". Compare on gcc-4.8:

$ gcc -mabi=ms
gcc: fatal error: no input files

$ gcc -mabi=foobar
gcc: error: unrecognized argument in option '-mabi=foobar'
gcc: note: valid arguments to '-mabi=' are: ms sysv

So, the documentation of gcc-4.4 is apparently correct, and -mabi is a
silently ignored switch.

Thanks
Laszlo

------------------------------------------------------------------------------

David Woodhouse

2015-07-23 12:17:05 UTC

Post by David Woodhouse
I'm slightly confused. Why does it *only* matter for varargs functions?
I thought that on some platforms, the difference between the ELF ABI
and the MS ABI extended to putting arguments in *completely* different
registers? Why doesn't that bite us?

It does not bite us because the calling convention is part of the
function's prototype (the declaration in the header file),

Right, thanks. Sorry, I'd actually worked this out before I sent my
email but forgot to remove the question.

The problem is *solely* about choosing the right conventions for
va_list handling, according to the ABI. It's *all* about GCC PR#50818.

Post by Laszlo Ersek
Any particular reason for disliking these hunks of
"EDKII_openssl-1.0.2d.patch" (over the other hunks)?

Oh, I dislike lots of other bits of that too. I'm just working through
them all separately. Are you on the openssl-dev mailing list? :)

Nope. :)

Lucky you :)

Post by Laszlo Ersek
In any case, if there was such a gcc PR, we'd still have to stick with
the compatibility cruft in edk2, for old gcc versions' sake.

Right. So what are the options?

1. Push a patch into OpenSSL upstream that marks ERR_add_error_data()
and any other varargs functions with EFIAPI. (Ick, no. I can't
imagine them accepting that requirement.)

2. Maintain the same patch for ourselves in perpetuity. (Ick, we
really don't *want* to be carrying patches, especially like that
one.)

3. Switch to -mabi=ms across the board (can we ditch GCC 4.4?)

4. Fix PR#50818. (Doesn't help until we can require a fixed GCC, in
which case we might as well just go with option #3).

5. Add -UNO_BUILTIN_VA_FUNCS to OpenSSL build flags or otherwise
tweak OpenSslSupport.h to avoid tripping up on it. (As with option
#1 and #2, this might work for OpenSSL but I'd rather have a fix
for the *general* case of non-explicitly-EFIAPI varargs functions.
Those are currently broken on x86_64 ELF builds *anywhere* in
Tianocore, right? Like in the PrintString function in
DuetPkg/DxeIpl/Debug.c, found with a very quick grep...)

Anything I missed?

I'm leaning towards a combination of #3 and #5. Switch to -mabi=ms
where we can, so the whole mess can go away in time. And the minimal
tweak to fix it for OpenSSL builds, although there *are* still issues
elsewhere in the tree.

--
David Woodhouse Open Source Technology Centre
***@intel.com Intel Corporation

Laszlo Ersek

2015-07-23 12:41:10 UTC

It does not bite us because the calling convention is part of the
function's prototype (the declaration in the header file),

Right, thanks. Sorry, I'd actually worked this out before I sent my
email but forgot to remove the question.
The problem is *solely* about choosing the right conventions for
va_list handling, according to the ABI. It's *all* about GCC PR#50818.

Post by Laszlo Ersek
Any particular reason for disliking these hunks of
"EDKII_openssl-1.0.2d.patch" (over the other hunks)?

Oh, I dislike lots of other bits of that too. I'm just working through
them all separately. Are you on the openssl-dev mailing list? :)

Nope. :)

Lucky you :)

Post by Laszlo Ersek
In any case, if there was such a gcc PR, we'd still have to stick with
the compatibility cruft in edk2, for old gcc versions' sake.

Right. So what are the options?
1. Push a patch into OpenSSL upstream that marks ERR_add_error_data()
and any other varargs functions with EFIAPI. (Ick, no. I can't
imagine them accepting that requirement.)
2. Maintain the same patch for ourselves in perpetuity. (Ick, we
really don't *want* to be carrying patches, especially like that
one.)
3. Switch to -mabi=ms across the board (can we ditch GCC 4.4?)
4. Fix PR#50818. (Doesn't help until we can require a fixed GCC, in
which case we might as well just go with option #3).
5. Add -UNO_BUILTIN_VA_FUNCS to OpenSSL build flags or otherwise
tweak OpenSslSupport.h to avoid tripping up on it. (As with option
#1 and #2, this might work for OpenSSL but I'd rather have a fix
for the *general* case of non-explicitly-EFIAPI varargs functions.
Those are currently broken on x86_64 ELF builds *anywhere* in
Tianocore, right? Like in the PrintString function in
DuetPkg/DxeIpl/Debug.c, found with a very quick grep...)
Anything I missed?
I'm leaning towards a combination of #3 and #5. Switch to -mabi=ms
where we can, so the whole mess can go away in time. And the minimal
tweak to fix it for OpenSSL builds, although there *are* still issues
elsewhere in the tree.

I'd go with #3 (phase out gcc-4.4, hardcode -mabi=ms).

I recently experimented with setting up a minimal, local "edk2 build
farm", for the gcc versions supported by edk2. Most of the details are
irrelevant, but I'll share how I picked the OS releases for the builder
virtual machines:
- I went to koji
- for each gcc version in 4.4 through 4.9, I located the *latest* fcXX
distro tag that still shipped that gcc version

The following mapping resulted:
- gcc-4.4 -> fc13
- gcc-4.5 -> fc14
- gcc-4.6 -> fc16
- gcc-4.7 -> fc18
- gcc-4.8 -> fc20
- gcc-4.9 -> fc21

Now, according to wikipedia, Fedora 13 reached End-of-life on
2011-06-04. That's more than four years ago.

Yes, RHEL-6 too provides gcc-4.4. I don't care. The RHEL-6 host kernel
cannot run OVMF with pflash-based, persistent UEFI variables anyway. So
losing the capability to build OVMF wouldn't be such a big problem.

Jordan mentioned (IIRC) that he preferred EFIAPI to make an actual
difference in DEBUG builds, in order to maintain the EFIAPI-cleanness of
the tree. I believe I disagree with that; to me it seems to be a
circular argument.

The tree needs to be EFIAPI-clean *only* if EFIAPI actually makes a
difference, and if (consequently) there's a potential to break stuff by
getting EFIAPI wrong. However, if EFIAPI is a no-op, then this
possibility for breakage falls away, and we shouldn't have to care about
any such cleanness.

As Peter said, the EFIAPI requirement is not a source level (ie. API)
one, it's an ABI one. If we can ensure the ABI compatibility, with
whatever means possible, we're good. A global -mabi=ms setting would
suffice.

... I realize this might be a knee-jerk opinion, but it's mine :)

Thanks
Laszlo

------------------------------------------------------------------------------

Laszlo Ersek

2015-07-23 12:47:50 UTC

Post by David Woodhouse
Right. So what are the options?
1. Push a patch into OpenSSL upstream that marks ERR_add_error_data()
and any other varargs functions with EFIAPI. (Ick, no. I can't
imagine them accepting that requirement.)
2. Maintain the same patch for ourselves in perpetuity. (Ick, we
really don't *want* to be carrying patches, especially like that
one.)
3. Switch to -mabi=ms across the board (can we ditch GCC 4.4?)
4. Fix PR#50818. (Doesn't help until we can require a fixed GCC, in
which case we might as well just go with option #3).
5. Add -UNO_BUILTIN_VA_FUNCS to OpenSSL build flags or otherwise
tweak OpenSslSupport.h to avoid tripping up on it. (As with option
#1 and #2, this might work for OpenSSL but I'd rather have a fix
for the *general* case of non-explicitly-EFIAPI varargs functions.
Those are currently broken on x86_64 ELF builds *anywhere* in
Tianocore, right? Like in the PrintString function in
DuetPkg/DxeIpl/Debug.c, found with a very quick grep...)
Anything I missed?
I'm leaning towards a combination of #3 and #5. Switch to -mabi=ms
where we can, so the whole mess can go away in time. And the minimal
tweak to fix it for OpenSSL builds, although there *are* still issues
elsewhere in the tree.

I'd go with #3 (phase out gcc-4.4, hardcode -mabi=ms).

That's viable for LLVM too, right? It should be.

Andrew?... :)

------------------------------------------------------------------------------

Laszlo Ersek

2015-07-23 13:21:13 UTC

I'd go with #3 (phase out gcc-4.4, hardcode -mabi=ms).

That's viable for LLVM too, right? It should be.

Andrew?... :)

You can add targets to clang to support the ABI you care about. For example the Xcode target for EFI X64 is -target x86_64-pc-win32-macho, as -arch x86_64 implies the System V Application Binary Interface. I don’t think that msabi is supported via the function attribute in clang, it was not a few years ago when we checked.
So it is possible to add custom targets to LLVM to support the ABI/image format you care about. Some one could add -target x86_64-pc-win32-elf for example. Our firmware team added -target x86_64-pc-win32-macho to the open source, so anything is possible. The nice thing about LLVM is you can build a complier that supports the OS ABI and EFI ABI (via a custom target), and not need a customer complier for EFI.
Sorry, so hardcoding abi=ms is how LLVM solves the problem.

If I understand correctly, that's exactly what we'd have liked to hear.
Is that right, David? The "-mabi=ms" flag would be specific to the GCC
toolchains in tools_def.txt. LLVM can take another parameter (or nothing
new at all, actually), the point is, the method used with GCC would be
identical to the one used with LLVM (just the actual command line
options would differ).

Thanks!
Laszlo

------------------------------------------------------------------------------

David Woodhouse

2015-07-23 13:30:27 UTC

Sorry, so hardcoding abi=ms is how LLVM solves the problem.

If I understand correctly, that's exactly what we'd have liked to hear.
Is that right, David?

Right. It basically means that EFIAPI becomes a no-op for *everybody*,
because we can all just use the Microsoft ABI for everything.

And certainly means that the 'Something has to get va_list right
dependent on whether this particular function is EFIAPI or not" problem
goes away, at least for us.

--
dwmw2

Peter Jones

2015-07-23 12:18:16 UTC

Because the compiler doesn't do __attribute__((__ms_abi__)) thunking
when it emits __builtin_va_*() handling code. Though you could look at
that as a gcc bug - i.e. if the function has the attribute, emit
__builtin_ms_va_copy() etc.

At some point I do wonder if we wouldn't be better off making EFIABI an
empty define and building with -mabi=ms . Is there a reason not to
aside from some people's loathing of the less efficient ms abi that got
us in to this mess to begin with?

Post by Laszlo Ersek
Any particular reason for disliking these hunks of
"EDKII_openssl-1.0.2d.patch" (over the other hunks)?

Oh, I dislike lots of other bits of that too. I'm just working through
them all separately. Are you on the openssl-dev mailing list? :)

Post by Laszlo Ersek
So, this issue is not fixable globally under gcc. With gcc (compiling
for x86_64), EFIAPI and non-EFIAPI differ from each other,..

Well, why don't we just add -mabi=ms to the compiler flags? Then EFIAPI
in the source code could be a no-op, right?

... So of course you had the same thought.

I don't know of one, but I haven't looked any harder than you have.

--
Peter

------------------------------------------------------------------------------

David Woodhouse

2015-07-23 12:20:51 UTC

I don't know of one, but I haven't looked any harder than you have.

If you take a look at ix86_fn_abi_va_list() it actually looks like it
*tries* to get it right in some circumstances. It just doesn't seem to
work. So I really do think of this as a compiler bug. PR#50818.

--
dwmw2

Ard Biesheuvel

2015-06-23 14:07:28 UTC

Great that you were able to track this down. It also explains why I
didn't see any of this on AArch64, since we don't have this EFIAPI
issue, fortunately.

Long, Qin

2015-06-23 14:12:27 UTC

Laszlo,

Yes, the EFIAPI definition for ERR_add_error_data() was missed in this updated patch file.
It's cool. Thanks for catching this.

Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Tuesday, June 23, 2015 10:07 PM
To: Laszlo Ersek
Cc: edk2-***@lists.sourceforge.net; Long, Qin; Gary Ching-Pang Lin; Peter Jones
Subject: Re: [PATCH] CryptoPkg: OpensslLib: reintroduce EFIAPI for ERR_add_error_data()

Post by Laszlo Ersek
Git commit f93f78ea70 (SVN r17633), with subject "CryptoPkg: Update
openssl patch file from 0.9.8zf to 1.0.2c", replaced the file
CryptoPkg/Library/OpensslLib/EDKII_openssl-0.9.8zf.patch
with
CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
In the process, two hunks were lost that used to add EFIAPI to the
declaration of the variadic function ERR_add_error_data().
The VA_START() macro, from "MdePkg/Include/Base.h", expands to an
EFIAPI-dependent implementation when
!defined(__CC_ARM) && (!defined(__GNUC__) ||
defined(NO_BUILTIN_VA_FUNCS))
Under such circumstances, the va_start() macro invocation in
ERR_add_error_data() -- which is translated to VA_START() by
"CryptoPkg/Include/OpenSslSupport.h" -- results in EFIAPI-dependent
code, but callers of the function pass the arguments incorrectly,
because the declaration doesn't state EFIAPI.
This leads to crashes when ERR_add_error_vdata(), called by
ERR_add_error_data(), tries to access the arguments forwarded to it.
Restore the missing hunk from before SVN r17633.
Contributed-under: TianoCore Contribution Agreement 1.0

Great that you were able to track this down. It also explains why I didn't see any of this on AArch64, since we don't have this EFIAPI issue, fortunately.

Post by Laszlo Ersek
---
.../Library/OpensslLib/EDKII_openssl-1.0.2c.patch | 34
++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
index 54e14d8..0d9575e 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
@@ -344,3 +344,37 @@ diff U3 crypto/opensslconf.h crypto/opensslconf.h
#endif
#if defined(HEADER_RC4_LOCL_H) && !defined(CONFIG_HEADER_RC4_LOCL_H)
+diff U3 crypto/err/err.c crypto/err/err.c
+--- crypto/err/err.c
++++ crypto/err/err.c
+ es->err_data_flags[i] = flags;
+ }
+
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...) #else
+ void ERR_add_error_data(int num, ...)
++#endif
+ {
+ va_list args;
+ va_start(args, num);
+diff U3 crypto/err/err.h crypto/err/err.h
+--- crypto/err/err.h
++++ crypto/err/err.h
+OPENSSL_NO_BIO void ERR_print_errors(BIO *bp); # endif
++
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...); #else
+ void ERR_add_error_data(int num, ...);
++#endif
++
+ void ERR_add_error_vdata(int num, va_list args); void
+ ERR_load_strings(int lib, ERR_STRING_DATA str[]); void
+ ERR_unload_strings(int lib, ERR_STRING_DATA str[]);
--
1.8.3.1

Long, Qin

2015-06-23 14:16:26 UTC

Reviewed-by: Qin Long <***@intel.com>

Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: Long, Qin [mailto:***@intel.com]
Sent: Tuesday, June 23, 2015 10:12 PM
To: Ard Biesheuvel; Laszlo Ersek
Cc: edk2-***@lists.sourceforge.net
Subject: Re: [edk2] [PATCH] CryptoPkg: OpensslLib: reintroduce EFIAPI for ERR_add_error_data()

Laszlo,

Yes, the EFIAPI definition for ERR_add_error_data() was missed in this updated patch file.
It's cool. Thanks for catching this.

Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Tuesday, June 23, 2015 10:07 PM
To: Laszlo Ersek
Cc: edk2-***@lists.sourceforge.net; Long, Qin; Gary Ching-Pang Lin; Peter Jones
Subject: Re: [PATCH] CryptoPkg: OpensslLib: reintroduce EFIAPI for ERR_add_error_data()

Post by Laszlo Ersek
Git commit f93f78ea70 (SVN r17633), with subject "CryptoPkg: Update
openssl patch file from 0.9.8zf to 1.0.2c", replaced the file
CryptoPkg/Library/OpensslLib/EDKII_openssl-0.9.8zf.patch
with
CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
In the process, two hunks were lost that used to add EFIAPI to the
declaration of the variadic function ERR_add_error_data().
The VA_START() macro, from "MdePkg/Include/Base.h", expands to an
EFIAPI-dependent implementation when
!defined(__CC_ARM) && (!defined(__GNUC__) ||
defined(NO_BUILTIN_VA_FUNCS))
Under such circumstances, the va_start() macro invocation in
ERR_add_error_data() -- which is translated to VA_START() by
"CryptoPkg/Include/OpenSslSupport.h" -- results in EFIAPI-dependent
code, but callers of the function pass the arguments incorrectly,
because the declaration doesn't state EFIAPI.
This leads to crashes when ERR_add_error_vdata(), called by
ERR_add_error_data(), tries to access the arguments forwarded to it.
Restore the missing hunk from before SVN r17633.
Contributed-under: TianoCore Contribution Agreement 1.0

Great that you were able to track this down. It also explains why I didn't see any of this on AArch64, since we don't have this EFIAPI issue, fortunately.

Post by Laszlo Ersek
---
.../Library/OpensslLib/EDKII_openssl-1.0.2c.patch | 34
++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
index 54e14d8..0d9575e 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
@@ -344,3 +344,37 @@ diff U3 crypto/opensslconf.h crypto/opensslconf.h
#endif
#if defined(HEADER_RC4_LOCL_H) && !defined(CONFIG_HEADER_RC4_LOCL_H)
+diff U3 crypto/err/err.c crypto/err/err.c
+--- crypto/err/err.c
++++ crypto/err/err.c
+ es->err_data_flags[i] = flags;
+ }
+
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...) #else
+ void ERR_add_error_data(int num, ...)
++#endif
+ {
+ va_list args;
+ va_start(args, num);
+diff U3 crypto/err/err.h crypto/err/err.h
+--- crypto/err/err.h
++++ crypto/err/err.h
+OPENSSL_NO_BIO void ERR_print_errors(BIO *bp); # endif
++
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...); #else
+ void ERR_add_error_data(int num, ...);
++#endif
++
+ void ERR_add_error_vdata(int num, va_list args); void
+ ERR_load_strings(int lib, ERR_STRING_DATA str[]); void
+ ERR_unload_strings(int lib, ERR_STRING_DATA str[]);
--
1.8.3.1

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Laszlo Ersek

2015-06-23 15:00:07 UTC

Thanks for the reviews. Since this issue breaks secure boot builds,
generally speaking, and because the pkg maintainer reviewed the patch, I
committed it at once: SVN r17689.

I hope that Gary won't mind my doing that before his feedback. (In
retrospect the fix should be quite non-controversial; it just restores
two earlier hunks, with changed pathnames.)

Thanks!
Laszlo

Post by Long, Qin
Best Regards & Thanks,
LONG, Qin
-----Original Message-----
Sent: Tuesday, June 23, 2015 10:12 PM
To: Ard Biesheuvel; Laszlo Ersek
Subject: Re: [edk2] [PATCH] CryptoPkg: OpensslLib: reintroduce EFIAPI for ERR_add_error_data()
Laszlo,
Yes, the EFIAPI definition for ERR_add_error_data() was missed in this updated patch file.
It's cool. Thanks for catching this.
Best Regards & Thanks,
LONG, Qin
-----Original Message-----
Sent: Tuesday, June 23, 2015 10:07 PM
To: Laszlo Ersek
Subject: Re: [PATCH] CryptoPkg: OpensslLib: reintroduce EFIAPI for ERR_add_error_data()

Post by Laszlo Ersek
Git commit f93f78ea70 (SVN r17633), with subject "CryptoPkg: Update
openssl patch file from 0.9.8zf to 1.0.2c", replaced the file
CryptoPkg/Library/OpensslLib/EDKII_openssl-0.9.8zf.patch
with
CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
In the process, two hunks were lost that used to add EFIAPI to the
declaration of the variadic function ERR_add_error_data().
The VA_START() macro, from "MdePkg/Include/Base.h", expands to an
EFIAPI-dependent implementation when
!defined(__CC_ARM) && (!defined(__GNUC__) ||
defined(NO_BUILTIN_VA_FUNCS))
Under such circumstances, the va_start() macro invocation in
ERR_add_error_data() -- which is translated to VA_START() by
"CryptoPkg/Include/OpenSslSupport.h" -- results in EFIAPI-dependent
code, but callers of the function pass the arguments incorrectly,
because the declaration doesn't state EFIAPI.
This leads to crashes when ERR_add_error_vdata(), called by
ERR_add_error_data(), tries to access the arguments forwarded to it.
Restore the missing hunk from before SVN r17633.
Contributed-under: TianoCore Contribution Agreement 1.0

Great that you were able to track this down. It also explains why I didn't see any of this on AArch64, since we don't have this EFIAPI issue, fortunately.

Post by Laszlo Ersek
---
.../Library/OpensslLib/EDKII_openssl-1.0.2c.patch | 34
++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
index 54e14d8..0d9575e 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
@@ -344,3 +344,37 @@ diff U3 crypto/opensslconf.h crypto/opensslconf.h
#endif
#if defined(HEADER_RC4_LOCL_H) && !defined(CONFIG_HEADER_RC4_LOCL_H)
+diff U3 crypto/err/err.c crypto/err/err.c
+--- crypto/err/err.c
++++ crypto/err/err.c
+ es->err_data_flags[i] = flags;
+ }
+
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...) #else
+ void ERR_add_error_data(int num, ...)
++#endif
+ {
+ va_list args;
+ va_start(args, num);
+diff U3 crypto/err/err.h crypto/err/err.h
+--- crypto/err/err.h
++++ crypto/err/err.h
+OPENSSL_NO_BIO void ERR_print_errors(BIO *bp); # endif
++
++/* Add EFIAPI for UEFI version. */
++#if defined(OPENSSL_SYS_UEFI)
++void EFIAPI ERR_add_error_data(int num, ...); #else
+ void ERR_add_error_data(int num, ...);
++#endif
++
+ void ERR_add_error_vdata(int num, va_list args); void
+ ERR_load_strings(int lib, ERR_STRING_DATA str[]); void
+ ERR_unload_strings(int lib, ERR_STRING_DATA str[]);
--
1.8.3.1

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Gary Ching-Pang Lin

2015-06-24 02:07:55 UTC