From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 30725820AC for ; Fri, 24 Feb 2017 20:04:31 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Feb 2017 20:04:30 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,202,1484035200"; d="scan'208,217";a="1102102423" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga001.jf.intel.com with ESMTP; 24 Feb 2017 20:04:30 -0800 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 24 Feb 2017 20:04:30 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 24 Feb 2017 20:04:29 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.204]) with mapi id 14.03.0248.002; Sat, 25 Feb 2017 12:04:26 +0800 From: "Yao, Jiewen" To: Ard Biesheuvel , "edk2-devel@lists.01.org" , "afish@apple.com" , "leif.lindholm@linaro.org" , "Kinney, Michael D" , "Gao, Liming" CC: "Tian, Feng" , "lersek@redhat.com" , "Zeng, Star" , "Yao, Jiewen" Thread-Topic: [edk2] [PATCH v2 0/5] RFC: increased memory protection Thread-Index: AQHSjq+UVtSdstapDkCZmWHXEHpEDaF460kQ Date: Sat, 25 Feb 2017 04:04:25 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A8F539C@shsmsx102.ccr.corp.intel.com> References: <1487948699-3179-1-git-send-email-ard.biesheuvel@linaro.org> In-Reply-To: <1487948699-3179-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 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 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: Sat, 25 Feb 2017 04:04:31 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thank you Ard. I like this patch - simple and obvious. 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 3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com 4) Patch V2 4/5 - 4.1) Can we follow the style of other memory type definition? (Such as PcdM= emoryProfileMemoryType) The reason is that people may want to have fine granularity control for loa= der 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 / EfiBootServic= esCode / 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|UIN= T64|0x00001048 ////////////////////////// Then the C-code can be like below: ////////////////////////// UINT64 GetPermissionAttributeForMemoryType ( IN EFI_MEMORY_TYPE MemoryType ) { UINT64 TestBit; if ((UINT32) MemoryType >=3D MEMORY_TYPE_OS_RESERVED_MIN) { TestBit =3D BIT63; } else if ((UINT32) MemoryType >=3D MEMORY_TYPE_OEM_RESERVED_MIN) { TestBit =3D BIT62; } else { TestBit =3D LShiftU64 (1, MemoryType); } if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) !=3D 0) { return EFI_MEMORY_XP; } else { return 0; } } ////////////////////////// 4.2) I prefer to setting default value to be 0x0 - to keep the compatibilit= y, at least for X86 platform. (I have no strong opinion for ARM.) 4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (ad= d "NX" keyword), so that people can know this PCD is to control NX attribut= e. Maybe we can apply other protection such as RO or RP later. What about your idea? 5) Patch V2 5/5 - 5.1) I think we should check the allocation happens IsInSmm, and skip Apply= MemoryProtection() 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 =3D FALSE; if (gSmmBase2 !=3D NULL) { gSmmBase2->InSmm (gSmmBase2, &InSmm); } return InSmm; } ////////////////////////// 5.2) I think we are not able to call ApplyMemoryProtection() inside of Core= AllocatePoolPages() and CoreFreePoolPages(). The reason is that: X86 CPU page table update algo might call AllocatePage= s(), to support page table split from big page to small page. CoreAcquireMemoryLock() may fail in such case, because the memory map is lo= cked in AllocatePool(). I think a safety way is to call ApplyMemoryProtection() at CoreAllocatePool= (), after InstallMemoryAttributesTableOnMemoryAllocation(). We do same thin= g 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(). 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 SetUefiImageMemoryA= ttributes() once? You may refer to MergeMemoryMapForNotPresentEntry() in UefiCpuPkg\PiSmmCpuD= xeSmm\SmmCpuMemoryManagement.c, which combines the memory map entry together, if the adjacent entry require= s same not-present attribute. In this case, we could define MergeMemoryMapForNonExecutable() in MemoryPro= tection.c, and used by ApplyDxeMemoryProtectionPolicy(). I believe it helps X86 platforms. Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ar= d > Biesheuvel > Sent: Friday, February 24, 2017 11:05 PM > To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; K= inney, > 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 directi= on, > 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 th= e > memory map and removes exec permissions from all regions that are not alr= eady > marked as 'code. This requires some preparatory work to ensure that the > DxeCore > itself is covered by a BootServicesCode region, not a BootServicesData re= gion. > Exec permissions are re-granted selectively, when the PE/COFF loader allo= cates > the space for it. Combined with Jiewen's code/data split, this removes al= l > RWX mapped regions. > > Changes since v1: > - allocate code pages for PE/COFF images in PeiCore, so that DxeCore page= s have > the expected memory type (as suggested by Jiewen) > - add patch to inhibit page table updates while syncing the GCD memory sp= ace > map with the page tables > - add PCD to set memory protection policy, which allows the policy for re= served > 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 poo= l > 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