From patchwork Tue Mar 21 13:49:08 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 95633 Delivered-To: patch@linaro.org Received: by 10.140.89.233 with SMTP id v96csp1448836qgd; Tue, 21 Mar 2017 06:49:51 -0700 (PDT) X-Received: by 10.98.33.155 with SMTP id o27mr18549184pfj.5.1490104191745; Tue, 21 Mar 2017 06:49:51 -0700 (PDT) Return-Path: Received: from ml01.01.org (ml01.01.org. [2001:19d0:306:5::1]) by mx.google.com with ESMTPS id 38si21363410pld.187.2017.03.21.06.49.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 21 Mar 2017 06:49:51 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 2001:19d0:306:5::1 as permitted sender) client-ip=2001:19d0:306:5::1; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 2001:19d0:306:5::1 as permitted sender) smtp.mailfrom=edk2-devel-bounces@lists.01.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id C9A0F80405; Tue, 21 Mar 2017 06:49:50 -0700 (PDT) X-Original-To: edk2-devel@lists.01.org Delivered-To: edk2-devel@lists.01.org Received: from mail-wr0-x232.google.com (mail-wr0-x232.google.com [IPv6:2a00:1450:400c:c0c::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9D95A803FD for ; Tue, 21 Mar 2017 06:49:49 -0700 (PDT) Received: by mail-wr0-x232.google.com with SMTP id u108so112353647wrb.3 for ; Tue, 21 Mar 2017 06:49:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id; bh=yhB0AC8BvUOGx+/mpdDRyLLj2+CX8IhMdKj6ym8oe2M=; b=BW0YB/HQweFwT43JnBBrBeX0+PPBqJp7NV9rBBmN/iyTP5nDDbyYIIsGY+oO50JJiF RRY+N3hMRLdZ88gmaoG2pGgKm9+6nZrxmjJqfdr1aclvbg3Uzvo688VzG6lbwbUx5Dpw SciNyyHph8/3dn59aU1NNknIr0jbFNJfiXFsM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=yhB0AC8BvUOGx+/mpdDRyLLj2+CX8IhMdKj6ym8oe2M=; b=DzKIEVQ/7TXUJNDpieSuXH3e90PpMJ0cPArEy82YUbY6Oov1ZvQzBOUsht+ipWn1EZ yYF82o7cVXZRl5080DzYVhTC2JLDGqSNdvCzLx4UnsqKk6uBk9qa0CmghmR8pCg8XfKd LEHcWGw1Zd0rmb/fn4HQx6sEm9Gcnly0p9oiQ7MUSHn7mYkAQZF8giFkpHVwlVEglYAC +fGB9dmAD2XrN6lpebOHK3eYMSGOx/m+6h4vY7T2aeErqcUll5oTySzh3651jHahfBNW ZI13gnSW5np5X8jSpjiwt+QeZd4HB/8t6ZZmqO1jLjIu5L7htrRl1pUR1VpskYm+HzjA FGUQ== X-Gm-Message-State: AFeK/H0hN0dFTKFnd20yGdlEGFeE2/a2/Zvx4+4b90XwgrYrfEGJ3faUmbZsAaHvfNDFSXsq X-Received: by 10.223.172.77 with SMTP id v71mr35191310wrc.131.1490104187782; Tue, 21 Mar 2017 06:49:47 -0700 (PDT) Received: from localhost.localdomain (134.21.90.92.rev.sfr.net. [92.90.21.134]) by smtp.gmail.com with ESMTPSA id m7sm5744294wmi.34.2017.03.21.06.49.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 21 Mar 2017 06:49:46 -0700 (PDT) From: Ard Biesheuvel To: edk2-devel@lists.01.org, jiewen.yao@Intel.com Date: Tue, 21 Mar 2017 13:49:08 +0000 Message-Id: <1490104148-4193-1-git-send-email-ard.biesheuvel@linaro.org> X-Mailer: git-send-email 2.7.4 Subject: [edk2] [PATCH] MdeModulePkg/MemoryProtection: split protect and unprotect paths X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: feng.tian@Intel.com, liming.gao@intel.com, star.zeng@intel.com, Ard Biesheuvel MIME-Version: 1.0 Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" Currently, the PE/COFF image memory protection code uses the same code paths for protecting and unprotecting an image. This is strange, since unprotecting an image involves a single call into the CPU arch protocol to clear the permission attributes of the entire range, and there is no need to parse the PE/COFF headers again. So let's store the ImageRecord entries in a linked list, so we can find it again at unprotect time, and simply clear the permissions. Note that this fixes a DEBUG hang on an ASSERT() that occurs when the PE/COFF image fails to load, which causes UnprotectUefiImage() to be invoked before the image is fully loaded. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel --- This is an alternative fix for the issue addressed by patch 'MdeModulePkg/DxeCore: ignore PdbPointer if ImageAddress == 0' (https://lists.01.org/pipermail/edk2-devel/2017-March/008766.html) MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 85 ++++++++------------ 1 file changed, 35 insertions(+), 50 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel Reviewed-by: Jiewen.yao@Intel.com diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c index 451cc35b9290..93f96f0c9502 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c @@ -74,6 +74,8 @@ UINT32 mImageProtectionPolicy; extern LIST_ENTRY mGcdMemorySpaceMap; +STATIC LIST_ENTRY mProtectedImageRecordList; + /** Sort code section in image record, based upon CodeSegmentBase from low to high. @@ -238,13 +240,10 @@ SetUefiImageMemoryAttributes ( Set UEFI image protection attributes. @param[in] ImageRecord A UEFI image record - @param[in] Protect TRUE: Protect the UEFI image. - FALSE: Unprotect the UEFI image. **/ VOID SetUefiImageProtectionAttributes ( - IN IMAGE_PROPERTIES_RECORD *ImageRecord, - IN BOOLEAN Protect + IN IMAGE_PROPERTIES_RECORD *ImageRecord ) { IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection; @@ -253,7 +252,6 @@ SetUefiImageProtectionAttributes ( LIST_ENTRY *ImageRecordCodeSectionList; UINT64 CurrentBase; UINT64 ImageEnd; - UINT64 Attribute; ImageRecordCodeSectionList = &ImageRecord->CodeSegmentList; @@ -276,29 +274,19 @@ SetUefiImageProtectionAttributes ( // // DATA // - if (Protect) { - Attribute = EFI_MEMORY_XP; - } else { - Attribute = 0; - } SetUefiImageMemoryAttributes ( CurrentBase, ImageRecordCodeSection->CodeSegmentBase - CurrentBase, - Attribute + EFI_MEMORY_XP ); } // // CODE // - if (Protect) { - Attribute = EFI_MEMORY_RO; - } else { - Attribute = 0; - } SetUefiImageMemoryAttributes ( ImageRecordCodeSection->CodeSegmentBase, ImageRecordCodeSection->CodeSegmentSize, - Attribute + EFI_MEMORY_RO ); CurrentBase = ImageRecordCodeSection->CodeSegmentBase + ImageRecordCodeSection->CodeSegmentSize; } @@ -310,15 +298,10 @@ SetUefiImageProtectionAttributes ( // // DATA // - if (Protect) { - Attribute = EFI_MEMORY_XP; - } else { - Attribute = 0; - } SetUefiImageMemoryAttributes ( CurrentBase, ImageEnd - CurrentBase, - Attribute + EFI_MEMORY_XP ); } return ; @@ -401,18 +384,15 @@ FreeImageRecord ( } /** - Protect or unprotect UEFI image common function. + Protect UEFI PE/COFF image @param[in] LoadedImage The loaded image protocol @param[in] LoadedImageDevicePath The loaded image device path protocol - @param[in] Protect TRUE: Protect the UEFI image. - FALSE: Unprotect the UEFI image. **/ VOID -ProtectUefiImageCommon ( +ProtectUefiImage ( IN EFI_LOADED_IMAGE_PROTOCOL *LoadedImage, - IN EFI_DEVICE_PATH_PROTOCOL *LoadedImageDevicePath, - IN BOOLEAN Protect + IN EFI_DEVICE_PATH_PROTOCOL *LoadedImageDevicePath ) { VOID *ImageAddress; @@ -617,35 +597,18 @@ ProtectUefiImageCommon ( // // CPU ARCH present. Update memory attribute directly. // - SetUefiImageProtectionAttributes (ImageRecord, Protect); + SetUefiImageProtectionAttributes (ImageRecord); // - // Clean up + // Record the image record in the list so we can undo the protections later // - FreeImageRecord (ImageRecord); + InsertTailList (&mProtectedImageRecordList, &ImageRecord->Link); Finish: return ; } /** - Protect UEFI image. - - @param[in] LoadedImage The loaded image protocol - @param[in] LoadedImageDevicePath The loaded image device path protocol -**/ -VOID -ProtectUefiImage ( - IN EFI_LOADED_IMAGE_PROTOCOL *LoadedImage, - IN EFI_DEVICE_PATH_PROTOCOL *LoadedImageDevicePath - ) -{ - if (PcdGet32(PcdImageProtectionPolicy) != 0) { - ProtectUefiImageCommon (LoadedImage, LoadedImageDevicePath, TRUE); - } -} - -/** Unprotect UEFI image. @param[in] LoadedImage The loaded image protocol @@ -657,8 +620,28 @@ UnprotectUefiImage ( IN EFI_DEVICE_PATH_PROTOCOL *LoadedImageDevicePath ) { + IMAGE_PROPERTIES_RECORD *ImageRecord; + LIST_ENTRY *ImageRecordLink; + if (PcdGet32(PcdImageProtectionPolicy) != 0) { - ProtectUefiImageCommon (LoadedImage, LoadedImageDevicePath, FALSE); + for (ImageRecordLink = mProtectedImageRecordList.ForwardLink; + ImageRecordLink != &mProtectedImageRecordList; + ImageRecordLink = ImageRecordLink->ForwardLink) { + ImageRecord = CR ( + ImageRecordLink, + IMAGE_PROPERTIES_RECORD, + Link, + IMAGE_PROPERTIES_RECORD_SIGNATURE + ); + + if (ImageRecord->ImageBase == (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase) { + SetUefiImageMemoryAttributes (ImageRecord->ImageBase, + ImageRecord->ImageSize, + 0); + FreeImageRecord (ImageRecord); + return; + } + } } } @@ -1027,6 +1010,8 @@ CoreInitializeMemoryProtection ( mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy); + InitializeListHead (&mProtectedImageRecordList); + // // Sanity check the PcdDxeNxMemoryProtectionPolicy setting: // - code regions should have no EFI_MEMORY_XP attribute