Message-ID: <20170823082425.GA12178@x1>
Date:   Wed, 23 Aug 2017 16:24:25 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Naoya Horiguchi <n-horiguchi@...jp.nec.com>
Cc:     Matt Fleming <matt@...eblueprint.co.uk>,
        Kees Cook <keescook@...omium.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
        "izumi.taku@...fujitsu.com" <izumi.taku@...fujitsu.com>,
        Thomas Garnier <thgarnie@...gle.com>,
        "fanc.fnst@...fujitsu.com" <fanc.fnst@...fujitsu.com>,
        Junichi Nomura <j-nomura@...jp.nec.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "dyoung@...hat.com" <dyoung@...hat.com>
Subject: Re: [PATCH] x86/boot: check overlap between kernel and
 EFI_BOOT_SERVICES_*

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 <n-horiguchi@...jp.nec.com>
> 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 <n-horiguchi@...jp.nec.com>
> ---
>  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 <linux/efi.h>
> +#include <asm/efi.h>
>  
>  /*
>   * 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
> 
> 

Powered by blists - more mailing lists