From 3e06fe42d63856e48c6457dbb7e816b82416c9ca Mon Sep 17 00:00:00 2001

From: Laszlo Ersek <lersek@redhat.com>

Date: Fri, 31 Jan 2020 12:42:44 +0100

Subject: [PATCH 08/12] SecurityPkg/DxeImageVerificationHandler: unnest

 AddImageExeInfo() call

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

RH-Author: Laszlo Ersek <lersek@redhat.com>

Message-id: <20200131124248.22369-9-lersek@redhat.com>

Patchwork-id: 93610

O-Subject: [RHEL-8.2.0 edk2 PATCH 08/12] SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call

Bugzilla: 1751993

RH-Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

RH-Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Before the "Done" label at the end of DxeImageVerificationHandler(), we

now have a single access to "Status": we set "Status" to EFI_ACCESS_DENIED

at the top of the function. Therefore, the (Status != EFI_SUCCESS)

condition is always true under the "Done" label.

Accordingly, unnest the AddImageExeInfo() call dependent on that

condition, remove the condition, and also rename the "Done" label to

"Failed".

Functionally, this patch is a no-op. It's easier to review with:

  git show -b -W

Cc: Chao Zhang <chao.b.zhang@intel.com>

Cc: Jian J Wang <jian.j.wang@intel.com>

Cc: Jiewen Yao <jiewen.yao@intel.com>

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129

Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Message-Id: <20200116190705.18816-8-lersek@redhat.com>

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

[lersek@redhat.com: replace EFI_D_INFO w/ DEBUG_INFO for PatchCheck.py]

[lersek@redhat.com: push with Mike's R-b due to Chinese New Year

 Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid

 <d3fbb76dabed4e1987c512c328c82810@intel.com>]

(cherry picked from commit c602e97446a8e818bf09182f5dc9f3fa409ece95)

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

---

 .../DxeImageVerificationLib.c                      | 34 ++++++++++------------

 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c

index 6ccce1f..51968bd 100644

--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c

+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c

@@ -1676,7 +1676,7 @@ DxeImageVerificationHandler (

     // The information can't be got from the invalid PeImage

     //

     DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: PeImage invalid. Cannot retrieve image information.\n"));

-    goto Done;

+    goto Failed;

   }

   DosHdr = (EFI_IMAGE_DOS_HEADER *) mImageBase;

@@ -1698,7 +1698,7 @@ DxeImageVerificationHandler (

     // It is not a valid Pe/Coff file.

     //

     DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Not a valid PE/COFF image.\n"));

-    goto Done;

+    goto Failed;

   }

   if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {

@@ -1729,7 +1729,7 @@ DxeImageVerificationHandler (

     //

     if (!HashPeImage (HASHALG_SHA256)) {

       DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image using %s.\n", mHashTypeStr));

-      goto Done;

+      goto Failed;

     }

     if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {

@@ -1737,7 +1737,7 @@ DxeImageVerificationHandler (

       // Image Hash is in forbidden database (DBX).

       //

       DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is forbidden by DBX.\n", mHashTypeStr));

-      goto Done;

+      goto Failed;

     }

     if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {

@@ -1751,7 +1751,7 @@ DxeImageVerificationHandler (

     // Image Hash is not found in both forbidden and allowed database.

     //

     DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is not found in DB/DBX.\n", mHashTypeStr));

-    goto Done;

+    goto Failed;

   }

   //

@@ -1860,7 +1860,7 @@ DxeImageVerificationHandler (

     SignatureListSize = sizeof (EFI_SIGNATURE_LIST) + sizeof (EFI_SIGNATURE_DATA) - 1 + mImageDigestSize;

     SignatureList     = (EFI_SIGNATURE_LIST *) AllocateZeroPool (SignatureListSize);

     if (SignatureList == NULL) {

-      goto Done;

+      goto Failed;

     }

     SignatureList->SignatureHeaderSize  = 0;

     SignatureList->SignatureListSize    = (UINT32) SignatureListSize;

@@ -1870,19 +1870,17 @@ DxeImageVerificationHandler (

     CopyMem (Signature->SignatureData, mImageDigest, mImageDigestSize);

   }

-Done:

-  if (Status != EFI_SUCCESS) {

-    //

-    // Policy decides to defer or reject the image; add its information in image executable information table.

-    //

-    NameStr = ConvertDevicePathToText (File, FALSE, TRUE);

-    AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize);

-    if (NameStr != NULL) {

-      DEBUG((EFI_D_INFO, "The image doesn't pass verification: %s\n", NameStr));

-      FreePool(NameStr);

-    }

-    Status = EFI_SECURITY_VIOLATION;

+Failed:

+  //

+  // Policy decides to defer or reject the image; add its information in image executable information table.

+  //

+  NameStr = ConvertDevicePathToText (File, FALSE, TRUE);

+  AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize);

+  if (NameStr != NULL) {

+    DEBUG ((DEBUG_INFO, "The image doesn't pass verification: %s\n", NameStr));

+    FreePool(NameStr);

   }

+  Status = EFI_SECURITY_VIOLATION;

   if (SignatureList != NULL) {

     FreePool (SignatureList);

-- 

1.8.3.1