From 3e06fe42d63856e48c6457dbb7e816b82416c9ca Mon Sep 17 00:00:00 2001 From: Laszlo Ersek 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 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é RH-Acked-by: Vitaly Kuznetsov 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 Cc: Jian J Wang Cc: Jiewen Yao Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129 Signed-off-by: Laszlo Ersek Message-Id: <20200116190705.18816-8-lersek@redhat.com> Reviewed-by: Michael D Kinney [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: ; msgid ] (cherry picked from commit c602e97446a8e818bf09182f5dc9f3fa409ece95) Signed-off-by: Miroslav Rezanina --- .../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