Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751157AbdH1Hzq (ORCPT ); Mon, 28 Aug 2017 03:55:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52020 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750735AbdH1Hzp (ORCPT ); Mon, 28 Aug 2017 03:55:45 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E049281DE6 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=bhe@redhat.com Date: Mon, 28 Aug 2017 15:55:42 +0800 From: Baoquan He To: Naoya Horiguchi Cc: Kees Cook , "linux-kernel@vger.kernel.org" , "x86@kernel.org" , Thomas Gleixner , "H. Peter Anvin" , Ingo Molnar , "izumi.taku@jp.fujitsu.com" , Thomas Garnier , "fanc.fnst@cn.fujitsu.com" , Matt Fleming , Junichi Nomura , Ard Biesheuvel Subject: Re: [PATCH v5] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_* and EFI_LOADER_* from KASLR's choice Message-ID: <20170828075542.GC23855@x1> References: <1503570831-9683-1-git-send-email-n-horiguchi@ah.jp.nec.com> <20170828065951.GB23855@x1> <20170828074444.GC23181@hori1.linux.bs1.fc.nec.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170828074444.GC23181@hori1.linux.bs1.fc.nec.co.jp> User-Agent: Mutt/1.7.0 (2016-08-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 28 Aug 2017 07:55:45 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4409 Lines: 124 On 08/28/17 at 07:44am, Naoya Horiguchi wrote: > From 93182d1d8c2ce11232f7686c01dc16ee96e062c4 Mon Sep 17 00:00:00 2001 > From: Naoya Horiguchi > Date: Mon, 28 Aug 2017 16:30:59 +0900 > Subject: [PATCH v5] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_* and EFI_LOADER_* > from KASLR's choice > > KASLR chooses kernel location from E820_TYPE_RAM regions by walking over > e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and > EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to > UEFI spec, all memory regions marked as EfiBootServicesCode and > EfiBootServicesData are available for free memory after the first call > of ExitBootServices(). So such regions should be usable for kernel on > spec basis. > > In x86, however, we have some workaround for broken firmware, where we > keep such regions reserved until SetVirtualAddressMap() is done. > See the following code in should_map_region(): > > static bool should_map_region(efi_memory_desc_t *md) > { > ... > /* > * Map boot services regions as a workaround for buggy > * firmware that accesses them even when they shouldn't. > * > * See efi_{reserve,free}_boot_services(). > */ > if (md->type == EFI_BOOT_SERVICES_CODE || > md->type == EFI_BOOT_SERVICES_DATA) > return false; > > This workaround suppressed a boot crash, but potential issues still > remain because no one prevents the regions from overlapping with kernel > image by KASLR. > > So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never > chosen as kernel memory for the workaround to work fine. Furthermore, > EFI_LOADER_{CODE|DATA} regions are also excluded because they can be > used after ExitBootServices() as defined in EFI spec. As a result, we > choose kernel address only from EFI_CONVENTIONAL_MEMORY which is the > only memory type we know to be free. > > Signed-off-by: Naoya Horiguchi Thanks, looks good to me. Ack it. Acked-by: Baoquan He > --- > v4 -> v5: > - addressed why EFI_LOADER_* is excluded. > - changed patch subject. > > v3 -> v4: > - update comment and patch description to mention why only > EFI_CONVENTIONAL_MEMORY is chosen. > - use efi_early_memdesc_ptr() > - I decided not to post cleanup patches (patch 2/2 in previous series) > because it's not necessary to fix the issue. > > v2 -> v3: > - skip EFI_LOADER_CODE and EFI_LOADER_DATA in region scan > > v1 -> v2: > - switch efi_mirror_found to local variable > - insert break when EFI_MEMORY_MORE_RELIABLE found > --- > arch/x86/boot/compressed/kaslr.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c > index 7de23bb279ce..ba5e9e5aaa89 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -597,19 +597,36 @@ process_efi_entries(unsigned long minimum, unsigned long image_size) > for (i = 0; i < nr_desc; i++) { > md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i); > if (md->attribute & EFI_MEMORY_MORE_RELIABLE) { > - region.start = md->phys_addr; > - region.size = md->num_pages << EFI_PAGE_SHIFT; > - process_mem_region(®ion, minimum, image_size); > efi_mirror_found = true; > - > - if (slot_area_index == MAX_SLOT_AREA) { > - debug_putstr("Aborted EFI scan (slot_areas full)!\n"); > - break; > - } > + break; > } > } > > - return efi_mirror_found; > + for (i = 0; i < nr_desc; i++) { > + md = efi_early_memdesc_ptr(pmap, e->efi_memdesc_size, i); > + > + /* > + * According to spec, EFI_BOOT_SERVICES_{CODE|DATA} are also > + * available for kernel image, but we don't include them for > + * the workaround for buggy firmware. > + * Only EFI_CONVENTIONAL_MEMORY is guaranteed to be free. > + */ > + if (md->type != EFI_CONVENTIONAL_MEMORY) > + continue; > + > + if (efi_mirror_found && > + !(md->attribute & EFI_MEMORY_MORE_RELIABLE)) > + continue; > + > + region.start = md->phys_addr; > + region.size = md->num_pages << EFI_PAGE_SHIFT; > + process_mem_region(®ion, minimum, image_size); > + if (slot_area_index == MAX_SLOT_AREA) { > + debug_putstr("Aborted EFI scan (slot_areas full)!\n"); > + break; > + } > + } > + return true; > } > #else > static inline bool > -- > 2.7.4 >