From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 28D17803FD for ; Tue, 21 Mar 2017 07:15:07 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga105.fm.intel.com with ESMTP; 21 Mar 2017 07:15:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,198,1486454400"; d="scan'208";a="63157869" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga002.jf.intel.com with ESMTP; 21 Mar 2017 07:15:05 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 21 Mar 2017 07:15:02 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 21 Mar 2017 07:15:02 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.132]) with mapi id 14.03.0248.002; Tue, 21 Mar 2017 22:15:00 +0800 From: "Yao, Jiewen" To: Ard Biesheuvel , "edk2-devel@lists.01.org" CC: "Zeng, Star" , "Tian, Feng" , "Gao, Liming" Thread-Topic: [PATCH] MdeModulePkg/MemoryProtection: split protect and unprotect paths Thread-Index: AQHSokoGkhX63x2D3UK70V4Lvszz46GfVhiA Date: Tue, 21 Mar 2017 14:14:59 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A907F2F@shsmsx102.ccr.corp.intel.com> References: <1490104148-4193-1-git-send-email-ard.biesheuvel@linaro.org> In-Reply-To: <1490104148-4193-1-git-send-email-ard.biesheuvel@linaro.org> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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 14:15:07 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Good idea. Reviewed-by: Jiewen.yao@Intel.com > -----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 ; G= ao, > Liming ; Ard Biesheuvel > Subject: [PATCH] MdeModulePkg/MemoryProtection: split protect and > unprotect paths >=20 > 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. >=20 > 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. >=20 > 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. >=20 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- >=20 > This is an alternative fix for the issue addressed by patch >=20 > 'MdeModulePkg/DxeCore: ignore PdbPointer if ImageAddress =3D=3D 0' >=20 > (https://lists.01.org/pipermail/edk2-devel/2017-March/008766.html) >=20 > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 85 ++++++++------------ > 1 file changed, 35 insertions(+), 50 deletions(-) >=20 > 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; >=20 > extern LIST_ENTRY mGcdMemorySpaceMap; >=20 > +STATIC LIST_ENTRY mProtectedImageRecordList; > + > /** > Sort code section in image record, based upon CodeSegmentBase from low= to > high. >=20 > @@ -238,13 +240,10 @@ SetUefiImageMemoryAttributes ( > Set UEFI image protection attributes. >=20 > @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; >=20 > ImageRecordCodeSectionList =3D &ImageRecord->CodeSegmentList; >=20 > @@ -276,29 +274,19 @@ SetUefiImageProtectionAttributes ( > // > // DATA > // > - if (Protect) { > - Attribute =3D EFI_MEMORY_XP; > - } else { > - Attribute =3D 0; > - } > SetUefiImageMemoryAttributes ( > CurrentBase, > ImageRecordCodeSection->CodeSegmentBase - CurrentBase, > - Attribute > + EFI_MEMORY_XP > ); > } > // > // CODE > // > - if (Protect) { > - Attribute =3D EFI_MEMORY_RO; > - } else { > - Attribute =3D 0; > - } > SetUefiImageMemoryAttributes ( > ImageRecordCodeSection->CodeSegmentBase, > ImageRecordCodeSection->CodeSegmentSize, > - Attribute > + EFI_MEMORY_RO > ); > CurrentBase =3D ImageRecordCodeSection->CodeSegmentBase + > ImageRecordCodeSection->CodeSegmentSize; > } > @@ -310,15 +298,10 @@ SetUefiImageProtectionAttributes ( > // > // DATA > // > - if (Protect) { > - Attribute =3D EFI_MEMORY_XP; > - } else { > - Attribute =3D 0; > - } > SetUefiImageMemoryAttributes ( > CurrentBase, > ImageEnd - CurrentBase, > - Attribute > + EFI_MEMORY_XP > ); > } > return ; > @@ -401,18 +384,15 @@ FreeImageRecord ( > } >=20 > /** > - Protect or unprotect UEFI image common function. > + Protect UEFI PE/COFF image >=20 > @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); >=20 > // > - // Clean up > + // Record the image record in the list so we can undo the protections = later > // > - FreeImageRecord (ImageRecord); > + InsertTailList (&mProtectedImageRecordList, &ImageRecord->Link); >=20 > Finish: > return ; > } >=20 > /** > - 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) !=3D 0) { > - ProtectUefiImageCommon (LoadedImage, LoadedImageDevicePath, > TRUE); > - } > -} > - > -/** > Unprotect UEFI image. >=20 > @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) !=3D 0) { > - ProtectUefiImageCommon (LoadedImage, LoadedImageDevicePath, > FALSE); > + for (ImageRecordLink =3D mProtectedImageRecordList.ForwardLink; > + ImageRecordLink !=3D &mProtectedImageRecordList; > + ImageRecordLink =3D ImageRecordLink->ForwardLink) { > + ImageRecord =3D CR ( > + ImageRecordLink, > + IMAGE_PROPERTIES_RECORD, > + Link, > + IMAGE_PROPERTIES_RECORD_SIGNATURE > + ); > + > + if (ImageRecord->ImageBase =3D=3D > (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase) { > + SetUefiImageMemoryAttributes (ImageRecord->ImageBase, > + ImageRecord->ImageSize, > + 0); > + FreeImageRecord (ImageRecord); > + return; > + } > + } > } > } >=20 > @@ -1027,6 +1010,8 @@ CoreInitializeMemoryProtection ( >=20 > mImageProtectionPolicy =3D PcdGet32(PcdImageProtectionPolicy); >=20 > + InitializeListHead (&mProtectedImageRecordList); > + > // > // Sanity check the PcdDxeNxMemoryProtectionPolicy setting: > // - code regions should have no EFI_MEMORY_XP attribute > -- > 2.7.4