From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (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 66D7481F06 for ; Tue, 28 Feb 2017 02:52:40 -0800 (PST) Received: by mail-it0-x232.google.com with SMTP id 203so6696807ith.0 for ; Tue, 28 Feb 2017 02:52:40 -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:content-transfer-encoding; bh=HBN2bXvTv//r7lHM0Bx/z4i2nRi/DwO5Wn6exqKywCs=; b=dSKkZUQh8lXRX6Osva8/jFVWkxQP68QFdHT0UnIOlp5BSBYHGmplXEJhCDHQmHm4Lj aZNYz0o9tSMSbbWD85RjGND6zYWE4oQ2XEwTNpkCjqTek8/DxQHOoAJELyyhq/saAR4U Ye+u1MAHc4W8P/3OiQoDQEfGMhnNKxZBODCBc= 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:content-transfer-encoding; bh=HBN2bXvTv//r7lHM0Bx/z4i2nRi/DwO5Wn6exqKywCs=; b=LQcMigHwwOs/2mg2pDmbSrnXjrb7phw2KtijAGqQi/2rNslH56UGpQmp440EQzZi8Z K0YNODKLixX7AGzF0Li+rriXK0zoi2maQsBFZ49mXMnjNo2IbLN7biYXJwydu9SX8Taj 1klPUVouXqmo0CYjELJwXpjuhCvsOHqWfhp3L+R3L+kJEZdy+fnjAc6ji5usX0g9nPl3 eTkZ9LI/OzW/rVeec8ozta6Xx+8U/Uke4lB7HeFB/hFPVvjjZflL6lSDX3aDmgooDjPx 91M7h0Jp6ecFFDzAbwtnRy8u+pdux0asP9RJ5Rmeonwj6EE+eV8m7FUBFYxrwqSURko0 q+Kw== X-Gm-Message-State: AMke39kAhHrBOnTMbhPqAISuQ6EIjP2gk2G8gPVjLfMi4vxDiPMW2W1j1yiDuqiq7OOGKWolhBV1nNa5gTutTNTW X-Received: by 10.36.118.68 with SMTP id z65mr909741itb.59.1488279159358; Tue, 28 Feb 2017 02:52:39 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 28 Feb 2017 02:52:38 -0800 (PST) In-Reply-To: References: <1488206291-25768-1-git-send-email-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Tue, 28 Feb 2017 10:52:38 +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 10:52:40 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 t= his v4. >> >> This series implements a memory protection policy that removes all execu= table >> permissions from writable memory regions, which greatly enhances securit= y. >> It is based on Jiewen's recent work, which is a step in the right direct= ion, >> 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 t= he >> memory map and removes exec permissions from all regions that are not al= ready >> marked as 'code. This requires some preparatory work to ensure that the = DxeCore >> itself is covered by a BootServicesCode region, not a BootServicesData r= egion. >> Exec permissions are re-granted selectively, when the PE/COFF loader all= ocates >> the space for it. Combined with Jiewen's code/data split, this removes a= ll >> 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 t= he 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 RA= M that >> is exploitable, and it also removes the need to apply memory protectio= ns 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 CoreAddMemoryDescrip= tor() >> - 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 t= hunks >> - 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 allocati= ons >> - added some R-b's >> >> Changes since v1: >> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pag= es have >> the expected memory type (as suggested by Jiewen) >> - add patch to inhibit page table updates while syncing the GCD memory s= pace >> map with the page tables >> - add PCD to set memory protection policy, which allows the policy for r= eserved >> and ACPI/NVS memory to be configured separately >> - move attribute manipulation into DxeCore page allocation code: this wa= y, we >> should be able to solve the EBC case by allocating BootServicesCode po= ol >> memory explicitly. >> >> Series can be found here: >> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=3Dmemp= rot-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\s= him.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0x= DB4976D3,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=3D0x001382F4148 > Loading driver at 0x001382F4000 EntryPoint=3D0x001382F4148 > 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 66666666= 66666666 > V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 00000000= 00000000 > V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 40100401= 40100401 > V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 00000000= 00000000 > V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 00000000= 00000000 > V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 00000000= 00000000 > V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 00000000= 00000000 > V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 00000000= 00000000 > V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 00000000= 00000000 > V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 00000000= 00000000 > V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 00000000= 00000000 > V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 00000000= 00000000 > V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 00000000= 00000000 > V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 00000000= 00000000 > V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 00000000= 00000000 > V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 00000000= 00000000 > > SP 0x000000013EC6AA50 ELR 0x00000001380F7400 SPSR 0x60000205 FPSR 0x= 00000000 > 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) Thanks a lot for taking the time to test this, Ard. > Stack dump: > 000013EC6A950: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6A970: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6A990: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6A9B0: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6A9D0: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6A9F0: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6AA10: 0000000000000000 0000000000000000 00000001380F7400 00000= 00060000205 > 000013EC6AA30: 0000000000000000 000000008600000E 00000001380F7400 00000= 00400000800 >> 000013EC6AA50: 000000013EC6AB50 00000001382F8198 0000000138367370 000000= 013A6EEA98 > 000013EC6AA70: 0000000138CDB698 000FD00000000000 00000001381F5018 00000= 00000000000 > 000013EC6AA90: 0000000000000000 00000001382F3D18 0000000000001000 00000= 00000000000 > 000013EC6AAB0: 000000013BFF0018 000000013A4BA518 000000013A6D2F98 00000= 00000000000 > 000013EC6AAD0: 0000000000000000 0000000000000000 00000001382F4000 00000= 000000CBAE0 > 000013EC6AAF0: 0000000200000001 0000000000000000 000000013A6D2AC0 11D29= 5625B1B31A1 > 000013EC6AB10: 3B7269C9A0003F8E 0000000000000000 00000001382F3F98 00000= 0003EC6AB58 > 000013EC6AB30: 000000013EC6AB60 800000000000000E 000000013EC6AB80 00000= 00000000000 > ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/= DefaultExceptionHandler.c(265): ((BOOLEAN)(0=3D=3D1)) > ----------------- > > The "shim.efi" binary is not built with the required section alignment, b= ut that's not a problem, it only elicits a warning, and that's it. "shim.ef= i" still proceeds to load "grubaa64.efi". > > However, "grubaa64.efi" blows up. > > I experience the same with my "RHEL for ARM 7.3" guest. > > In my "openSUSE Tumbleweed" guest, "shim.efi" isn't actually used; there = the "opensuse" UEFI boot option refers to grub directly. There I even catch= the message "Welcome to GRUB!", but then it crashes too: > > --------------- > [Bds]Booting opensuse > FSOpen: Open '\EFI\opensuse\grubaa64.efi' Success > [Bds] DevicePath expand: HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x= 800,0x4E000)/\EFI\opensuse\grubaa64.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x= 0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x= 4E000)/\EFI\opensuse\grubaa64.efi > [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/= Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-08= 1796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi. > InstallProtocolInterface: [EfiLoadedImageProtocol] 13A69FD40 > Loading driver at 0x00138391000 EntryPoint=3D0x00138391400 > Loading driver at 0x00138391000 EntryPoint=3D0x00138391400 > InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CD4E98 > ProtectUefiImageCommon - 0x3A69FD40 > - 0x0000000138391000 - 0x000000000002E600 > !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x200) is incorrect = !!!!!!!! > Welcome to GRUB! > > ^M > > Synchronous Exception at 0x00000000FFFF1F3C > > X0 0x00000000FFFF3720 X1 0x00000000FFFF1F3C X2 0x000000000000000D = X3 0x000000013839FA30 > X4 0x00000000FFFF1F3C X5 0x00000000FFFF0800 X6 0x00000000FFFF1000 = X7 0x0000000000000000 > X8 0x00000000FFFF2120 X9 0x000000000000001F X10 0x0000000000000000 = X11 0x000000013EC6A880 > X12 0x0098989800989898 X13 0x0098989800989898 X14 0x0000000000000001 = X15 0x0000000000000003 > X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 = X19 0x00000000FFFF3720 > X20 0x0000000138399000 X21 0x0000000138399000 X22 0x0000000000000000 = X23 0x0000000000000000 > X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 = X27 0x0000000000000000 > X28 0x0000000000000000 FP 0x000000013EC6AB60 LR 0x00000001383980CC > > V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 66666666= 66666666 > V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 00000000= 00000000 > V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 40100401= 40100401 > V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 00000000= 00000000 > V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 00000000= 00000000 > V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 00000000= 00000000 > V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 00000000= 00000000 > V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 00000000= 00000000 > V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 00000000= 00000000 > V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 00000000= 00000000 > V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 00000000= 00000000 > V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 00000000= 00000000 > V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 00000000= 00000000 > V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 00000000= 00000000 > V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 00000000= 00000000 > V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 00000000= 00000000 > > SP 0x000000013EC6AB60 ELR 0x00000000FFFF1F3C SPSR 0x60000205 FPSR 0x= 00000000 > ESR 0x8600000D FAR 0x00000000FFFF1F3C > > ESR : EC 0x21 IL 0x1 ISS 0x0000000D > > Instruction abort: Permission fault, first level > > Stack dump: > 000013EC6AA60: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6AA80: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6AAA0: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6AAC0: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6AAE0: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6AB00: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6AB20: 0000000000000000 0000000000000000 00000000FFFF1F3C 00000= 00060000205 > 000013EC6AB40: 0000000000000000 000000008600000D 00000000FFFF1F3C 00000= 00000000000 >> 000013EC6AB60: 000000013EC6AB80 0000000138399650 00000001383AAA18 000000= 0000000000 > 000013EC6AB80: 000000013EC6ABD0 000000013EC71E50 000000013A732218 00000= 0013A747718 > 000013EC6ABA0: 0000000000000000 0000000000000000 0000000000000000 00000= 00000000000 > 000013EC6ABC0: 0000000000000000 000000013A747718 000000013EC6AC50 00000= 0013BA5C62C > 000013EC6ABE0: 000000013EC6AC00 0000000138CD4060 0000000138CD4068 00000= 0013A6EE018 > 000013EC6AC00: 000000013EC6AC30 000000013BA577B4 0000000000000000 00000= 00000000000 > 000013EC6AC20: 000000013ECA83D8 0000000000000126 000000013EC6AC50 00000= 0013A69FD18 > 000013EC6AC40: 000000013A6EE018 0000000000000000 000000013EC6ACF0 00000= 0013BA4AF88 > ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/= DefaultExceptionHandler.c(265): ((BOOLEAN)(0=3D=3D1)) > --------------- > > If I revert the last patch in the series (leaving ArmVirtQemu's PCD at th= e default 0), then all three guests boot fine. > > This reminds me of the case when Star introduced the non-executable stack= for DXE, we turned it on in OVMF, and that killed the grub version shipped= with an older Debian release: > > https://www.mail-archive.com/edk2-devel@lists.01.org/msg02022.html > > So ultimately we had to make the PCD dynamic, and make it default to "off= " (see commit ab081a50e565, "OvmfPkg: PlatformPei: take no-exec DXE setting= s from the QEMU command line", 2015-09-15). > > Thanks > Laszlo