[v3] efi: prune invalid memory map entries

Message ID 1481624710-20892-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Dec. 13, 2016, 10:25 a.m.
From: Peter Jones <pjones@redhat.com>


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.

Cc: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Peter Jones <pjones@redhat.com>

[ardb: refactor for clarity with no functional changes, avoid PAGE_SHIFT]
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---

I took the liberty of refactoring the code because I found it difficult
to understand. No functional changes are intended, although I couldn't
figure out if the memcpy() in the original code is (a) correct, and (b)
attempts to copy multiple entries at once.

Also, pr_warn sounds more appropriate for complaining about broken firmware,
but perhaps this is archite-cultural thing as well.

 arch/x86/platform/efi/efi.c | 70 ++++++++++++++++++++
 include/linux/efi.h         |  1 +
 2 files changed, 71 insertions(+)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Matt Fleming Dec. 20, 2016, 12:53 p.m. | #1
On Tue, 13 Dec, at 10:25:10AM, Ard Biesheuvel wrote:
> From: Peter Jones <pjones@redhat.com>

> 

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

> 

> Cc: Matt Fleming <matt@codeblueprint.co.uk>

> Signed-off-by: Peter Jones <pjones@redhat.com>

> [ardb: refactor for clarity with no functional changes, avoid PAGE_SHIFT]

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

> 

> I took the liberty of refactoring the code because I found it difficult

> to understand. No functional changes are intended, although I couldn't

> figure out if the memcpy() in the original code is (a) correct, and (b)

> attempts to copy multiple entries at once.

> 

> Also, pr_warn sounds more appropriate for complaining about broken firmware,

> but perhaps this is archite-cultural thing as well.

> 

>  arch/x86/platform/efi/efi.c | 70 ++++++++++++++++++++

>  include/linux/efi.h         |  1 +

>  2 files changed, 71 insertions(+)

> 

> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c

> index bf99aa7005eb..0a1550b82beb 100644

> --- a/arch/x86/platform/efi/efi.c

> +++ b/arch/x86/platform/efi/efi.c

> @@ -210,6 +210,74 @@ int __init efi_memblock_x86_reserve_range(void)

>  	return 0;

>  }

>  

> +#define OVERFLOW_ADDR_SHIFT	(64 - EFI_PAGE_SHIFT)

> +#define OVERFLOW_ADDR_MASK	(U64_MAX << OVERFLOW_ADDR_SHIFT)

> +#define U64_HIGH_BIT		(~(U64_MAX >> 1))

> +

> +static bool __init efi_memmap_entry_valid(const efi_memory_desc_t *md, int i)

> +{

> +	static __initdata bool once = true;

> +	u64 end = (md->num_pages << EFI_PAGE_SHIFT) + md->phys_addr - 1;

> +	u64 end_hi = 0;

> +	char buf[64];

> +

> +	if (md->num_pages == 0) {

> +		end = 0;

> +	} 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;

> +

> +		if ((md->phys_addr & U64_HIGH_BIT) && !(end & U64_HIGH_BIT))

> +			end_hi += 1;

> +	} else {

> +		return true;

> +	}

> +

> +	if (once) {

> +		pr_warn(FW_BUG "Invalid EFI memory map entries:\n");

> +		once = false;

> +	}


Maybe use pr_warn_once() here instead of rolling your own 'once' flag?

Otherwise this looks fine to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Dec. 24, 2016, 2:10 p.m. | #2
On 20 December 2016 at 12:53, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 13 Dec, at 10:25:10AM, Ard Biesheuvel wrote:

>> From: Peter Jones <pjones@redhat.com>

>>

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

>>

>> Cc: Matt Fleming <matt@codeblueprint.co.uk>

>> Signed-off-by: Peter Jones <pjones@redhat.com>

>> [ardb: refactor for clarity with no functional changes, avoid PAGE_SHIFT]

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>

>> I took the liberty of refactoring the code because I found it difficult

>> to understand. No functional changes are intended, although I couldn't

>> figure out if the memcpy() in the original code is (a) correct, and (b)

>> attempts to copy multiple entries at once.

>>

>> Also, pr_warn sounds more appropriate for complaining about broken firmware,

>> but perhaps this is archite-cultural thing as well.

>>

>>  arch/x86/platform/efi/efi.c | 70 ++++++++++++++++++++

>>  include/linux/efi.h         |  1 +

>>  2 files changed, 71 insertions(+)

>>

>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c

>> index bf99aa7005eb..0a1550b82beb 100644

>> --- a/arch/x86/platform/efi/efi.c

>> +++ b/arch/x86/platform/efi/efi.c

>> @@ -210,6 +210,74 @@ int __init efi_memblock_x86_reserve_range(void)

>>       return 0;

>>  }

>>

>> +#define OVERFLOW_ADDR_SHIFT  (64 - EFI_PAGE_SHIFT)

>> +#define OVERFLOW_ADDR_MASK   (U64_MAX << OVERFLOW_ADDR_SHIFT)

>> +#define U64_HIGH_BIT         (~(U64_MAX >> 1))

>> +

>> +static bool __init efi_memmap_entry_valid(const efi_memory_desc_t *md, int i)

>> +{

>> +     static __initdata bool once = true;

>> +     u64 end = (md->num_pages << EFI_PAGE_SHIFT) + md->phys_addr - 1;

>> +     u64 end_hi = 0;

>> +     char buf[64];

>> +

>> +     if (md->num_pages == 0) {

>> +             end = 0;

>> +     } 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;

>> +

>> +             if ((md->phys_addr & U64_HIGH_BIT) && !(end & U64_HIGH_BIT))

>> +                     end_hi += 1;

>> +     } else {

>> +             return true;

>> +     }

>> +

>> +     if (once) {

>> +             pr_warn(FW_BUG "Invalid EFI memory map entries:\n");

>> +             once = false;

>> +     }

>

> Maybe use pr_warn_once() here instead of rolling your own 'once' flag?

>

> Otherwise this looks fine to me.


OK, I have pushed this patch to efi/next with the suggested modification.

Peter, what else do we need on top?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index bf99aa7005eb..0a1550b82beb 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -210,6 +210,74 @@  int __init efi_memblock_x86_reserve_range(void)
 	return 0;
 }
 
+#define OVERFLOW_ADDR_SHIFT	(64 - EFI_PAGE_SHIFT)
+#define OVERFLOW_ADDR_MASK	(U64_MAX << OVERFLOW_ADDR_SHIFT)
+#define U64_HIGH_BIT		(~(U64_MAX >> 1))
+
+static bool __init efi_memmap_entry_valid(const efi_memory_desc_t *md, int i)
+{
+	static __initdata bool once = true;
+	u64 end = (md->num_pages << EFI_PAGE_SHIFT) + md->phys_addr - 1;
+	u64 end_hi = 0;
+	char buf[64];
+
+	if (md->num_pages == 0) {
+		end = 0;
+	} 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;
+
+		if ((md->phys_addr & U64_HIGH_BIT) && !(end & U64_HIGH_BIT))
+			end_hi += 1;
+	} else {
+		return true;
+	}
+
+	if (once) {
+		pr_warn(FW_BUG "Invalid EFI memory map entries:\n");
+		once = false;
+	}
+
+	if (end_hi) {
+		pr_warn("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_warn("mem%02u: %s range=[0x%016llx-0x%016llx] (invalid)\n",
+			i, efi_md_typeattr_format(buf, sizeof(buf), md),
+			md->phys_addr, end);
+	}
+	return false;
+}
+
+static void __init efi_clean_memmap(void)
+{
+	efi_memory_desc_t *out = efi.memmap.map;
+	const efi_memory_desc_t *in = out;
+	const efi_memory_desc_t *end = efi.memmap.map_end;
+	int i, n_removal;
+
+	for (i = n_removal = 0; in < end; i++) {
+		if (efi_memmap_entry_valid(in, i)) {
+			if (out != in)
+				memcpy(out, in, efi.memmap.desc_size);
+			out = (void *)out + efi.memmap.desc_size;
+		} else {
+			n_removal++;
+		}
+		in = (void *)in + efi.memmap.desc_size;
+	}
+
+	if (n_removal > 0) {
+		u64 size = efi.memmap.nr_map - n_removal;
+
+		pr_warn("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 +540,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 2d089487d2da..fda79cdf9f10 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;