Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752146AbdGFJNr (ORCPT ); Thu, 6 Jul 2017 05:13:47 -0400 Received: from cn.fujitsu.com ([59.151.112.132]:12466 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751680AbdGFJNp (ORCPT ); Thu, 6 Jul 2017 05:13:45 -0400 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="20940532" Date: Thu, 6 Jul 2017 17:13:32 +0800 From: Chao Fan To: Naoya Horiguchi CC: Baoquan He , Kees Cook , LKML , "x86@kernel.org" , Thomas Gleixner , "H. Peter Anvin" , Ingo Molnar , "izumi.taku@jp.fujitsu.com" , Thomas Garnier , Matt Fleming , Junichi Nomura Subject: Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Message-ID: <20170706091327.GA26868@localhost.localdomain> References: <20170706083106.GA21796@hori1.linux.bs1.fc.nec.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20170706083106.GA21796@hori1.linux.bs1.fc.nec.co.jp> User-Agent: Mutt/1.8.0 (2017-02-23) X-Originating-IP: [10.167.226.75] X-yoursite-MailScanner-ID: F3AAA46B4C60.ADD1A X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: fanc.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5671 Lines: 163 On Thu, Jul 06, 2017 at 08:31:07AM +0000, Naoya Horiguchi wrote: >Hi Baoquan, everyone, > >I'm also interested in KASLR/EFI related issue (but not the same issue >with yours, so I separated the thread.) > >This patch is based on Baoquan's recent patches[1], adding more code >on the new function process_efi_entry(). >If it's OK, could you queue this onto your tree/series? > >[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized" > https://lkml.org/lkml/2017/7/5/98 > >Thanks, >Naoya Horiguchi >--- >From: Naoya Horiguchi >Date: Thu, 6 Jul 2017 16:40:52 +0900 >Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} 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. > >Signed-off-by: Naoya Horiguchi >--- > arch/x86/boot/compressed/kaslr.c | 41 +++++++++++++++++++++++++++++++--------- > 1 file changed, 32 insertions(+), 9 deletions(-) > >diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c >index 94f08fd375ae..f43fed0441a6 100644 >--- a/arch/x86/boot/compressed/kaslr.c >+++ b/arch/x86/boot/compressed/kaslr.c >@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry, > /* Marks if efi mirror regions have been found and handled. */ > static bool efi_mirror_found; > >-static void process_efi_entry(unsigned long minimum, unsigned long image_size) >+/* Returns true if we really enter efi memmap walk, otherwise returns false. */ >+static bool process_efi_entry(unsigned long minimum, unsigned long image_size) > { > struct efi_info *e = &boot_params->efi_info; > struct mem_vector region; >@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size) > signature = (char *)&boot_params->efi_info.efi_loader_signature; > if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) && > strncmp(signature, EFI64_LOADER_SIGNATURE, 4)) >- return; >+ return false; > > #ifdef CONFIG_X86_32 > /* Can't handle data above 4GB at this time */ > if (e->efi_memmap_hi) { > warn("Memory map is above 4GB, EFI should be disabled.\n"); >- return; >+ return false; > } > pmap = e->efi_memmap; > #else >@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size) > nr_desc = e->efi_memmap_size / e->efi_memdesc_size; > for (i = 0; i < nr_desc; i++) { > md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size)); >- 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); >+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE) > efi_mirror_found = true; Hi Horiguchi-san, If efi_mirror_found is changed to be true, we won't need to walk other entries, so I think: if (md->attribute & EFI_MEMORY_MORE_RELIABLE) { efi_mirror_found = true; break; } will be enough to show that mirror regions exist. And will walk less entries. How do you think about this? Another question: what's the benifit of putting this part of "efi_mirror_found = true" to a independent cycle. Thanks, Chao Fan >+ } >+ >+ for (i = 0; i < nr_desc; i++) { >+ md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size)); >+ >+ /* >+ * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot >+ * services regions could be accessed after ExitBootServices() >+ * due to the workaround for buggy firmware. >+ */ >+ if (!(md->type == EFI_LOADER_CODE || >+ md->type == EFI_LOADER_DATA || >+ 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; > } > > static void process_e820_entry(unsigned long minimum, unsigned long image_size) >@@ -637,8 +661,7 @@ static unsigned long find_random_phys_addr(unsigned long minimum, > minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN); > > #ifdef CONFIG_EFI >- process_efi_entry(minimum, image_size); >- if (efi_mirror_found) >+ if (process_efi_entry(minimum, image_size)) > return slots_fetch_random(); > #endif > >-- >2.7.4 > > >