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

Groups > linux.kernel > #1694706 > unrolled thread

Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

#1694706 — Re: [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services()

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? 

[toc] | [next] | [standalone]

#1696678

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.

Thanks,
Naoya Horiguchi

[toc] | [prev] | [next] | [standalone]

#1696706

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
that EFI firmware engineer don't put EFI_BOOT_SERVICES_* far from
0x1000000 where normal kernel is loaded.

[toc] | [prev] | [next] | [standalone]

#1696715

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
> 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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1699285 — Re: [PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_*

[Multipart message — attachments visible in raw view] — view raw

Hi Naoya,

[auto build test ERROR on efi/next]
[also build test ERROR on v4.13-rc2 next-20170728]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Naoya-Horiguchi/x86-boot-check-overlap-between-kernel-and-EFI_BOOT_SERVICES_/20170729-170209
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: x86_64-lkp (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/boot/compressed/misc.o: In function `extract_kernel':
>> misc.c:(.text+0x2cf9): undefined reference to `initialize_identity_maps'
   arch/x86/boot/compressed/misc.o: In function `efi_kernel_boot_services_overlap':
>> misc.c:(.init.text+0x7b): undefined reference to `add_identity_map'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1699307 — Re: [PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_*

Hi Naoya,

[auto build test WARNING on efi/next]
[also build test WARNING on v4.13-rc2 next-20170728]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Naoya-Horiguchi/x86-boot-check-overlap-between-kernel-and-EFI_BOOT_SERVICES_/20170729-170209
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)


Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1699308 — [RFC PATCH] x86/boot: efi_kernel_boot_services_overlap can be static

Fixes: 7ebe6cf3a6da ("x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_*")
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 misc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b23159f..981d5f3 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -172,7 +172,7 @@ void __puthex(unsigned long value)
 }
 
 #ifdef CONFIG_EFI
-bool __init
+static bool __init
 efi_kernel_boot_services_overlap(unsigned long start, unsigned long size)
 {
 	int i;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web