Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751668AbdG1GuJ convert rfc822-to-8bit (ORCPT ); Fri, 28 Jul 2017 02:50:09 -0400 Received: from tyo162.gate.nec.co.jp ([114.179.232.162]:56946 "EHLO tyo162.gate.nec.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751095AbdG1GuI (ORCPT ); Fri, 28 Jul 2017 02:50:08 -0400 From: Naoya Horiguchi To: Baoquan He 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: [PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_* Thread-Topic: [PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_* Thread-Index: AQHTB216wA7QkTOLakGTUD0zwSm2Iw== Date: Fri, 28 Jul 2017 06:48:10 +0000 Message-ID: <20170728064810.GA26208@hori1.linux.bs1.fc.nec.co.jp> 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> In-Reply-To: <20170726013432.GA1117@x1> Accept-Language: en-US, ja-JP Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.128.101.6] Content-Type: text/plain; charset="iso-2022-jp" Content-ID: <3AC6D9A9C3FDA4418513EE947AE3F22A@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: 6150 Lines: 172 On Wed, Jul 26, 2017 at 09:34:32AM +0800, Baoquan He wrote: > On 07/26/17 at 09:13am, Baoquan He wrote: > > On 07/26/17 at 12:12am, Naoya Horiguchi wrote: > > > On Mon, Jul 24, 2017 at 02:20:44PM +0100, Matt Fleming wrote: > > > > On Mon, 10 Jul, at 02:51:36PM, Naoya Horiguchi wrote: > > > > > EFI_BOOT_SERVICES_{CODE|DATA} regions never overlap the kernel now, > > > > > so we can clean up the check in efi_reserve_boot_services(). > > > > > > > > > > Signed-off-by: Naoya Horiguchi > > > > > --- > > > > > arch/x86/platform/efi/quirks.c | 23 +---------------------- > > > > > 1 file changed, 1 insertion(+), 22 deletions(-) > > > > > > > > Is this true for kernels not using KASLR? > > > > > > Thank you for pointing out this. It's not true depending on memmap layout. > > > If a firmware does not define the memory around the kernel address > > > (0x1000000 or CONFIG_PHYSICAL_START) as EFI_BOOT_SERVICES_*, no overlap > > > happens. That's true in my testing server, but I don't think that we can > > > expect it generally. > > > > > > 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. Thanks, Naoya Horiguchi --- 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