Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752179AbdGFJXp convert rfc822-to-8bit (ORCPT ); Thu, 6 Jul 2017 05:23:45 -0400 Received: from tyo161.gate.nec.co.jp ([114.179.232.161]:36186 "EHLO tyo161.gate.nec.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752117AbdGFJXo (ORCPT ); Thu, 6 Jul 2017 05:23:44 -0400 From: Naoya Horiguchi To: Chao Fan 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 Thread-Topic: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Thread-Index: AQHS9jI2mWWdXsD8kkSmPgVylBsaHqJF7QIAgAACiwA= Date: Thu, 6 Jul 2017 09:22:38 +0000 Message-ID: <20170706092238.GA24491@hori1.linux.bs1.fc.nec.co.jp> References: <20170706083106.GA21796@hori1.linux.bs1.fc.nec.co.jp> <20170706091327.GA26868@localhost.localdomain> In-Reply-To: <20170706091327.GA26868@localhost.localdomain> Accept-Language: en-US, ja-JP Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.128.101.10] Content-Type: text/plain; charset="iso-2022-jp" Content-ID: <7006DDD74E6BDB4FA7480A6BB5A4AD77@gisp.nec.co.jp> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5097 Lines: 126 On Thu, Jul 06, 2017 at 05:13:32PM +0800, Chao Fan wrote: > 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? Thank you for the review, Chao. And you're right, I'll add break here. # I'll post revised one tomorrow waiting for more comments. > Another question: what's the benifit of putting this part of > "efi_mirror_found = true" to a independent cycle. We can't easily cancel process_mem_region(), so if we process a few normal regions like EFI_CONVENTIONAL_MEMORY and then find a EFI_MEMORY_MORE_RELIABLE region, that's a bit troublesome. So I decided to first check whether EFI_MEMORY_MORE_RELIABLE region exists or not. Thanks, Naoya Horiguchi