From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22b.google.com (mail-io0-x22b.google.com [IPv6:2607:f8b0:4001:c06::22b]) (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 397E181F3A for ; Tue, 28 Feb 2017 03:47:08 -0800 (PST) Received: by mail-io0-x22b.google.com with SMTP id 90so7573699ios.1 for ; Tue, 28 Feb 2017 03:47: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=JgWx9OfFJMH47PQoYLXkcw19HkpemcbadgffMtjWUyM=; b=EAtBF9Vghtn1Iur0L8lzqehprPpMGgDGN+orQGfuzC9Y05q+vEcGQ2pX7Nku/LoS4T 6IQM+++gyXfhFgQVrr812i1nK+YWSSXisf+aYuMQw3wdZt0hQjuWM/r5eXkqn8XlW4wq v3ede4hm+RonhHq0HwswHMcIOr8/Hf7ExEa9M= 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=JgWx9OfFJMH47PQoYLXkcw19HkpemcbadgffMtjWUyM=; b=ZLWKaauCBs2xwBb0ZK/3mFSzEP3m3tUHhI94Vf/dSX0E7WE3KU8B9nnnfua44wAcIJ OS4PQrkRtkWEIDP9/qJU+ERzagFKtBEESuiMUYslswHYS4hheODQNrC1TE7tYgVMSnIA Z1wyZ+OX6Gj/fuLAYmzYtTRitzXFYf9NE0ECdCNZsdSv5KRuuAXNKXvPR1jvDul1+F0M vxNpqlqPv3z/zTktd6Gs68DknJFtlEuZIlDFxdRpEcO3aXL/+cvXOFZTa5H6iUgaUGud KADoLhsYprNxHa9m+Ph06wt8rJEgQJseWuaRHjIKDcYDVopCgfm3qqmkbgeGjsxbSWbR ucdg== X-Gm-Message-State: AMke39kfW9Nx7RyZeNJ5KY8t7AO7urZzVLsUxSM+HQoZliG9vavAWUKqPSJV1gAbsT0nSQkWXEQmKKJZMT6A2SRU X-Received: by 10.107.13.130 with SMTP id 124mr1900612ion.83.1488282427159; Tue, 28 Feb 2017 03:47:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 28 Feb 2017 03:47:06 -0800 (PST) In-Reply-To: References: <1488206291-25768-1-git-send-email-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Tue, 28 Feb 2017 11:47:06 +0000 Message-ID: To: Laszlo Ersek , Leif Lindholm Cc: "edk2-devel@lists.01.org" , "afish@apple.com" , "Kinney, Michael D" , "Gao, Liming" , "Yao, Jiewen" , "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 11:47:08 -0000 Content-Type: text/plain; charset=UTF-8 On 28 February 2017 at 10:59, Ard Biesheuvel wrote: > On 28 February 2017 at 10:52, Ard Biesheuvel wrote: >> On 28 February 2017 at 10:46, Laszlo Ersek wrote: >>> On 02/27/17 15:38, Ard Biesheuvel wrote: >>>> 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-take2-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(-) >>>> >>> >>> I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues. >>> >>> However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM): >>> >>> ----------------- >>> [Bds]Booting Fedora >>> FSOpen: Open '\EFI\fedora\shim.efi' Success >>> [Bds] DevicePath expand: HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi >>> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi. >>> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A6D2AC0 >>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148 >>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148 >>> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CDBD98 >>> ProtectUefiImageCommon - 0x3A6D2AC0 >>> - 0x00000001382F4000 - 0x00000000000CBAE0 >>> !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x20) is incorrect !!!!!!!! >>> FSOpen: Open '\EFI\fedora\grubaa64.efi' Success >>> >>> >>> Synchronous Exception at 0x00000001380F7400 >>> >>> X0 0x000000013A6EEA98 X1 0x000000013BFF0018 X2 0x00000001380F7400 X3 0x00000000000FD000 >>> X4 0x0000000000000000 X5 0x0000000000000000 X6 0x0000000138362AF4 X7 0x0000000000000000 >>> X8 0x000000013C01F548 X9 0x0000000200000000 X10 0x00000001380F6000 X11 0x00000001382F3FFF >>> X12 0x0000000000000000 X13 0x0000000000000008 X14 0x0000000000000000 X15 0x0000000000000000 >>> X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x0000000138CDB698 >>> X20 0x000000013A746E18 X21 0x0000000000000000 X22 0x0000000000000000 X23 0x0000000000000000 >>> X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000 >>> X28 0x0000000000000000 FP 0x000000013EC6AA50 LR 0x00000001382F80F8 >>> >>> V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666 >>> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000 >>> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401 >>> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000 >>> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000 >>> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000 >>> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000 >>> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000 >>> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000 >>> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000 >>> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000 >>> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000 >>> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000 >>> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000 >>> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000 >>> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000 >>> >>> SP 0x000000013EC6AA50 ELR 0x00000001380F7400 SPSR 0x60000205 FPSR 0x00000000 >>> ESR 0x8600000E FAR 0x00000001380F7400 >>> >>> ESR : EC 0x21 IL 0x1 ISS 0x0000000E >>> >>> Instruction abort: Permission fault, second level >>> >> >> Hmm, that is disappointing. This is probably due to GRUB's modular >> nature, which means it allocates memory and loads executable code into >> it, under the assumption that memory is always executable in UEFI. >> >> The short term fix is to remove the NX bit from LoaderData regions, >> but in the mean time, I will work with Leif to get this fixed properly >> (assuming there is a proper way to fix this) >> > > Care to have a quick go at using 0xC000000000007FD1 instead? (if you > are not already doing so) > Adding my own data point: running CelloBoard with gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy set to 0xC000000000007FD5 happily boots straight into the kernel, but crashes when booting via GRUB. Changing the value to 0xC000000000007FD1 gets things working again.