From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22d.google.com (mail-it0-x22d.google.com [IPv6:2607:f8b0:4001:c0b::22d]) (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 9132982189 for ; Tue, 28 Feb 2017 06:59:08 -0800 (PST) Received: by mail-it0-x22d.google.com with SMTP id h10so78646414ith.1 for ; Tue, 28 Feb 2017 06:59:08 -0800 (PST) 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=9KULoBuFvnAuSNjSf4uBb/CXZ0T3coDnUtdMq9MMobs=; b=gPP0opoqisC/ajLn27MI73JBGXKrZFxZYtQG/Gm/v/NRc80fOFnY6pZS/Xen2ZKaNR CddyXATOwNylSwyoXSBIiJTY7WCHguQd8G2tkpCrbTLCGCh4QdFHH4xLRFH3l5caG8i9 f0Khlv2dsCtW4++UB196Io5D51CgbL+w/yn88= 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=9KULoBuFvnAuSNjSf4uBb/CXZ0T3coDnUtdMq9MMobs=; b=VHIoYR4B/gB/+5vWCrJF0MgTY2bX3ZIlDn9cnpeOYycTEIu5fnLEIfBO+Kofj18MMi G3EUalmvaB20eNRyzbYOzojTQRYcQW4s3qCSzMfWX1B6lN9Lvc3pdV+7GVcUbM4LJUUC JK3Su2WOFWPa5fVgEG0gZrIlEn3lD3O19oBKKO/71LemIeOu2dGxnZSPARgYp37U6py3 DJnnsm0Ki2D+gtYu6CD/4OjxmxK5VtHSvsCfQe9wRQ8J8U4VuvrjN1y0q+H45H5QIFL1 0DpJA39VHn6nytHzoSqCSOnOcSCT5tSVGOvoO5wU4ruLuqujOumU+KviiE42RGVDN2sy tPOQ== X-Gm-Message-State: AMke39mQHcFeJ21X3eCBeielcdjkzyeowS5lotZ+ZKc4zy1y3a9A+0bhq+8zhXWwDBQsdaJXbRsW6Yop8KjqDNiN X-Received: by 10.36.77.10 with SMTP id l10mr3034243itb.59.1488293947715; Tue, 28 Feb 2017 06:59:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 28 Feb 2017 06:59:07 -0800 (PST) In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503A8F6795@shsmsx102.ccr.corp.intel.com> References: <1488206291-25768-1-git-send-email-ard.biesheuvel@linaro.org> <74D8A39837DF1E4DA445A8C0B3885C503A8F6795@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Tue, 28 Feb 2017 14:59:07 +0000 Message-ID: To: "Yao, Jiewen" , "leif.lindholm@linaro.org" , "Gao, Liming" , "lersek@redhat.com" Cc: "edk2-devel@lists.01.org" , "afish@apple.com" , "Kinney, Michael D" , "Tian, Feng" , "Zeng, Star" Subject: Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: 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: Tue, 28 Feb 2017 14:59:08 -0000 Content-Type: text/plain; charset=UTF-8 On 28 February 2017 at 05:48, Yao, Jiewen wrote: > All series reviewed-by: jiewen.yao@intel.com > X86 platform regression tested-by: Jiewen.yao@intel.com > Thanks all for the reviews and testing. I am going to proceed and merge this series, except for the last patch enabling it for ArmVirtQemu. That allows us to test it more widely on out of tree platforms. In the mean time, the recommendation is to disable NX protection for EfiLoaderData regions, on AArch64 as well as on X86. In the future, we may want to promote the PCD to one that can be configured as dynamic, and let the user decide which policy to use. I will follow up with a separate patch if we intend to go down that road. Regards, Ard. >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Monday, February 27, 2017 10:38 PM >> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, >> Michael D ; Gao, Liming ; >> Yao, Jiewen >> Cc: lersek@redhat.com; Tian, Feng ; Zeng, Star >> ; Ard Biesheuvel >> Subject: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection >> >> Hello all, >> >> First of all, thanks for the reviews and regression testing. However, I did >> not add the tested-by tags nor some of the R-b's, given the changes in this v4. >> >> This series implements a memory protection policy 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 v3: >> - mandate that the same policy applies to EfiConventionalMemory regions and >> EfiBootServicesData regions: they are unlikely to differ in practice, and >> dealing with that corner case greatly complicates the implementation, given >> the way DxeCore allocates memory for itself in the implementation of the >> page >> and pool allocation routines. >> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD >> memory space map: without this, we may still have a large region of RAM that >> is exploitable, and it also removes the need to apply memory protections in >> PromoteMemoryResource (), which is very difficult to achieve without a major >> restructuring of the code due to the way locking is implemented here. >> - add missing ApplyMemoryProtectionPolicy() call to >> CoreAddMemoryDescriptor() >> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4) >> - incorporate feedback from Liming (#2, #6) >> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7) >> >> Changes since v2: >> - added patch to make EBC use EfiBootServicesCode pool allocations for thunks >> - redefine PCD according to Jiewen's feedback, including default value >> - use sorted memory map and merge adjacent entries with the same policy, to >> prevent unnecessary page table splitting >> - ignore policy when executing in SMM >> - refactor the logic for managing permission attributes of pool allocations >> - added some R-b's >> >> 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. >> >> Series can be found here: >> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-tak >> e2-v4 >> >> Ard Biesheuvel (7): >> ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() >> MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF >> images >> MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks >> MdeModulePkg/DxeCore: use separate lock for pool allocations >> MdeModulePkg: define PCD for DXE memory protection policy >> MdeModulePkg/DxeCore: implement memory protection policy >> ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all >> platforms >> >> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 + >> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 + >> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 + >> ArmVirtPkg/ArmVirt.dsc.inc | 6 + >> MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++ >> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + >> MdeModulePkg/Core/Dxe/Mem/Page.c | 7 + >> MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++- >> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 >> +++++++++++++++++++- >> MdeModulePkg/Core/Pei/Image/Image.c | 23 +- >> MdeModulePkg/MdeModulePkg.dec | 32 ++ >> MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c | 2 +- >> MdeModulePkg/Universal/EbcDxe/EbcInt.c | 23 ++ >> MdeModulePkg/Universal/EbcDxe/EbcInt.h | 14 + >> MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c | 2 +- >> MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c | 2 +- >> MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c | 2 +- >> 17 files changed, 558 insertions(+), 24 deletions(-) >> >> -- >> 2.7.4 >