Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753653AbdHWIYe (ORCPT ); Wed, 23 Aug 2017 04:24:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57888 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753629AbdHWIYa (ORCPT ); Wed, 23 Aug 2017 04:24:30 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3E75180F6D Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=bhe@redhat.com Date: Wed, 23 Aug 2017 16:24:25 +0800 From: Baoquan He To: Naoya Horiguchi Cc: Matt Fleming , 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" , Junichi Nomura , Ard Biesheuvel , "dyoung@redhat.com" Subject: Re: [PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_* Message-ID: <20170823082425.GA12178@x1> References: <20170710054733.GA22619@hori1.linux.bs1.fc.nec.co.jp> <1499665896-23731-2-git-send-email-n-horiguchi@ah.jp.nec.com> <20170724132044.GB11076@codeblueprint.co.uk> <20170726001230.GA32325@hori1.linux.bs1.fc.nec.co.jp> <20170726011331.GA24304@x1> <20170726013432.GA1117@x1> <20170728064810.GA26208@hori1.linux.bs1.fc.nec.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170728064810.GA26208@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.27]); Wed, 23 Aug 2017 08:24:30 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5996 Lines: 166 Hi Naoya, On 07/28/17 at 06:48am, Naoya Horiguchi wrote: > > > > So I think of adding some assertion in the patch 1/2 to detect this overlap > > > > in extract_kernel() even for no KASLR case. > > > > > > EFI_BOOT_SERVICES_* memory are collected as e820 region of > > > E820_TYPE_RAM, how can we guarantee kernel won't use them after jumping > > > into the running kernel whether KASLR enabled or not? We can only wish > > Current kernels have no such check, so it's not guarantted, I think. > And I guess that most firmwares (luckily?) avoid the overlap, and/or > if the overlap happens, that might not cause any detectable error. > > > > that EFI firmware engineer don't put EFI_BOOT_SERVICES_* far from > > sorry, typo. I meant EFI boot > > service region need be put far from 0x1000000. Otherwise normal kernel could > > allocate memory bottom up and stomp on them. It's embarassment caused by > > the hardware flaw of x86 platfrom. > > > 0x1000000 where normal kernel is loaded. > > > > > > So I think of adding some assertion in the patch 1/2 to detect this overlap > > > > in extract_kernel() even for no KASLR case. > > Anyway I wrote the following patch adding the assertion as a separate one. > I'm glad if I can get your feedback or advise. Sorry, I didn't notice your mail in this deep thread. I think it makes sense to do something about the overlap. Maybe you can repost a patch independently, or put it together with the patchset excluding EFI_BOOT_SERVICES_{CODE|DATA}. Let's see what x86 maintainers and EFI subsystem maintainers will say. By the way, Ingo has helped to put my patchset of kernel mirror handling related to kaslr into tip/x86/boot, you can base your EFI_BOOT_SERVICES_{CODE|DATA} excluding patchset and repost. Maybe start a new thread, it might not be easy to follow in a too deep thread. THanks Baoquan > From: Naoya Horiguchi > Date: Thu, 27 Jul 2017 13:19:35 +0900 > Subject: [PATCH] x86/boot: check overlap between kernel and > EFI_BOOT_SERVICES_* > > Even when KASLR is turned off, kernel still could overlap with > EFI_BOOT_SERVICES_* region, for example because of firmware memmap > layout and/or CONFIG_PHYSICAL_START. > > So this patch adds an assertion that we are free from the overlap > which is called for non-KASLR case. > > Signed-off-by: Naoya Horiguchi > --- > arch/x86/boot/compressed/kaslr.c | 3 --- > arch/x86/boot/compressed/misc.c | 56 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c > index a6604717d4fe..5549c80b45c2 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -721,9 +721,6 @@ void choose_random_location(unsigned long input, > > boot_params->hdr.loadflags |= KASLR_FLAG; > > - /* Prepare to add new identity pagetables on demand. */ > - initialize_identity_maps(); > - > /* Record the various known unsafe memory ranges. */ > mem_avoid_init(input, input_size, *output); > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > index a0838ab929f2..b23159fa159c 100644 > --- a/arch/x86/boot/compressed/misc.c > +++ b/arch/x86/boot/compressed/misc.c > @@ -15,6 +15,8 @@ > #include "error.h" > #include "../string.h" > #include "../voffset.h" > +#include > +#include > > /* > * WARNING!! > @@ -169,6 +171,55 @@ void __puthex(unsigned long value) > } > } > > +#ifdef CONFIG_EFI > +bool __init > +efi_kernel_boot_services_overlap(unsigned long start, unsigned long size) > +{ > + int i; > + u32 nr_desc; > + struct efi_info *e = &boot_params->efi_info; > + efi_memory_desc_t *md; > + char *signature; > + unsigned long pmap; > + > + signature = (char *)&boot_params->efi_info.efi_loader_signature; > + if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) && > + strncmp(signature, EFI64_LOADER_SIGNATURE, 4)) > + return false; > + > +#ifdef CONFIG_X86 /* 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 false; > + } > + pmap = e->efi_memmap; > +#else > + pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32)); > +#endif > + > + add_identity_map(pmap, e->efi_memmap_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->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) <= start) > + continue; > + if (md->phys_addr >= start + size) > + continue; > + if (md->type == EFI_BOOT_SERVICES_CODE || > + md->type == EFI_BOOT_SERVICES_DATA) > + return true; > + } > + return false; > +} > +#else > +bool __init > +efi_kernel_boot_services_overlap(unsigned long start, unsigned long size) > +{ > + return false; > +} > +#endif > + > #if CONFIG_X86_NEED_RELOCS > static void handle_relocations(void *output, unsigned long output_len, > unsigned long virt_addr) > @@ -372,6 +423,9 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap, > debug_putaddr(output_len); > debug_putaddr(kernel_total_size); > > + /* Prepare to add new identity pagetables on demand. */ > + initialize_identity_maps(); > + > /* > * The memory hole needed for the kernel is the larger of either > * the entire decompressed kernel plus relocation table, or the > @@ -402,6 +456,8 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap, > if (virt_addr != LOAD_PHYSICAL_ADDR) > error("Destination virtual address changed when not relocatable"); > #endif > + if (efi_kernel_boot_services_overlap((unsigned long)output, output_len)) > + error("Kernel overlaps EFI_BOOT_SERVICES area"); > > debug_putstr("\nDecompressing Linux... "); > __decompress(input_data, input_len, NULL, NULL, output, output_len, > -- > 2.7.4 > >