From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 AA7C781F5B for ; Mon, 27 Feb 2017 00:56:47 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ECE7BC04B320; Mon, 27 Feb 2017 08:56:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-99.phx2.redhat.com [10.3.116.99]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1R8uip9012300; Mon, 27 Feb 2017 03:56:45 -0500 To: Ard Biesheuvel , "Yao, Jiewen" References: <1487948699-3179-1-git-send-email-ard.biesheuvel@linaro.org> <74D8A39837DF1E4DA445A8C0B3885C503A8F539C@shsmsx102.ccr.corp.intel.com> Cc: "edk2-devel@lists.01.org" , "afish@apple.com" , "leif.lindholm@linaro.org" , "Kinney, Michael D" , "Gao, Liming" , "Tian, Feng" , "Zeng, Star" From: Laszlo Ersek Message-ID: Date: Mon, 27 Feb 2017 09:56:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 27 Feb 2017 08:56:48 +0000 (UTC) Subject: Re: [PATCH v2 0/5] RFC: increased memory protection X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Feb 2017 08:56:47 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 02/26/17 16:09, Ard Biesheuvel wrote: > On 25 February 2017 at 04:04, Yao, Jiewen wrote: >> Thank you Ard. I like this patch - simple and obvious. >> > > Thank you > >> I put all my comment together for your consideration. >> >> 1) Patch V2 1/5 -- reviewed-by: Jiewen.yao@Intel.com >> 2) Patch V2 2/5 - reviewed-by: Jiewen.yao@intel.com > > OK > >> 3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com >> > > I may be able to drop this if the ApplyMemoryProtection() calls need > to be moved elsewhere for pool allocations. > >> 4) Patch V2 4/5 - >> 4.1) Can we follow the style of other memory type definition? (Such as >> PcdMemoryProfileMemoryType) >> >> The reason is that people may want to have fine granularity control for >> loader data or persistent memory. >> >> My proposal is below: >> ////////////////////////// >> ## Set DXE memory protection policy. The policy is bitwise. >> # If a bit is set, memory regions of the associated type will be mapped >> # non-executable.

>> # >> # Below is bit mask for this PCD: (Order is same as UEFI spec)
>> # EfiReservedMemoryType 0x0001
>> # EfiLoaderCode 0x0002
>> # EfiLoaderData 0x0004
>> # EfiBootServicesCode 0x0008
>> # EfiBootServicesData 0x0010
>> # EfiRuntimeServicesCode 0x0020
>> # EfiRuntimeServicesData 0x0040
>> # EfiConventionalMemory 0x0080
>> # EfiUnusableMemory 0x0100
>> # EfiACPIReclaimMemory 0x0200
>> # EfiACPIMemoryNVS 0x0400
>> # EfiMemoryMappedIO 0x0800
>> # EfiMemoryMappedIOPortSpace 0x1000
>> # EfiPalCode 0x2000
>> # EfiPersistentMemory 0x4000
>> # OEM Reserved 0x4000000000000000
>> # OS Reserved 0x8000000000000000
>> # >> # NOTE: User must NOT set NX protection for EfiLoaderCode / >> EfiBootServicesCode / EfiRuntimeServicesCode.
>> # >> # e.g. 0x7FD5 can be used for all memory except Code.
>> # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved. >>
>> # >> # @Prompt Set DXE memory protection policy. >> >> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048 >> ////////////////////////// >> >> Then the C-code can be like below: >> >> ////////////////////////// >> UINT64 >> GetPermissionAttributeForMemoryType ( >> IN EFI_MEMORY_TYPE MemoryType >> ) >> { >> UINT64 TestBit; >> >> if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) { >> TestBit = BIT63; >> } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) { >> TestBit = BIT62; >> } else { >> TestBit = LShiftU64 (1, MemoryType); >> } >> >> if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) != 0) { >> return EFI_MEMORY_XP; >> } else { >> return 0; >> } >> } >> ////////////////////////// >> > > Thanks, I will use your definition instead. > >> 4.2) I prefer to setting default value to be 0x0 - to keep the >> compatibility, at least for X86 platform. (I have no strong opinion for >> ARM.) >> > > Yes, naturally. For this RFC series, I used a default that enables the > feature, but I agree that this should be opt-in > >> 4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (add >> "NX" keyword), so that people can know this PCD is to control NX attribute. >> Maybe we can apply other protection such as RO or RP later. >> What about your idea? >> > > OK > >> 5) Patch V2 5/5 - >> 5.1) I think we should check the allocation happens IsInSmm, and skip >> ApplyMemoryProtection() if it is in Smm. >> >> The reason is that SMM maintains its own page table. >> >> Below code is for your reference. >> >> ////////////////////////// >> BOOLEAN >> IsInSmm ( >> VOID >> ) >> { >> BOOLEAN InSmm; >> >> InSmm = FALSE; >> if (gSmmBase2 != NULL) { >> gSmmBase2->InSmm (gSmmBase2, &InSmm); >> } >> return InSmm; >> } >> ////////////////////////// >> > > OK > >> 5.2) I think we are not able to call ApplyMemoryProtection() inside of >> CoreAllocatePoolPages() and CoreFreePoolPages(). >> The reason is that: X86 CPU page table update algo might call >> AllocatePages(), to support page table split from big page to small page. >> CoreAcquireMemoryLock() may fail in such case, because the memory map is >> locked in AllocatePool(). >> >> I think a safety way is to call ApplyMemoryProtection() at >> CoreAllocatePool(), after InstallMemoryAttributesTableOnMemoryAllocation(). >> We do same thing as CoreAllocatePage(). >> >> We can update CoreInternalAllocatePool() to return the necessary parameters >> back to indicate if CoreAllocatePoolPages() happens, and where is the new >> pages. >> Same thing for CoreFreePool(). >> > > I did realise this. But in my implementation, EfiConventionalMemory > and EfiBootServicesData always have the same policy, so the recursion > can never happen. Of course, with your version of the PCD, this could > occur, and we need to address it. > >> 5.3) In order to reduce the fragmentation of X86 page table, I recommend we >> do a little enhancement in ApplyDxeMemoryProtectionPolicy(). >> Can we can combine the memory need NX together and call >> SetUefiImageMemoryAttributes() once? >> >> You may refer to MergeMemoryMapForNotPresentEntry() in >> UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c, >> which combines the memory map entry together, if the adjacent entry requires >> same not-present attribute. >> >> In this case, we could define MergeMemoryMapForNonExecutable() in >> MemoryProtection.c, and used by ApplyDxeMemoryProtectionPolicy(). >> I believe it helps X86 platforms. >> > > Sure. I also need to copy SortMemoryMap() then, which performs a > bubble sort :-( And BaseSortLib cannot be used in DXE_CORE modules. You might want to check out MdePkg/Include/Library/OrderedCollectionLib.h MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf The library instance is consumable for all modules, as long as they have DebugLib and MemoryAllocationLib resolutions. When used in DXE_CORE (for which FreePool() has an actual implementation in "MdeModulePkg/Library/DxeCoreMemoryAllocationLib/MemoryAllocationLib.c"), it won't even leak memory (as opposed to usage in PEIMs, where FreePool() does nothing). An example that uses this library for sorting can be found in "OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c". Feel free to decide against it, I just thought I should mention it. Thanks Laszlo > > In any case, I will proceed with respinning these patches, > > Thanks for the feedback, > Ard. > > >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard >>> Biesheuvel >>> Sent: Friday, February 24, 2017 11:05 PM >>> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; >>> Kinney, >>> Michael D ; Gao, Liming >>> ; >>> Yao, Jiewen >>> Cc: Tian, Feng ; lersek@redhat.com; Zeng, Star >>> ; Ard Biesheuvel >>> Subject: [edk2] [PATCH v2 0/5] RFC: increased memory protection >> >> >>> >>> Hello all, >>> >>> This is a proof of concept implementation that removes all executable >>> permissions from writable memory regions, which greatly enhances security. >>> It is based on Jiewen's recent work, which is a step in the right >>> direction, >>> but still leaves most of memory exploitable due to the default R+W+X >>> permissions. >>> >>> The idea is that the implementation of the CPU arch protocol goes over the >>> memory map and removes exec permissions from all regions that are not >>> already >>> marked as 'code. This requires some preparatory work to ensure that the >>> DxeCore >>> itself is covered by a BootServicesCode region, not a BootServicesData >>> region. >>> Exec permissions are re-granted selectively, when the PE/COFF loader >>> allocates >>> the space for it. Combined with Jiewen's code/data split, this removes all >>> RWX mapped regions. >>> >>> Changes since v1: >>> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages >>> have >>> the expected memory type (as suggested by Jiewen) >>> - add patch to inhibit page table updates while syncing the GCD memory >>> space >>> map with the page tables >>> - add PCD to set memory protection policy, which allows the policy for >>> reserved >>> and ACPI/NVS memory to be configured separately >>> - move attribute manipulation into DxeCore page allocation code: this way, >>> we >>> should be able to solve the EBC case by allocating BootServicesCode pool >>> memory explicitly. >>> >>> Ard Biesheuvel (5): >>> ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() >>> MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF >>> images >>> MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages () >>> MdeModulePkg: define PCD for DXE memory protection policy >>> MdeModulePkg/DxeCore: implement memory protection policy >>> >>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 + >>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 + >>> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 + >>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + >>> MdeModulePkg/Core/Dxe/Mem/Imem.h | 2 + >>> MdeModulePkg/Core/Dxe/Mem/Page.c | 106 >>> ++++++++++++++++++++ >>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 5 +- >>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104 >>> ++++++++++++++++++- >>> MdeModulePkg/Core/Pei/Image/Image.c | 10 +- >>> MdeModulePkg/MdeModulePkg.dec | 16 +++ >>> 10 files changed, 246 insertions(+), 6 deletions(-) >>> >>> -- >>> 2.7.4 >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel