On 06/23/15 10:43, Gary Ching-Pang Lin wrote:
> On Tue, Jun 23, 2015 at 10:07:36AM +0200, Laszlo Ersek wrote:
>> On 06/23/15 04:25, Gary Ching-Pang Lin wrote:
>>> On Mon, Jun 22, 2015 at 02:24:55PM -0400, Peter Jones wrote:
>>>> On Sat, Jun 20, 2015 at 03:01:17PM +0200, Ard Biesheuvel wrote:
>>>>
>>>>> I wonder what is going on here. My AArch64 boot tests work fine
>>>>> with these patches applied, but they don't use shim. (They do use
>>>>> GRUB as an intermediate loader calling LoadImage() to boot a
>>>>> signed kernel).
>>>>>
>>>>> Are there any plans or patches yet to move shim to a more recent
>>>>> OpenSSL version? It shouldn't be affecting things like this, but
>>>>> it would allow a quick check if someone has patches already.
>>>>
>>>> Yes, there's a plan to do so - Gary Lin has had a patch in progress
>>>> and was waiting for this patch to hit before sending it to me.  I
>>>> expect to see it any time.  (I would not be surprised if he's
>>>> trying to debug an analog to this exact same issue...)
>>>>
>>> I'm currently busy with other things so the update in shim may be
>>> delayed for a while.
>>>
>>>> That said, it's unclear to me how shim being on a prior openssl
>>>> version would cause the problem Laszlo is seeing - there's no
>>>> cross-linkage of any kind between the two openssl builds in memory.
>>>>
>>> shim and grub2 are using the openssl lib independent from the one in
>>> firmware, so it surprised me the openssl update patches broke the
>>> bootloaders. I just tested OVMF R17650 with openSUSE 13.2 and
>>> everything went well. The shim we use in openSUSE 13.2 is 0.7 + a
>>> series of patches (most of them are upstream patches). Hope this
>>> could narrow down the issue.
>>
>> Huh. R17650 is past the openssl-1.0.2c update.
>>
>> Can you please give me (or link for me) the install media for
>> openSUSE 13.2? I'd like to try it.
>>
> openSUSE 13.2 is available in:
> https://software.opensuse.org/132/
>
> Maybe you could download the shim first and test it in a vfat dir
> (ex: -hda fat:hda-contents/)? Just put shim.efi and MokManager.efi
> in the same directory, and shim will load MokManager when there is
> no grub2. If it works, then put grub.efi in the same directory and
> see whether the grub console shows or not. It would be easier to
> identify the problem.
>
> The openSUSE shim is available in
> https://build.opensuse.org/package/binaries/openSUSE:13.2/shim?repository=standard

Thanks, I tested this. It fails the same way for me as my earlier test.

Luckily though, I happened to have the serial console of the VM open at
this point, and I got the following exception dump there:

  FS1:\opensuse\usr\lib64\efi\> shim.efi
  !!!! X64 Exception Type - 000000000000000D     CPU Apic ID - 00000000 !!!!
  RIP  - 000000001FDEFDF3, CS  - 0000000000000028, RFLAGS - 0000000000010206
  ExceptionData - 0000000000000000
  RAX  - 1FF5FF7400000000, RCX - 1FF5FF7400000000, RDX - 0000000000000004
  RBX  - 000000001FDF430E, RSP - 000000001FF5FEB0, RBP - 000000001FF5FEE0
  RSI  - 0000000000000000, RDI - 0000000000000051
  R8   - 000000001EE66458, R9  - 000000001EE66458, R10 - 00000000FFFFFFFF
  R11  - 0000000000000000, R12 - 0000000000000000, R13 - 0000000000000000
  R14  - 0000000000000000, R15 - 0000000000000000
  DS   - 0000000000000008, ES  - 0000000000000008, FS  - 0000000000000008
  GS   - 0000000000000008, SS  - 0000000000000008
  CR0  - 0000000080000033, CR2 - 0000000000000000, CR3 - 000000001FEFF000
  CR4  - 0000000000000668, CR8 - 0000000000000000
  DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
  DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
  GDTR - 000000001FEED7D8 000000000000003F, LDTR - 0000000000000000
  IDTR - 000000001F712018 0000000000000FFF,   TR - 0000000000000000
  FXSAVE_STATE - 000000001FF5FB10
  !!!! Find PE image 
.../Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe/DEBUG/SecurityStubDxe.dll
 (ImageBase=000000001FDEA000, EntryPoint=000000001FDEA2AF) !!!!

I quickly repeated my original, Fedora-based test (now looking at the
serial console too), and it fails with the same exception.

The crash itself seems to be inside AsciiStrLen() -- based on RIP,
ImageBase, EntryPoint --, but I don't see the call stack that leads up
to it.

...

Okay, after a few more hours of struggle, I found the bug. It's the
usual VA_START <-> EFIAPI problem.

As discussed earlier (on several occasions), in practice *all* code in
edk2 that uses VA_START() *must* be EFIAPI. Refer to the third
VA_START() macro definition in "MdePkg/Include/Base.h". That macro
definition *depends* on the EFIAPI calling convention. This is violated
by the openssl library.

The call stack that I reconstructed (with manual debug messages) goes
like this, when edk2 tries to validate the shim binary:

Security2StubAuthenticate()            
[MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c]
  ExecuteSecurity2Handlers()           
[MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c]
    // via function pointer
    // "Security2Handler"
    DxeImageVerificationHandler        
[SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c]
      IsAllowedByDb()                  
[SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c]
        AuthenticodeVerify()           
[CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c]
          Pkcs7Verify()                
[CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c]
            PKCS7_verify()             
[CryptoPkg/Library/OpensslLib/openssl-1.0.2c/crypto/pkcs7/pk7_smime.c]
              ERR_add_error_data()     
[CryptoPkg/Library/OpensslLib/openssl-1.0.2c/crypto/err/err.c]
                ERR_add_error_vdata()  
[CryptoPkg/Library/OpensslLib/openssl-1.0.2c/crypto/err/err.c]
                  strlen()             BOOM

More closely, look at the following in the PKCS7_verify() function:

   323              i = X509_verify_cert(&cert_ctx);
   324              if (i <= 0)
   325                  j = X509_STORE_CTX_get_error(&cert_ctx);
   326              X509_STORE_CTX_cleanup(&cert_ctx);
   327              if (i <= 0) {
   328                  PKCS7err(PKCS7_F_PKCS7_VERIFY,
   329                           PKCS7_R_CERTIFICATE_VERIFY_ERROR);
   330                  ERR_add_error_data(2, "Verify error:",
   331                                     X509_verify_cert_error_string(j));
   332                  sk_X509_free(signers);
   333                  return 0;
   334              }

The verification step on line 323 fails. (I don't know why, but I guess
shim.efi could still be accepted by other means -- we are only inside
IsAllowedByDb(), and there are other possibilities for a binary to pass
the check.)

In any case, the variable "j" is assigned. The error message that "j"
corresponds to is:

  unable to get local issuer certificate

I printed this error string separately, and it is all right per se. The
function call that blows up is on line 330, the ERR_add_error_data()
one.

Namely:

  1075  void ERR_add_error_data(int num, ...)
  1076  {
  1077      va_list args;
  1078      va_start(args, num);
  1079      ERR_add_error_vdata(num, args);
  1080      va_end(args);
  1081  }

This function uses va_start(), which, according to
"CryptoPkg/Include/OpenSslSupport.h", gets turned into VA_START().
However, the function does not apply the EFIAPI calling convention, and
that causes it to blow up.

In PKCS7_verify(), before the call to ERR_add_error_data(), I extracted
the string "Verify error:" as:

    STATIC CONST CHAR8 err1[] = "Verify error:";

and then I printed the *address* of err1, before passing err1 to
ERR_add_error_data(). It was: 0x3FE6CC98, a valid address.

Inside ERR_add_error_data() --> ERR_add_error_vdata(), we have:

  1083  void ERR_add_error_vdata(int num, va_list args)
  1084  {
  1085      int i, n, s;
  1086      char *str, *p, *a;
  1087
  1088      s = 80;
  1089      str = OPENSSL_malloc(s + 1);
  1090      if (str == NULL)
  1091          return;
  1092      str[0] = '\0';
  1093
  1094      n = 0;
  1095      for (i = 0; i < num; i++) {
  1096          a = va_arg(args, char *);
  1097          /* ignore NULLs, thanks to Bob Beck <b...@obtuse.com> */
  1098          if (a != NULL) {
  1099              n += strlen(a);

The function retrieves the address of the string, and calls strlen() on
it -- which corresponds to AsciiStrLen(), again according to
OpenSslSupport.h.

Now, just before the call to strlen(), I printed the address stored "a"
*again* -- and it was 0x3FF6_0474_0000_0000.

That address is consistent with the exception dump I'm getting right
after on the serial console; the address is visible in both RAX and RCX.
Plus, the call stack that I tracked down is consistent with the
exception RIP identifying AsciiStrLen().

Summary: while storing the error message from the X509 certificate
verification, things blow up, because ERR_add_error_data() is a varargs
function, but lacks EFIAPI, hence turns its arguments into garbage.

Now, why did this use to work with 0.9.8zf? Easy; just compare the
following two files:

- CryptoPkg/Library/OpensslLib/openssl-0.9.8zf/crypto/err/err.c
- CryptoPkg/Library/OpensslLib/openssl-1.0.2c/crypto/err/err.c

In 0.9.8zf, the prototype of the ERR_add_error_data() function looks
like this:

   324  /* Add EFIAPI for UEFI version. */
   325  #if defined(OPENSSL_SYS_UEFI)
   326  void EFIAPI ERR_add_error_data(int num, ...)
   327  #else
   328  void ERR_add_error_data(int num, ...)
   329  #endif
   330  {
   331      va_list args;
   332      int i, n, s;
   333      char *str, *p, *a;

And that's gone in 1.0.2c.

The bug is in:

commit f93f78ea70f14f0ca17e122588b85f947adfa569
Author: Qin Long <qin.l...@intel.com>
Date:   Tue Jun 16 00:52:17 2015 +0000

    CryptoPkg: Update openssl patch file from 0.9.8zf to 1.0.2c

    This patch adds a patch file for openssl-1.0.2c, and removes
    the patch file for openssl-0.9.8zf.

    Contributed-under: TianoCore Contribution Agreement 1.0
    Signed-off-by: Qin Long <qin.l...@intel.com>
    Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>

    git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@17633 
6f19259b-4bc3-4df7-8a09-765794883524

Because

  CryptoPkg/Library/OpensslLib/EDKII_openssl-0.9.8zf.patch

used to have
a hunk that added the EFIAPI declspec to ERR_add_error_data(), but

  CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch

dropped that hunk.

I'll post a patch soon.

Thanks
Laszlo

------------------------------------------------------------------------------
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to