From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x230.google.com (mail-io0-x230.google.com [IPv6:2607:f8b0:4001:c06::230]) (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 53E4881F1A for ; Tue, 28 Feb 2017 02:59:25 -0800 (PST) Received: by mail-io0-x230.google.com with SMTP id l7so6866976ioe.3 for ; Tue, 28 Feb 2017 02:59:25 -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=f7OL/2nO2gVZEYPgXc+CBZzbulzo2F1fOUo6GEQFkp8=; b=XD1UPteYBHeRsrDKs98A0/9oFdLbi0mP5mpcNgDcl5dy/XO8T5APr9DGToj7QZvqN7 xEqrqQaF1aqCKEpq0f2i3gaUwP9qrgzlWBB8x+U4KqrF9OgC5cUx3fqHmyisx9OE3Chn rtLbv06nZFAaw0l/3qGX0H07oXvgrpa0jUJso= 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=f7OL/2nO2gVZEYPgXc+CBZzbulzo2F1fOUo6GEQFkp8=; b=O3OCjl5/oAP/jAqBp5fsFuIotlmOc+Sgs9QRLGDBpJ2RFEJ5yo8WGYasiY+CinqXWK XJ8tioC192ISLlo1+PzRPhGPncxm9RLNZSl7bbweWLlbc/ZmT5zQusAuzvg1SyUl5Ndv BeOYtLxX9Ls28RPz28N9/mrRTzJwwTnbgdUNr5SWBIlSoa9JB0s7R1X/W5S1ibYiPGRF 65m8qd9+4Auq61JQv1wR3nm8VeUI/HpWhpD05JZSb083vSxTYmqByx/WD8HS72RXWKnG UEptiTSsesaBdhp/G55xU/0xPJqZtwszYEN/ZuIdE5jGxORiG+X2OULOjmkO7BQ1BrX8 I77g== X-Gm-Message-State: AMke39myAoxq2Ke8g990ZA8JTp2iKJKYbOWWbgZmjiLkyEfVC64t2R3kUPtca+t7B6ox/erCcLMabajUS2OPWSCd X-Received: by 10.107.168.21 with SMTP id r21mr1675348ioe.45.1488279560615; Tue, 28 Feb 2017 02:59:20 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 28 Feb 2017 02:59:20 -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:59:20 +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:59:25 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 28 February 2017 at 10:52, Ard Biesheuvel wr= ote: > 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 exec= utable >>> permissions from writable memory regions, which greatly enhances securi= ty. >>> It is based on Jiewen's recent work, which is a step in the right direc= tion, >>> 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 a= lready >>> 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 al= locates >>> 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 R= AM that >>> is exploitable, and it also removes the need to apply memory protecti= ons 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 CoreAddMemoryDescri= ptor() >>> - 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 allocat= ions >>> - added some R-b's >>> >>> Changes since v1: >>> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pa= ges 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 w= ay, we >>> should be able to solve the EBC case by allocating BootServicesCode p= ool >>> memory explicitly. >>> >>> Series can be found here: >>> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=3Dmem= prot-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 zer= o 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,0= xDB4976D3,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 6666666= 666666666 >> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000= 000000000 >> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040= 140100401 >> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000= 000000000 >> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000= 000000000 >> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000= 000000000 >> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000= 000000000 >> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000= 000000000 >> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000= 000000000 >> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000= 000000000 >> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000= 000000000 >> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000= 000000000 >> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000= 000000000 >> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000= 000000000 >> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000= 000000000 >> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000= 000000000 >> >> SP 0x000000013EC6AA50 ELR 0x00000001380F7400 SPSR 0x60000205 FPSR 0= x00000000 >> 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) Thanks, Ard. >> Stack dump: >> 000013EC6A950: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6A970: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6A990: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6A9B0: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6A9D0: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6A9F0: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6AA10: 0000000000000000 0000000000000000 00000001380F7400 0000= 000060000205 >> 000013EC6AA30: 0000000000000000 000000008600000E 00000001380F7400 0000= 000400000800 >>> 000013EC6AA50: 000000013EC6AB50 00000001382F8198 0000000138367370 00000= 0013A6EEA98 >> 000013EC6AA70: 0000000138CDB698 000FD00000000000 00000001381F5018 0000= 000000000000 >> 000013EC6AA90: 0000000000000000 00000001382F3D18 0000000000001000 0000= 000000000000 >> 000013EC6AAB0: 000000013BFF0018 000000013A4BA518 000000013A6D2F98 0000= 000000000000 >> 000013EC6AAD0: 0000000000000000 0000000000000000 00000001382F4000 0000= 0000000CBAE0 >> 000013EC6AAF0: 0000000200000001 0000000000000000 000000013A6D2AC0 11D2= 95625B1B31A1 >> 000013EC6AB10: 3B7269C9A0003F8E 0000000000000000 00000001382F3F98 0000= 00003EC6AB58 >> 000013EC6AB30: 000000013EC6AB60 800000000000000E 000000013EC6AB80 0000= 000000000000 >> 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, = but that's not a problem, it only elicits a warning, and that's it. "shim.e= fi" 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 catc= h 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,0= x800,0x4E000)/\EFI\opensuse\grubaa64.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0= x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0= x4E000)/\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-0= 81796D9CDB4,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 6666666= 666666666 >> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000= 000000000 >> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040= 140100401 >> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000= 000000000 >> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000= 000000000 >> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000= 000000000 >> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000= 000000000 >> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000= 000000000 >> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000= 000000000 >> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000= 000000000 >> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000= 000000000 >> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000= 000000000 >> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000= 000000000 >> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000= 000000000 >> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000= 000000000 >> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000= 000000000 >> >> SP 0x000000013EC6AB60 ELR 0x00000000FFFF1F3C SPSR 0x60000205 FPSR 0= x00000000 >> ESR 0x8600000D FAR 0x00000000FFFF1F3C >> >> ESR : EC 0x21 IL 0x1 ISS 0x0000000D >> >> Instruction abort: Permission fault, first level >> >> Stack dump: >> 000013EC6AA60: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6AA80: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6AAA0: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6AAC0: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6AAE0: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6AB00: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6AB20: 0000000000000000 0000000000000000 00000000FFFF1F3C 0000= 000060000205 >> 000013EC6AB40: 0000000000000000 000000008600000D 00000000FFFF1F3C 0000= 000000000000 >>> 000013EC6AB60: 000000013EC6AB80 0000000138399650 00000001383AAA18 00000= 00000000000 >> 000013EC6AB80: 000000013EC6ABD0 000000013EC71E50 000000013A732218 0000= 00013A747718 >> 000013EC6ABA0: 0000000000000000 0000000000000000 0000000000000000 0000= 000000000000 >> 000013EC6ABC0: 0000000000000000 000000013A747718 000000013EC6AC50 0000= 00013BA5C62C >> 000013EC6ABE0: 000000013EC6AC00 0000000138CD4060 0000000138CD4068 0000= 00013A6EE018 >> 000013EC6AC00: 000000013EC6AC30 000000013BA577B4 0000000000000000 0000= 000000000000 >> 000013EC6AC20: 000000013ECA83D8 0000000000000126 000000013EC6AC50 0000= 00013A69FD18 >> 000013EC6AC40: 000000013A6EE018 0000000000000000 000000013EC6ACF0 0000= 00013BA4AF88 >> 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 t= he default 0), then all three guests boot fine. >> >> This reminds me of the case when Star introduced the non-executable stac= k for DXE, we turned it on in OVMF, and that killed the grub version shippe= d 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 "of= f" (see commit ab081a50e565, "OvmfPkg: PlatformPei: take no-exec DXE settin= gs from the QEMU command line", 2015-09-15). >> >> Thanks >> Laszlo