From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x236.google.com (mail-io0-x236.google.com [IPv6:2607:f8b0:4001:c06::236]) (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 EE2B3803D2 for ; Tue, 21 Mar 2017 14:30:37 -0700 (PDT) Received: by mail-io0-x236.google.com with SMTP id l7so56232933ioe.3 for ; Tue, 21 Mar 2017 14:30:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=uj/xrrd1tsU+CEnkUQjN0mlHP//MXYkThSmS0d4umwI=; b=K+N2rCn/PjPbIOauMkFhq+ECFvogfRL6cAXyJFvskrjEIYm1PpchM3rUaYd6oX5t2w eCNON971QA5fl+gkNHZm0ESW4x5MBklZOUqxhum6zonKEtH3jwPLcVLHfTG24RMHxwmP PB1/UfEQw7XJt7nJPSeiGTH6sO8KyMF4LGQ38= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=uj/xrrd1tsU+CEnkUQjN0mlHP//MXYkThSmS0d4umwI=; b=pSL2QSN4WoqwCKzmM/Mp2HLzq0yUKeVqBPUgfvD94jX0wOyIpEdyxmYuBpq9mK3rON uJ6BXD+THTS7C5q2ZJmCIwolUUBO1zLSkfoYssfeqmwa7MmWlhpAh7umQ8Z/7kSc+8CB iK2xQULH4S1ud8Rlu4TpKTukWNFSp03lvKcIpHvUlcMkVuB70M3NT6C6L1snX+KE8MTn JFiyU6lktrdr6ujUZ1W4SSAx7wbwrmu0bq5v9mclmMgipb9gAPVOyNFI2DDTe3sbyk9X vec4K3nWHsUEMVymUAUozXadrBkI3A5o6vdM83ncPy553vGmSnlgK/ceNq1zuSGQrzw5 0z9A== X-Gm-Message-State: AFeK/H3ZHwsJMI51KBQb/Z7zp6n13T8DQwrMZe5ox4k9ox5A5+tStp5fzqqxL0XeNayN0xbwlQwKsS4yQzSEUyio X-Received: by 10.107.168.21 with SMTP id r21mr31665641ioe.45.1490131837272; Tue, 21 Mar 2017 14:30:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 21 Mar 2017 14:30:36 -0700 (PDT) In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503A907F2F@shsmsx102.ccr.corp.intel.com> References: <1490104148-4193-1-git-send-email-ard.biesheuvel@linaro.org> <74D8A39837DF1E4DA445A8C0B3885C503A907F2F@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Tue, 21 Mar 2017 21:30:36 +0000 Message-ID: To: "Yao, Jiewen" Cc: "edk2-devel@lists.01.org" , "Zeng, Star" , "Tian, Feng" , "Gao, Liming" Subject: Re: [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: , X-List-Received-Date: Tue, 21 Mar 2017 21:30:38 -0000 Content-Type: text/plain; charset=UTF-8 On 21 March 2017 at 14:14, Yao, Jiewen wrote: > Good idea. > > Reviewed-by: Jiewen.yao@Intel.com > Pushed, thanks. > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Tuesday, March 21, 2017 9:49 PM >> To: edk2-devel@lists.01.org; Yao, Jiewen >> Cc: Zeng, Star ; Tian, Feng ; Gao, >> Liming ; Ard Biesheuvel >> Subject: [PATCH] MdeModulePkg/MemoryProtection: split protect and >> unprotect paths >> >> 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(-) >> >> 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 >> -- >> 2.7.4 >