[PATCH 1/4] efi: efi_print_memmap(): Call out invalid entries in the memory map early.

[PATCH 1/4] efi: efi_print_memmap(): Call out invalid entries in the memory map early.

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.
Currently the log output for this case (with efi=debug) looks like:

[    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |  |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)

This is clearly wrong, and also not as informative as it could be.  This
patch changes it so that if we find obviously invalid memory map
entries, say so when we're printing the memory map.  It also detects the
display of the address range calculation overflow, so the new output is:

[    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entry for 0x0 pages at 0x0000000000000000
[    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0x0000000000000000] (0MB)

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 936a488..0263391 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -217,11 +217,27 @@ void __init efi_print_memmap(void)
 
 	for_each_efi_memory_desc(md) {
 		char buf[64];
+		bool valid = true;
+		u64 size = (md->num_pages << EFI_PAGE_SHIFT) - 1;
+
+		if (md->num_pages == 0) {
+			size = 0;
+			valid = false;
+		}
+
+		if (md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT)) {
+			size = (u64)-1LL;
+			valid = false;
+		}
+
+		if (!valid)
+			pr_info(FW_BUG "Invalid EFI memory map entry for 0x%llx pages at 0x%016llx\n",
+				md->num_pages, md->phys_addr);
 
 		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
 			i++, efi_md_typeattr_format(buf, sizeof(buf), md),
 			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
+			md->phys_addr + size,
 			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
 	}
 }
-- 
2.9.3

[PATCH 2/4] efi: efi_map_region(): traceback if we try to map invalid sized regions

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.
We shouldn't ever try to map these errors, so if we get as far as
efi_map_region(), show a traceback.

This additionally makes should_map_region() say not to map them, but I
fixed both places in case another caller of efi_map_region() ever arises
in the future.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c    |  4 ++++
 arch/x86/platform/efi/efi_64.c | 19 ++++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 0263391..d9f7131 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -707,6 +707,10 @@ static bool should_map_region(efi_memory_desc_t *md)
 	if (IS_ENABLED(CONFIG_X86_32))
 		return false;
 
+	if (md->num_pages == 0 ||
+	    md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT))
+		return false;
+
 	/*
 	 * Map all of RAM so that we can access arguments in the 1:1
 	 * mapping when making EFI runtime calls.
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 319148b..7ee5d97 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -313,11 +313,24 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va)
 
 void __init efi_map_region(efi_memory_desc_t *md)
 {
-	unsigned long size = md->num_pages << PAGE_SHIFT;
+	u64 size = md->num_pages << PAGE_SHIFT;
 	u64 pa = md->phys_addr;
 
-	if (efi_enabled(EFI_OLD_MEMMAP))
-		return old_map_region(md);
+	/*
+	 * hah hah the system firmware is having a good one on us
+	 */
+	if (md->num_pages == 0 ||
+	    md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT)) {
+		pr_err("memmap from %p to %p is unreasonable.  Not mapping it.\n",
+		       (void *)pa, (void *)(pa+size));
+		WARN_ON(1);
+		return;
+	}
+
+	if (efi_enabled(EFI_OLD_MEMMAP)) {
+		old_map_region(md);
+		return;
+	}
 
 	/*
 	 * Make sure the 1:1 mappings are present as a catch-all for b0rked
-- 
2.9.3

[PATCH 3/4] efi: efi_memmap_insert(): don't insert a region more than once

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.

When this is true on a machine with an ESRT config table,
efi_esrt_init() will try to reserve that table with efi_mem_reserve().
When it does, efi_arch_mem_reserve() will use efi_mem_desc_lookup() to
find the memory map in question, call efi_memmap_split_count() to
determine how many regions it will be split into, allocates new ram for
the map, and calls efi_memmap_insert() to add the entry.

efi_memmap_insert() iterates across all regions in the old map, and for
each region, it tests whether the address for the new map is within that
region.  Unfortunately we'll try to insert the new map in *each* of
these entries, causing even more duplicate entries.  But
efi_memmap_split_count() only returned the split count for the first
entry, because it was called with the result of efi_mem_desc_lookup(),
which only returns the first entry it came across.  As a result, this
will overflow the memory map's allocation and eventually panic.

Unfortunately the method of computing the size does not handle the
0xffffffffffffffff or 0x0 edge cases well, and all valid addresses wind
up being covered by any such entry.  Even if the math weren't off by 1
byte in the num_pages=0 case, a duplicate entry in the table, or an
entry with num_pages of ((u64)-1LL >> EFI_PAGE_SHIFT) or (u64)-1LL (i.e.
either the maximum number of virtually addressable pages or just the
largest value you can stuff in the field) would have the same effect.

In any case, if we see more than one memory map in efi_memmap_insert()
that covers the same region, it is an error to try to split up more than
one of them, overflowing the allocation.  So this makes it never try to
split more than one region per invocation of efi_arch_mem_reserve().

I have not attempted to address the math error.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/memmap.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index f03ddec..5b71c71 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -219,6 +219,7 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	efi_memory_desc_t *md;
 	u64 start, end;
 	void *old, *new;
+	bool did_split=false;
 
 	/* modifying range */
 	m_start = mem->range.start;
@@ -251,6 +252,15 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 
 		if (m_start <= start &&
 		    (start < m_end && m_end < end)) {
+			if (did_split) {
+				pr_warn_once("%s: memory map for 0x%llx pages at 0x%016llx also covers address 0x%016llx.  Not splitting.\n",
+					     __func__,
+					     md->num_pages, md->phys_addr,
+					     m_start);
+				continue;
+			}
+
+			did_split = true;
 			/* first part */
 			md->attribute |= m_attr;
 			md->num_pages = (m_end - md->phys_addr + 1) >>
@@ -265,9 +275,18 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 		}
 
 		if ((start < m_start && m_start < end) && m_end < end) {
+			if (did_split) {
+				pr_warn_once("%s: memory map for 0x%llx pages at 0x%016llx also covers address 0x%016llx.  Not splitting.\n",
+					     __func__,
+					     md->num_pages, md->phys_addr,
+					     m_start);
+				continue;
+			}
+			did_split = true;
 			/* first part */
 			md->num_pages = (m_start - md->phys_addr) >>
 				EFI_PAGE_SHIFT;
+
 			/* middle part */
 			new += old_memmap->desc_size;
 			memcpy(new, old, old_memmap->desc_size);
@@ -284,9 +303,17 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 			md->num_pages = (end - m_end) >>
 				EFI_PAGE_SHIFT;
 		}
-
+// else
 		if ((start < m_start && m_start < end) &&
 		    (end <= m_end)) {
+			if (did_split) {
+				pr_warn_once("%s: memory map for 0x%llx pages at 0x%016llx also covers address 0x%016llx.  Not splitting.\n",
+					     __func__,
+					     md->num_pages, md->phys_addr,
+					     m_start);
+				continue;
+			}
+			did_split = true;
 			/* first part */
 			md->num_pages = (m_start - md->phys_addr) >>
 				EFI_PAGE_SHIFT;
-- 
2.9.3

[PATCH 4/4] efi: efi_memmap_insert(): don't split regions with invalid sizes.

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.

If we're inserting a new memmap and we find a map that is either 0
pages or all of possible memory (or more!), skip it.  When a map exists
at 0 that's 0 pages, the "end" math here winds up making *every* address
within the range, and so it'll try to split that entry, and things go
poorly after that.  The same would be true if num_pages were (u64)-1LL
(all bits set) or (u64)-1LL >> EFI_PAGE_SHIFT (i.e. all bits set as a
size in bytes, but then shifted to page size to fill the table in).

Don't even try to split those entries, they're nonsense.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/memmap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 5b71c71..f8c6870 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -244,6 +244,13 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 		/* copy original EFI memory descriptor */
 		memcpy(new, old, old_memmap->desc_size);
 		md = new;
+		if (md->num_pages == 0 ||
+		    md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT)) {
+			pr_warn("%s: Skipping absurd memory map entry for 0x%llx pages at 0x%016llx.\n",
+				__func__, md->num_pages, md->phys_addr);
+			continue;
+		}
+
 		start = md->phys_addr;
 		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
 
-- 
2.9.3

Re: [PATCH 1/4] efi: efi_print_memmap(): Call out invalid entries in the memory map early.

[Ard Biesheuvel]

On 7 December 2016 at 21:38, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
> (2.28), include memory map entries with phys_addr=0x0 and num_pages=0.
> Currently the log output for this case (with efi=debug) looks like:
>
> [    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |  |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
>
> This is clearly wrong, and also not as informative as it could be.  This
> patch changes it so that if we find obviously invalid memory map
> entries, say so when we're printing the memory map.  It also detects the
> display of the address range calculation overflow, so the new output is:
>
> [    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entry for 0x0 pages at 0x0000000000000000
> [    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0x0000000000000000] (0MB)
>
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 936a488..0263391 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -217,11 +217,27 @@ void __init efi_print_memmap(void)
>
>         for_each_efi_memory_desc(md) {
>                 char buf[64];
> +               bool valid = true;
> +               u64 size = (md->num_pages << EFI_PAGE_SHIFT) - 1;
> +
> +               if (md->num_pages == 0) {
> +                       size = 0;
> +                       valid = false;
> +               }
> +

This makes sense to me

> +               if (md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT)) {
> +                       size = (u64)-1LL;
> +                       valid = false;
> +               }
> +

... but this not so much: setting size to U64_MAX does not do much to
clarify the line printed below

> +               if (!valid)
> +                       pr_info(FW_BUG "Invalid EFI memory map entry for 0x%llx pages at 0x%016llx\n",
> +                               md->num_pages, md->phys_addr);
>
>                 pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
>                         i++, efi_md_typeattr_format(buf, sizeof(buf), md),
>                         md->phys_addr,
> -                       md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
> +                       md->phys_addr + size,

... given that this wraps around to md->phys_addr -1

>                         (md->num_pages >> (20 - EFI_PAGE_SHIFT)));

... and this still uses the bogus num_pages value

>         }
>  }
> --
> 2.9.3
>

In general, I think it makes sense to disregard entries with num_pages
== 0, and disregarding values of num_pages that are obviously bogus is
also fine (although not as meaningful, given the use case), but that
means we should ignore the whole entry, and refrain from performing
arithmetic involving any of these values, or 'sanitize' the size by
setting it to U64_MAX

Re: [PATCH 1/4] efi: efi_print_memmap(): Call out invalid entries in the memory map early.

[Peter Jones]

On Fri, Dec 09, 2016 at 04:18:04PM +0000, Ard Biesheuvel wrote:
> On 7 December 2016 at 21:38, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
> > (2.28), include memory map entries with phys_addr=0x0 and num_pages=0.
> > Currently the log output for this case (with efi=debug) looks like:
> >
> > [    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |  |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> >
> > This is clearly wrong, and also not as informative as it could be.  This
> > patch changes it so that if we find obviously invalid memory map
> > entries, say so when we're printing the memory map.  It also detects the
> > display of the address range calculation overflow, so the new output is:
> >
> > [    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entry for 0x0 pages at 0x0000000000000000
> > [    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0x0000000000000000] (0MB)
> >
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/platform/efi/efi.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 936a488..0263391 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -217,11 +217,27 @@ void __init efi_print_memmap(void)
> >
> >         for_each_efi_memory_desc(md) {
> >                 char buf[64];
> > +               bool valid = true;
> > +               u64 size = (md->num_pages << EFI_PAGE_SHIFT) - 1;
> > +
> > +               if (md->num_pages == 0) {
> > +                       size = 0;
> > +                       valid = false;
> > +               }
> > +
> 
> This makes sense to me
> 
> > +               if (md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT)) {
> > +                       size = (u64)-1LL;
> > +                       valid = false;
> > +               }
> > +
> 
> ... but this not so much: setting size to U64_MAX does not do much to
> clarify the line printed below
> 
> > +               if (!valid)
> > +                       pr_info(FW_BUG "Invalid EFI memory map entry for 0x%llx pages at 0x%016llx\n",
> > +                               md->num_pages, md->phys_addr);
> >
> >                 pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
> >                         i++, efi_md_typeattr_format(buf, sizeof(buf), md),
> >                         md->phys_addr,
> > -                       md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
> > +                       md->phys_addr + size,
> 
> ... given that this wraps around to md->phys_addr -1
> 
> >                         (md->num_pages >> (20 - EFI_PAGE_SHIFT)));
> 
> ... and this still uses the bogus num_pages value

That's true, I didn't even try to fix the shorthand version.  I've fixed
that to just say (invalid) now instead of trying to print something like
17592186044416MB or whatnot.

> In general, I think it makes sense to disregard entries with num_pages
> == 0, and disregarding values of num_pages that are obviously bogus is
> also fine (although not as meaningful, given the use case), but that
> means we should ignore the whole entry, and refrain from performing
> arithmetic involving any of these values, or 'sanitize' the size by
> setting it to U64_MAX

I've rewritten the first patch with this in mind - now I print the
warning and cull them from the list.  It also does that separately from
efi_print_memmap(), so it still fires even when efi=debug isn't true.

Though given the first patch, it's not clear that the second or fourth
one are really needed any more.  They'd only protect against bad entries
we've added ourselves.  But I've left them intact until we've discussed
this further.

One question - we should probably be doing this on non-x86 as well.
What's the best way to do that?

All that said, this patchset lets the machine in front of me boot.

-- 
  Peter

[PATCH 1/4] efi: prune invalid memory map entries

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.

Currently the log output for this case (with efi=debug) looks like:

[    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |  |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)

This is clearly wrong, and also not as informative as it could be.  This
patch changes it so that if we find obviously invalid memory map
entries, we print an error and those entries.  It also detects the
display of the address range calculation overflow, so the new output is:

[    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
[    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0x0000000000000000] (invalid)

It also detects memory map sizes that would overflow the physical
address, for example phys_addr=0xfffffffffffff000 and
num_pages=0x0200000000000001, and prints:

[    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
[    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[phys_addr=0xfffffffffffff000-0x20ffffffffffffffff] (invalid)

It then removes these entries from the memory map.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 81 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/efi.h         |  1 +
 2 files changed, 82 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index bf99aa7..039b5cf 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -210,6 +210,85 @@ int __init efi_memblock_x86_reserve_range(void)
 	return 0;
 }
 
+#define OVERFLOW_ADDR_SHIFT (64-PAGE_SHIFT)
+#define OVERFLOW_ADDR_MASK (U64_MAX << OVERFLOW_ADDR_SHIFT)
+#define U64_HIGH_BIT (~(U64_MAX >> 1))
+
+void __init efi_clean_memmap(void)
+{
+	int i = 0, n_removal = 0;
+	int extra = 0;
+	void *to = efi.memmap.map;
+	void *from = efi.memmap.map + efi.memmap.desc_size;
+	bool once = true;
+
+	while (to < efi.memmap.map_end - extra) {
+		efi_memory_desc_t *md = (efi_memory_desc_t *)to;
+		char buf[64];
+		bool valid = true;
+
+		u64 end = (md->num_pages << EFI_PAGE_SHIFT);
+		u64 end_hi = 0;
+
+		end += md->phys_addr - 1;
+
+		if (md->num_pages == 0) {
+			end = 0;
+			valid = false;
+		} else if (md->num_pages > EFI_PAGES_MAX ||
+		    EFI_PAGES_MAX - md->num_pages <
+		    (md->phys_addr >> EFI_PAGE_SHIFT)) {
+			end_hi = (md->num_pages & OVERFLOW_ADDR_MASK)
+				>> OVERFLOW_ADDR_SHIFT;
+			valid = false;
+
+			if ((md->phys_addr & U64_HIGH_BIT) &&
+			    !(end & U64_HIGH_BIT))
+				end_hi += 1;
+		}
+
+		if (valid) {
+			to += efi.memmap.desc_size;
+			from += efi.memmap.desc_size;
+		} else {
+			if (once) {
+				pr_info(FW_BUG "Invalid EFI memory map entries:\n");
+				once = false;
+			}
+			n_removal++;
+
+			if (end_hi)
+				pr_info("mem%02u: %s range=[0x%016llx-0x%llx%016llx] (invalid)\n",
+					i, efi_md_typeattr_format(buf,
+								  sizeof(buf),
+								  md),
+					md->phys_addr, end_hi, end);
+			else
+				pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (invalid)\n",
+					i, efi_md_typeattr_format(buf,
+								  sizeof(buf),
+								  md),
+					md->phys_addr, end);
+
+			if ((u64)from < (u64)efi.memmap.map_end - extra)
+				memcpy(to, from,
+				       (u64)efi.memmap.map_end - (u64)from -
+				       extra);
+
+			extra += efi.memmap.desc_size;
+		}
+		i++;
+	}
+	if (extra) {
+		u64 size = ((u64)efi.memmap.map_end -
+			    (u64)efi.memmap.map - extra) /
+			   efi.memmap.desc_size;
+
+		pr_info("Removing %d invalid memory map entries.\n", n_removal);
+		efi_memmap_install(efi.memmap.phys_map, size);
+	}
+}
+
 void __init efi_print_memmap(void)
 {
 	efi_memory_desc_t *md;
@@ -472,6 +551,8 @@ void __init efi_init(void)
 		}
 	}
 
+	efi_clean_memmap();
+
 	if (efi_enabled(EFI_DBG))
 		efi_print_memmap();
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4c1b3ea..712a3aa 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -103,6 +103,7 @@ typedef	struct {
 
 #define EFI_PAGE_SHIFT		12
 #define EFI_PAGE_SIZE		(1UL << EFI_PAGE_SHIFT)
+#define EFI_PAGES_MAX		(U64_MAX >> EFI_PAGE_SHIFT)
 
 typedef struct {
 	u32 type;
-- 
2.9.3

[PATCH 2/4] efi: efi_map_region(): traceback if we try to map invalid sized regions

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.
We shouldn't ever try to map these errors, so if we get as far as
efi_map_region(), show a traceback.

This additionally makes should_map_region() say not to map them, but I
fixed both places in case another caller of efi_map_region() ever arises
in the future.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c    |  5 +++++
 arch/x86/platform/efi/efi_64.c | 17 ++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 039b5cf..90903ce 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -772,6 +772,11 @@ static bool should_map_region(efi_memory_desc_t *md)
 	if (IS_ENABLED(CONFIG_X86_32))
 		return false;
 
+	if (md->num_pages == 0 ||
+	    md->num_pages > EFI_PAGES_MAX ||
+	    EFI_PAGES_MAX - md->num_pages < (md->phys_addr >> EFI_PAGE_SHIFT))
+		return false;
+
 	/*
 	 * Map all of RAM so that we can access arguments in the 1:1
 	 * mapping when making EFI runtime calls.
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index de12d9f..95d429b 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -283,11 +283,22 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va)
 
 void __init efi_map_region(efi_memory_desc_t *md)
 {
-	unsigned long size = md->num_pages << PAGE_SHIFT;
+	u64 size = md->num_pages << PAGE_SHIFT;
 	u64 pa = md->phys_addr;
 
-	if (efi_enabled(EFI_OLD_MEMMAP))
-		return old_map_region(md);
+	if (md->num_pages == 0 ||
+	    md->num_pages > EFI_PAGES_MAX ||
+	    EFI_PAGES_MAX - md->num_pages < (md->phys_addr >> EFI_PAGE_SHIFT)) {
+		pr_err("memmap from %p to %p is unreasonable.  Not mapping it.\n",
+		       (void *)pa, (void *)(pa+size));
+		WARN_ON(1);
+		return;
+	}
+
+	if (efi_enabled(EFI_OLD_MEMMAP)) {
+		old_map_region(md);
+		return;
+	}
 
 	/*
 	 * Make sure the 1:1 mappings are present as a catch-all for b0rked
-- 
2.9.3

[PATCH 3/4] efi: efi_memmap_insert(): don't insert a region more than once

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.

When this is true on a machine with an ESRT config table,
efi_esrt_init() will try to reserve that table with efi_mem_reserve().
When it does, efi_arch_mem_reserve() will use efi_mem_desc_lookup() to
find the memory map in question, call efi_memmap_split_count() to
determine how many regions it will be split into, allocates new ram for
the map, and calls efi_memmap_insert() to add the entry.

efi_memmap_insert() iterates across all regions in the old map, and for
each region, it tests whether the address for the new map is within that
region.  Unfortunately we'll try to insert the new map in *each* of
these entries, causing even more duplicate entries.  But
efi_memmap_split_count() only returned the split count for the first
entry, because it was called with the result of efi_mem_desc_lookup(),
which only returns the first entry it came across.  As a result, this
will overflow the memory map's allocation and eventually panic.

In any case, if we see more than one memory map in efi_memmap_insert()
that covers the same region, it is an error to try to split up more than
one of them, overflowing the allocation.  So this makes it never try to
split more than one region per invocation of efi_arch_mem_reserve().

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/memmap.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index f03ddec..d3ba722 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -219,6 +219,7 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	efi_memory_desc_t *md;
 	u64 start, end;
 	void *old, *new;
+	bool did_split = false;
 
 	/* modifying range */
 	m_start = mem->range.start;
@@ -251,6 +252,15 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 
 		if (m_start <= start &&
 		    (start < m_end && m_end < end)) {
+			if (did_split) {
+				pr_warn_once("%s: memory map for 0x%llx pages at 0x%016llx also covers address 0x%016llx.  Not splitting.\n",
+					     __func__,
+					     md->num_pages, md->phys_addr,
+					     m_start);
+				continue;
+			}
+
+			did_split = true;
 			/* first part */
 			md->attribute |= m_attr;
 			md->num_pages = (m_end - md->phys_addr + 1) >>
@@ -265,9 +275,18 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 		}
 
 		if ((start < m_start && m_start < end) && m_end < end) {
+			if (did_split) {
+				pr_warn_once("%s: memory map for 0x%llx pages at 0x%016llx also covers address 0x%016llx.  Not splitting.\n",
+					     __func__,
+					     md->num_pages, md->phys_addr,
+					     m_start);
+				continue;
+			}
+			did_split = true;
 			/* first part */
 			md->num_pages = (m_start - md->phys_addr) >>
 				EFI_PAGE_SHIFT;
+
 			/* middle part */
 			new += old_memmap->desc_size;
 			memcpy(new, old, old_memmap->desc_size);
@@ -287,6 +306,14 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 
 		if ((start < m_start && m_start < end) &&
 		    (end <= m_end)) {
+			if (did_split) {
+				pr_warn_once("%s: memory map for 0x%llx pages at 0x%016llx also covers address 0x%016llx.  Not splitting.\n",
+					     __func__,
+					     md->num_pages, md->phys_addr,
+					     m_start);
+				continue;
+			}
+			did_split = true;
 			/* first part */
 			md->num_pages = (m_start - md->phys_addr) >>
 				EFI_PAGE_SHIFT;
-- 
2.9.3

[PATCH 4/4] efi: efi_memmap_insert(): don't split regions with invalid sizes.

[Peter Jones]

Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
(2.28), include memory map entries with phys_addr=0x0 and num_pages=0.

If we're inserting a new memmap and we find a map that is either 0
pages or all of possible memory (or more!), skip it.  When a map exists
at 0 that's 0 pages, the "end" math here winds up making *every* address
within the range, and so it'll try to split that entry, and things go
poorly after that.  The same would be true if num_pages were U64_MAX or
(U64_MAX >> EFI_PAGE_SHIFT) (i.e. all bits set as a size in bytes, but
then shifted to page size to fill the table in).

Don't even try to split those entries, they're nonsense.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efi/memmap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index d3ba722..4503971 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -244,6 +244,15 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 		/* copy original EFI memory descriptor */
 		memcpy(new, old, old_memmap->desc_size);
 		md = new;
+		if (md->num_pages == 0 ||
+		    md->num_pages > EFI_PAGES_MAX ||
+		    EFI_PAGES_MAX - md->num_pages <
+		    (md->phys_addr >> EFI_PAGE_SHIFT)) {
+			pr_warn("%s: Skipping absurd memory map entry for 0x%llx pages at 0x%016llx.\n",
+				__func__, md->num_pages, md->phys_addr);
+			continue;
+		}
+
 		start = md->phys_addr;
 		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
 
-- 
2.9.3

Re: [PATCH 1/4] efi: efi_print_memmap(): Call out invalid entries in the memory map early.

[Ard Biesheuvel]

On 12 December 2016 at 23:40, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Dec 09, 2016 at 04:18:04PM +0000, Ard Biesheuvel wrote:
>> On 7 December 2016 at 21:38, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > Some machines, such as the Lenovo ThinkPad W541 with firmware GNET80WW
>> > (2.28), include memory map entries with phys_addr=0x0 and num_pages=0.
>> > Currently the log output for this case (with efi=debug) looks like:
>> >
>> > [    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |  |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
>> >
>> > This is clearly wrong, and also not as informative as it could be.  This
>> > patch changes it so that if we find obviously invalid memory map
>> > entries, say so when we're printing the memory map.  It also detects the
>> > display of the address range calculation overflow, so the new output is:
>> >
>> > [    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entry for 0x0 pages at 0x0000000000000000
>> > [    0.000000] efi: mem45: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0x0000000000000000] (0MB)
>> >
>> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  arch/x86/platform/efi/efi.c | 18 +++++++++++++++++-
>> >  1 file changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> > index 936a488..0263391 100644
>> > --- a/arch/x86/platform/efi/efi.c
>> > +++ b/arch/x86/platform/efi/efi.c
>> > @@ -217,11 +217,27 @@ void __init efi_print_memmap(void)
>> >
>> >         for_each_efi_memory_desc(md) {
>> >                 char buf[64];
>> > +               bool valid = true;
>> > +               u64 size = (md->num_pages << EFI_PAGE_SHIFT) - 1;
>> > +
>> > +               if (md->num_pages == 0) {
>> > +                       size = 0;
>> > +                       valid = false;
>> > +               }
>> > +
>>
>> This makes sense to me
>>
>> > +               if (md->num_pages >= (((u64)-1LL) >> EFI_PAGE_SHIFT)) {
>> > +                       size = (u64)-1LL;
>> > +                       valid = false;
>> > +               }
>> > +
>>
>> ... but this not so much: setting size to U64_MAX does not do much to
>> clarify the line printed below
>>
>> > +               if (!valid)
>> > +                       pr_info(FW_BUG "Invalid EFI memory map entry for 0x%llx pages at 0x%016llx\n",
>> > +                               md->num_pages, md->phys_addr);
>> >
>> >                 pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
>> >                         i++, efi_md_typeattr_format(buf, sizeof(buf), md),
>> >                         md->phys_addr,
>> > -                       md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
>> > +                       md->phys_addr + size,
>>
>> ... given that this wraps around to md->phys_addr -1
>>
>> >                         (md->num_pages >> (20 - EFI_PAGE_SHIFT)));
>>
>> ... and this still uses the bogus num_pages value
>
> That's true, I didn't even try to fix the shorthand version.  I've fixed
> that to just say (invalid) now instead of trying to print something like
> 17592186044416MB or whatnot.
>

Much better, thanks.

>> In general, I think it makes sense to disregard entries with num_pages
>> == 0, and disregarding values of num_pages that are obviously bogus is
>> also fine (although not as meaningful, given the use case), but that
>> means we should ignore the whole entry, and refrain from performing
>> arithmetic involving any of these values, or 'sanitize' the size by
>> setting it to U64_MAX
>
> I've rewritten the first patch with this in mind - now I print the
> warning and cull them from the list.  It also does that separately from
> efi_print_memmap(), so it still fires even when efi=debug isn't true.
>

Good

> Though given the first patch, it's not clear that the second or fourth
> one are really needed any more.  They'd only protect against bad entries
> we've added ourselves.  But I've left them intact until we've discussed
> this further.
>
> One question - we should probably be doing this on non-x86 as well.
> What's the best way to do that?
>

I don't think there is any point to going out of our way to recheck
for bogus entries if we've already removed them from our copy of the
memory map.

As far as ARM/arm64 are concerned: we are still in normative mode, and
firmware that breaks booting on Linux is unlikely to ship. Instead, we
should probably push for a clarification in the spec that the memory
map should not contain entries with a zero page count. (Other bogus
entries are already banned implicitly since they cannot accurately
reflect reality)

> All that said, this patchset lets the machine in front of me boot.
>

Yes, I understood that this exercise was not for entertainment :-)

Re: [PATCH 1/4] efi: efi_print_memmap(): Call out invalid entries in the memory map early.

[Peter Jones]

On Tue, Dec 13, 2016 at 09:19:26AM +0000, Ard Biesheuvel wrote:
> 
> > Though given the first patch, it's not clear that the second or fourth
> > one are really needed any more.  They'd only protect against bad entries
> > we've added ourselves.  But I've left them intact until we've discussed
> > this further.
> >
> > One question - we should probably be doing this on non-x86 as well.
> > What's the best way to do that?
> >
> 
> I don't think there is any point to going out of our way to recheck
> for bogus entries if we've already removed them from our copy of the
> memory map.

Yeah, that sounds reasonable enough.  We probably should still do the
third patch in the series, btw.  I wrote that one before I realized
these were 0-page maps instead of U64_MAX-page maps, and the end
calculations underrun if the num_pages value is 0.  So the result was
that the 0-page entries all had U64_MAX sizes (and thus spanned all of
memory), but I think it's still relevant even once we've made those
entries not exist.

AFAICS if there's ever a second entry that matches the same address
efi_memmap_insert() will split more times than efi_memmap_split_count()
indicates, and that's clearly not as intended.  And I can easily believe
in a situation where some misguided firmware vendor has a call that says
RegisterConfigurationTable(addr, size) that creates a new
EfiRuntimeServicesData memory map entry, but refuses to split up an
entry that's EfiACPIMemoryNVS when they call it on ACPI table data.
It's just too easy to get wrong.

I'm also really thinking the three big clauses in efi_memmap_insert()
should have an "else" stuck between each of them, since the case of
split_count=3 is clearly the middle one, and it's not supposed to
reflect that the first and third case might each fire.   But since the
bug can trigger over multiple entries, and we have to walk the whole
table each time, we need to ensure it will only trigger once across the
whole run.

And, you know, it's not like any of us believe we won't see more memory
maps with errors in the future.

> As far as ARM/arm64 are concerned: we are still in normative mode, and
> firmware that breaks booting on Linux is unlikely to ship. Instead, we
> should probably push for a clarification in the spec that the memory
> map should not contain entries with a zero page count. (Other bogus
> entries are already banned implicitly since they cannot accurately
> reflect reality)

Fair enough regarding ARM - must be nice ;)  I'll write up an ECR for
USWG to handle the rest of the world.  I'm going to add language to
explicitly ban the impossible values as well - belt and suspenders, etc.
It should be pretty straightforward.

Thanks!

-- 
  Peter