Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1698515

[PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_*

Show all headers | View raw

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 <n-horiguchi@ah.jp.nec.com>
> > > > > ---
> > > > >  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 <n-horiguchi@ah.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@ah.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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH v3 2/2] x86/efi: clean up dead code around  efi_reserve_boot_services() Matt Fleming <matt@codeblueprint.co.uk> - 2017-07-24 15:30 +0200
  Re: [PATCH v3 2/2] x86/efi: clean up dead code around  efi_reserve_boot_services() Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-26 02:20 +0200
    Re: [PATCH v3 2/2] x86/efi: clean up dead code around  efi_reserve_boot_services() Baoquan He <bhe@redhat.com> - 2017-07-26 03:20 +0200
      Re: [PATCH v3 2/2] x86/efi: clean up dead code around  efi_reserve_boot_services() Baoquan He <bhe@redhat.com> - 2017-07-26 03:40 +0200
        [PATCH] x86/boot: check overlap between kernel and  EFI_BOOT_SERVICES_* Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-28 09:00 +0200
          Re: [PATCH] x86/boot: check overlap between kernel and  EFI_BOOT_SERVICES_* kbuild test robot <lkp@intel.com> - 2017-07-29 12:10 +0200
          Re: [PATCH] x86/boot: check overlap between kernel and  EFI_BOOT_SERVICES_* kbuild test robot <lkp@intel.com> - 2017-07-29 15:10 +0200
          [RFC PATCH] x86/boot: efi_kernel_boot_services_overlap can be static kbuild test robot <lkp@intel.com> - 2017-07-29 15:10 +0200

csiph-web