[PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Dave Young]

Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
(emulated the panic with using same invalid phys addr in a uefi vm)
There are also a bug in bugzilla.kernel.org:
https://bugzilla.kernel.org/show_bug.cgi?id=195633

This happens after below commit:
7b0a911 efi/x86: Move the EFI BGRT init code to early init code

The root cause is the firmware on those machines provides invalid bgrt
image addresses.

With original efi bgrt code we initialize bgrt late
and use ioremap to map the image address. In ioremap code we check the 
address is a valid physical address or not before really map it.  

With current new efi bgrt code we moved the initialization to early code
so we switch to early_memremap which does not check the phys_addr like
ioremap does. This lead to the early kernel panics.

Fix this by checking the image physical address, if it is not within
any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
then the original ioremap checking, according to spec the BGRT data
should fall into EFI_BOOT_SERVICES_DATA.

[    0.000000] BUG: unable to handle kernel paging request at ffffffffff280001
[    0.000000] IP: efi_bgrt_init+0xfb/0x153
[    0.000000] PGD 6e00b067 
[    0.000000] P4D 6e00b067 
[    0.000000] PUD 6e00d067 
[    0.000000] PMD 6e221067 
[    0.000000] PTE 8a08e01800000163
[    0.000000] 
[    0.000000] Oops: 0009 [#1] SMP
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.12.0-rc4+ #135
[    0.000000] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[    0.000000] task: ffffffff9840f4c0 task.stack: ffffffff98400000
[    0.000000] RIP: 0010:efi_bgrt_init+0xfb/0x153
[    0.000000] RSP: 0000:ffffffff98403d50 EFLAGS: 00010082
[    0.000000] RAX: ffffffffff280001 RBX: 0000000000000000 RCX: 0000000000000006
[    0.000000] RDX: 0a08e01800001000 RSI: 8a08e01800000163 RDI: 000000000000057e
[    0.000000] RBP: ffffffff98403d68 R08: 0000000000000041 R09: 0000000000000002
[    0.000000] R10: 0000000000000000 R11: ffff8c063cff8fc6 R12: ffffffff981d1fb2
[    0.000000] R13: ffffffff986b4fa0 R14: 0000000000000010 R15: 0000000000000000
[    0.000000] FS:  0000000000000000(0000) GS:ffffffff984db000(0000) knlGS:0000000000000000
[    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.000000] CR2: ffffffffff280001 CR3: 000000006e00a000 CR4: 00000000000406b0
[    0.000000] Call Trace:
[    0.000000]  ? bgrt_init+0xbc/0xbc
[    0.000000]  acpi_parse_bgrt+0xe/0x12
[    0.000000]  acpi_table_parse+0x89/0xb8
[    0.000000]  acpi_boot_init+0x445/0x4e2
[    0.000000]  ? acpi_parse_x2apic+0x79/0x79
[    0.000000]  ? dmi_ignore_irq0_timer_override+0x33/0x33
[    0.000000]  setup_arch+0xb63/0xc82
[    0.000000]  ? early_idt_handler_array+0x120/0x120
[    0.000000]  start_kernel+0xb7/0x443
[    0.000000]  ? early_idt_handler_array+0x120/0x120
[    0.000000]  x86_64_start_reservations+0x29/0x2b
[    0.000000]  x86_64_start_kernel+0x154/0x177
[    0.000000]  secondary_startup_64+0x9f/0x9f
[    0.000000] Code: 3f ff eb 6c 48 bf 01 00 00 00 18 e0 08 0a be 06 00 00 00 e8 ef 2b fe ff 48 85 c0 75 0e 48 c7 c7 88 09 22 98 e8 e1 31 3f ff eb 45 <66> 44 8b 20 be 06 00 00 00 48 89 c7 8b 58 02 e8 91 2c fe ff 66 
[    0.000000] RIP: efi_bgrt_init+0xfb/0x153 RSP: ffffffff98403d50
[    0.000000] CR2: ffffffffff280001
[    0.000000] ---[ end trace 9843d3b7cbcab26a ]---
[    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!

Fixes: 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
Reported-by: Maniaxx <tripleshiftone@gmail.com>
Signed-off-by: Dave Young <dyoung@redhat.com>
---
 drivers/firmware/efi/efi-bgrt.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

--- linux.orig/drivers/firmware/efi/efi-bgrt.c
+++ linux/drivers/firmware/efi/efi-bgrt.c
@@ -27,6 +27,31 @@ struct bmp_header {
 	u32 size;
 } __packed;
 
+static bool efi_bgrt_addr_valid(u64 addr)
+{
+	efi_memory_desc_t *md;
+
+	if (!efi_enabled(EFI_MEMMAP)) {
+		pr_err("EFI_MEMMAP is not enabled.\n");
+		return true;
+	}
+
+	for_each_efi_memory_desc(md) {
+		u64 size;
+		u64 end;
+
+		if (md->type != EFI_BOOT_SERVICES_DATA)
+			continue;
+
+		size = md->num_pages << EFI_PAGE_SHIFT;
+		end = md->phys_addr + size;
+		if (addr >= md->phys_addr && addr < end)
+			return true;
+	}
+
+	return false;
+}
+
 void __init efi_bgrt_init(struct acpi_table_header *table)
 {
 	void *image;
@@ -65,6 +90,10 @@ void __init efi_bgrt_init(struct acpi_ta
 		goto out;
 	}
 
+	if (!efi_bgrt_addr_valid(bgrt->image_address)) {
+		pr_notice("Ignoring BGRT: invalid image address\n");
+		goto out;
+	}
 	image = early_memremap(bgrt->image_address, sizeof(bmp_header));
 	if (!image) {
 		pr_notice("Ignoring BGRT: failed to map image header memory\n");

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Dave Young]

The subject tag should be efi instead of x86/efi since the code
is in general driver code now. Matt/Ard, if need resend please let me
know. Please help review the patch.

Maniaxx, can you verify it on your machine? It passed my test with
an emulation of your wrong address.

On 06/08/17 at 01:32pm, Dave Young wrote:
> Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
> (emulated the panic with using same invalid phys addr in a uefi vm)
> There are also a bug in bugzilla.kernel.org:
> https://bugzilla.kernel.org/show_bug.cgi?id=195633
> 
> This happens after below commit:
> 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> 
> The root cause is the firmware on those machines provides invalid bgrt
> image addresses.
> 
> With original efi bgrt code we initialize bgrt late
> and use ioremap to map the image address. In ioremap code we check the 
> address is a valid physical address or not before really map it.  
> 
> With current new efi bgrt code we moved the initialization to early code
> so we switch to early_memremap which does not check the phys_addr like
> ioremap does. This lead to the early kernel panics.
> 
> Fix this by checking the image physical address, if it is not within
> any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
> then the original ioremap checking, according to spec the BGRT data
> should fall into EFI_BOOT_SERVICES_DATA.
> 
> [    0.000000] BUG: unable to handle kernel paging request at ffffffffff280001
> [    0.000000] IP: efi_bgrt_init+0xfb/0x153
> [    0.000000] PGD 6e00b067 
> [    0.000000] P4D 6e00b067 
> [    0.000000] PUD 6e00d067 
> [    0.000000] PMD 6e221067 
> [    0.000000] PTE 8a08e01800000163
> [    0.000000] 
> [    0.000000] Oops: 0009 [#1] SMP
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.12.0-rc4+ #135
> [    0.000000] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> [    0.000000] task: ffffffff9840f4c0 task.stack: ffffffff98400000
> [    0.000000] RIP: 0010:efi_bgrt_init+0xfb/0x153
> [    0.000000] RSP: 0000:ffffffff98403d50 EFLAGS: 00010082
> [    0.000000] RAX: ffffffffff280001 RBX: 0000000000000000 RCX: 0000000000000006
> [    0.000000] RDX: 0a08e01800001000 RSI: 8a08e01800000163 RDI: 000000000000057e
> [    0.000000] RBP: ffffffff98403d68 R08: 0000000000000041 R09: 0000000000000002
> [    0.000000] R10: 0000000000000000 R11: ffff8c063cff8fc6 R12: ffffffff981d1fb2
> [    0.000000] R13: ffffffff986b4fa0 R14: 0000000000000010 R15: 0000000000000000
> [    0.000000] FS:  0000000000000000(0000) GS:ffffffff984db000(0000) knlGS:0000000000000000
> [    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.000000] CR2: ffffffffff280001 CR3: 000000006e00a000 CR4: 00000000000406b0
> [    0.000000] Call Trace:
> [    0.000000]  ? bgrt_init+0xbc/0xbc
> [    0.000000]  acpi_parse_bgrt+0xe/0x12
> [    0.000000]  acpi_table_parse+0x89/0xb8
> [    0.000000]  acpi_boot_init+0x445/0x4e2
> [    0.000000]  ? acpi_parse_x2apic+0x79/0x79
> [    0.000000]  ? dmi_ignore_irq0_timer_override+0x33/0x33
> [    0.000000]  setup_arch+0xb63/0xc82
> [    0.000000]  ? early_idt_handler_array+0x120/0x120
> [    0.000000]  start_kernel+0xb7/0x443
> [    0.000000]  ? early_idt_handler_array+0x120/0x120
> [    0.000000]  x86_64_start_reservations+0x29/0x2b
> [    0.000000]  x86_64_start_kernel+0x154/0x177
> [    0.000000]  secondary_startup_64+0x9f/0x9f
> [    0.000000] Code: 3f ff eb 6c 48 bf 01 00 00 00 18 e0 08 0a be 06 00 00 00 e8 ef 2b fe ff 48 85 c0 75 0e 48 c7 c7 88 09 22 98 e8 e1 31 3f ff eb 45 <66> 44 8b 20 be 06 00 00 00 48 89 c7 8b 58 02 e8 91 2c fe ff 66 
> [    0.000000] RIP: efi_bgrt_init+0xfb/0x153 RSP: ffffffff98403d50
> [    0.000000] CR2: ffffffffff280001
> [    0.000000] ---[ end trace 9843d3b7cbcab26a ]---
> [    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
> 
> Fixes: 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> Reported-by: Maniaxx <tripleshiftone@gmail.com>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  drivers/firmware/efi/efi-bgrt.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> --- linux.orig/drivers/firmware/efi/efi-bgrt.c
> +++ linux/drivers/firmware/efi/efi-bgrt.c
> @@ -27,6 +27,31 @@ struct bmp_header {
>  	u32 size;
>  } __packed;
>  
> +static bool efi_bgrt_addr_valid(u64 addr)
> +{
> +	efi_memory_desc_t *md;
> +
> +	if (!efi_enabled(EFI_MEMMAP)) {
> +		pr_err("EFI_MEMMAP is not enabled.\n");
> +		return true;
> +	}
> +
> +	for_each_efi_memory_desc(md) {
> +		u64 size;
> +		u64 end;
> +
> +		if (md->type != EFI_BOOT_SERVICES_DATA)
> +			continue;
> +
> +		size = md->num_pages << EFI_PAGE_SHIFT;
> +		end = md->phys_addr + size;
> +		if (addr >= md->phys_addr && addr < end)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  void __init efi_bgrt_init(struct acpi_table_header *table)
>  {
>  	void *image;
> @@ -65,6 +90,10 @@ void __init efi_bgrt_init(struct acpi_ta
>  		goto out;
>  	}
>  
> +	if (!efi_bgrt_addr_valid(bgrt->image_address)) {
> +		pr_notice("Ignoring BGRT: invalid image address\n");
> +		goto out;
> +	}
>  	image = early_memremap(bgrt->image_address, sizeof(bmp_header));
>  	if (!image) {
>  		pr_notice("Ignoring BGRT: failed to map image header memory\n");

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Ard Biesheuvel]

On 8 June 2017 at 05:32, Dave Young <dyoung@redhat.com> wrote:
> Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
> (emulated the panic with using same invalid phys addr in a uefi vm)
> There are also a bug in bugzilla.kernel.org:
> https://bugzilla.kernel.org/show_bug.cgi?id=195633
>
> This happens after below commit:
> 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
>
> The root cause is the firmware on those machines provides invalid bgrt
> image addresses.
>
> With original efi bgrt code we initialize bgrt late
> and use ioremap to map the image address. In ioremap code we check the
> address is a valid physical address or not before really map it.
>
> With current new efi bgrt code we moved the initialization to early code
> so we switch to early_memremap which does not check the phys_addr like
> ioremap does. This lead to the early kernel panics.
>
> Fix this by checking the image physical address, if it is not within
> any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
> then the original ioremap checking, according to spec the BGRT data
> should fall into EFI_BOOT_SERVICES_DATA.
>

Which spec? The UEFI spec does not mention BGRT, and given that it is
an ACPI table, I would expect an ACPI reclaim region to be the most
appropriate. A quick test with QEMU confirms this:

ACPI: BGRT 0x000000013A5E0000 000038 (v01 INTEL  EDK2     00000002
 01000013)

and

efi:   0x00013a5e0000-0x00013a5effff [ACPI Reclaim Memory|   |  |  |
|  |  |  |   |WB|  |  |  ]

So while I agree that we have to fix this, and that checking the BGRT
address against the UEFI memory map is the most appropriate course of
action, requiring a certain region type is probably not what we want.

We have a similar check for ESRT, in efi_mem_desc_lookup(), which
looks a bit dodgy tbh, given that it allows any region type (including
MMIO), as long as it has the EFI_MEMORY_RUNTIME attribute, which is
almost certainly incorrect.

So what I would like to see is a function that can tell you whether a
certain address is covered by a region of a type that is normal
memory, and is occupied, i.e.,

EFI_RESERVED_TYPE
EFI_LOADER_CODE
EFI_LOADER_DATA
EFI_BOOT_SERVICES_CODE
EFI_BOOT_SERVICES_DATA
EFI_RUNTIME_SERVICES_CODE
EFI_RUNTIME_SERVICES_DATA
EFI_ACPI_RECLAIM_MEMORY
EFI_ACPI_MEMORY_NVS

The EFI_MEMORY_RUNTIME attribute is irrelevant: the firmware itself
does not have to read these tables at runtime, so it doesn't matter
whether the O/S maps them on its behalf.

If you could please stick that in drivers/firmware/efi/efi.c, and
rework the patch to use it instead? I will move the ESRT code to it as
well once this is merged.


> [    0.000000] BUG: unable to handle kernel paging request at ffffffffff280001
> [    0.000000] IP: efi_bgrt_init+0xfb/0x153
> [    0.000000] PGD 6e00b067
> [    0.000000] P4D 6e00b067
> [    0.000000] PUD 6e00d067
> [    0.000000] PMD 6e221067
> [    0.000000] PTE 8a08e01800000163
> [    0.000000]
> [    0.000000] Oops: 0009 [#1] SMP
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.12.0-rc4+ #135
> [    0.000000] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> [    0.000000] task: ffffffff9840f4c0 task.stack: ffffffff98400000
> [    0.000000] RIP: 0010:efi_bgrt_init+0xfb/0x153
> [    0.000000] RSP: 0000:ffffffff98403d50 EFLAGS: 00010082
> [    0.000000] RAX: ffffffffff280001 RBX: 0000000000000000 RCX: 0000000000000006
> [    0.000000] RDX: 0a08e01800001000 RSI: 8a08e01800000163 RDI: 000000000000057e
> [    0.000000] RBP: ffffffff98403d68 R08: 0000000000000041 R09: 0000000000000002
> [    0.000000] R10: 0000000000000000 R11: ffff8c063cff8fc6 R12: ffffffff981d1fb2
> [    0.000000] R13: ffffffff986b4fa0 R14: 0000000000000010 R15: 0000000000000000
> [    0.000000] FS:  0000000000000000(0000) GS:ffffffff984db000(0000) knlGS:0000000000000000
> [    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.000000] CR2: ffffffffff280001 CR3: 000000006e00a000 CR4: 00000000000406b0
> [    0.000000] Call Trace:
> [    0.000000]  ? bgrt_init+0xbc/0xbc
> [    0.000000]  acpi_parse_bgrt+0xe/0x12
> [    0.000000]  acpi_table_parse+0x89/0xb8
> [    0.000000]  acpi_boot_init+0x445/0x4e2
> [    0.000000]  ? acpi_parse_x2apic+0x79/0x79
> [    0.000000]  ? dmi_ignore_irq0_timer_override+0x33/0x33
> [    0.000000]  setup_arch+0xb63/0xc82
> [    0.000000]  ? early_idt_handler_array+0x120/0x120
> [    0.000000]  start_kernel+0xb7/0x443
> [    0.000000]  ? early_idt_handler_array+0x120/0x120
> [    0.000000]  x86_64_start_reservations+0x29/0x2b
> [    0.000000]  x86_64_start_kernel+0x154/0x177
> [    0.000000]  secondary_startup_64+0x9f/0x9f
> [    0.000000] Code: 3f ff eb 6c 48 bf 01 00 00 00 18 e0 08 0a be 06 00 00 00 e8 ef 2b fe ff 48 85 c0 75 0e 48 c7 c7 88 09 22 98 e8 e1 31 3f ff eb 45 <66> 44 8b 20 be 06 00 00 00 48 89 c7 8b 58 02 e8 91 2c fe ff 66
> [    0.000000] RIP: efi_bgrt_init+0xfb/0x153 RSP: ffffffff98403d50
> [    0.000000] CR2: ffffffffff280001
> [    0.000000] ---[ end trace 9843d3b7cbcab26a ]---
> [    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>
> Fixes: 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> Reported-by: Maniaxx <tripleshiftone@gmail.com>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  drivers/firmware/efi/efi-bgrt.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> --- linux.orig/drivers/firmware/efi/efi-bgrt.c
> +++ linux/drivers/firmware/efi/efi-bgrt.c
> @@ -27,6 +27,31 @@ struct bmp_header {
>         u32 size;
>  } __packed;
>
> +static bool efi_bgrt_addr_valid(u64 addr)
> +{
> +       efi_memory_desc_t *md;
> +
> +       if (!efi_enabled(EFI_MEMMAP)) {
> +               pr_err("EFI_MEMMAP is not enabled.\n");
> +               return true;
> +       }
> +
> +       for_each_efi_memory_desc(md) {
> +               u64 size;
> +               u64 end;
> +
> +               if (md->type != EFI_BOOT_SERVICES_DATA)
> +                       continue;
> +
> +               size = md->num_pages << EFI_PAGE_SHIFT;
> +               end = md->phys_addr + size;
> +               if (addr >= md->phys_addr && addr < end)
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  void __init efi_bgrt_init(struct acpi_table_header *table)
>  {
>         void *image;
> @@ -65,6 +90,10 @@ void __init efi_bgrt_init(struct acpi_ta
>                 goto out;
>         }
>
> +       if (!efi_bgrt_addr_valid(bgrt->image_address)) {
> +               pr_notice("Ignoring BGRT: invalid image address\n");
> +               goto out;
> +       }
>         image = early_memremap(bgrt->image_address, sizeof(bmp_header));
>         if (!image) {
>                 pr_notice("Ignoring BGRT: failed to map image header memory\n");

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Maniaxx]

On 08.06.2017 at 07:38 wrote Dave Young:
> Maniaxx, can you verify it on your machine? It passed my test with
> an emulation of your wrong address.

Works. Bails out properly on 4.12.0-rc4-gb29794ec95c6.

[    0.000000] efi_bgrt: Ignoring BGRT: invalid image address

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Dave Young]

On 06/08/17 at 10:02am, Ard Biesheuvel wrote:
> On 8 June 2017 at 05:32, Dave Young <dyoung@redhat.com> wrote:
> > Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
> > (emulated the panic with using same invalid phys addr in a uefi vm)
> > There are also a bug in bugzilla.kernel.org:
> > https://bugzilla.kernel.org/show_bug.cgi?id=195633
> >
> > This happens after below commit:
> > 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> >
> > The root cause is the firmware on those machines provides invalid bgrt
> > image addresses.
> >
> > With original efi bgrt code we initialize bgrt late
> > and use ioremap to map the image address. In ioremap code we check the
> > address is a valid physical address or not before really map it.
> >
> > With current new efi bgrt code we moved the initialization to early code
> > so we switch to early_memremap which does not check the phys_addr like
> > ioremap does. This lead to the early kernel panics.
> >
> > Fix this by checking the image physical address, if it is not within
> > any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
> > then the original ioremap checking, according to spec the BGRT data
> > should fall into EFI_BOOT_SERVICES_DATA.
> >
> 
> Which spec? The UEFI spec does not mention BGRT, and given that it is

It is mentioned in ACPI spec, see 6.1 spec section 5.2.22.4
http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf

> an ACPI table, I would expect an ACPI reclaim region to be the most
> appropriate. A quick test with QEMU confirms this:
> 
> ACPI: BGRT 0x000000013A5E0000 000038 (v01 INTEL  EDK2     00000002
>  01000013)
> 
> and
> 
> efi:   0x00013a5e0000-0x00013a5effff [ACPI Reclaim Memory|   |  |  |
> |  |  |  |   |WB|  |  |  ]
> 
> So while I agree that we have to fix this, and that checking the BGRT
> address against the UEFI memory map is the most appropriate course of
> action, requiring a certain region type is probably not what we want.
> 
> We have a similar check for ESRT, in efi_mem_desc_lookup(), which
> looks a bit dodgy tbh, given that it allows any region type (including
> MMIO), as long as it has the EFI_MEMORY_RUNTIME attribute, which is
> almost certainly incorrect.

Yes, I had that in mind but it delayed for something else ..

> 
> So what I would like to see is a function that can tell you whether a
> certain address is covered by a region of a type that is normal
> memory, and is occupied, i.e.,
> 
> EFI_RESERVED_TYPE
> EFI_LOADER_CODE
> EFI_LOADER_DATA
> EFI_BOOT_SERVICES_CODE
> EFI_BOOT_SERVICES_DATA
> EFI_RUNTIME_SERVICES_CODE
> EFI_RUNTIME_SERVICES_DATA
> EFI_ACPI_RECLAIM_MEMORY
> EFI_ACPI_MEMORY_NVS
> 
> The EFI_MEMORY_RUNTIME attribute is irrelevant: the firmware itself
> does not have to read these tables at runtime, so it doesn't matter
> whether the O/S maps them on its behalf.
> 
> If you could please stick that in drivers/firmware/efi/efi.c, and
> rework the patch to use it instead? I will move the ESRT code to it as
> well once this is merged.

Will think about this, I had some plan to change the desc lookup
function but it was delayed for something else. For this bgrt issue since
acpi spec clearly said it is in boot data, can we just check the boot
data areas?

Thanks
Dave

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Ard Biesheuvel]

On 8 June 2017 at 14:20, Dave Young <dyoung@redhat.com> wrote:
> On 06/08/17 at 10:02am, Ard Biesheuvel wrote:
>> On 8 June 2017 at 05:32, Dave Young <dyoung@redhat.com> wrote:
>> > Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
>> > (emulated the panic with using same invalid phys addr in a uefi vm)
>> > There are also a bug in bugzilla.kernel.org:
>> > https://bugzilla.kernel.org/show_bug.cgi?id=195633
>> >
>> > This happens after below commit:
>> > 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
>> >
>> > The root cause is the firmware on those machines provides invalid bgrt
>> > image addresses.
>> >
>> > With original efi bgrt code we initialize bgrt late
>> > and use ioremap to map the image address. In ioremap code we check the
>> > address is a valid physical address or not before really map it.
>> >
>> > With current new efi bgrt code we moved the initialization to early code
>> > so we switch to early_memremap which does not check the phys_addr like
>> > ioremap does. This lead to the early kernel panics.
>> >
>> > Fix this by checking the image physical address, if it is not within
>> > any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
>> > then the original ioremap checking, according to spec the BGRT data
>> > should fall into EFI_BOOT_SERVICES_DATA.
>> >
>>
>> Which spec? The UEFI spec does not mention BGRT, and given that it is
>
> It is mentioned in ACPI spec, see 6.1 spec section 5.2.22.4
> http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
>

I understand that. The reason for asking 'which spec' was your claim
that 'according to spec the BGRT data should fall into
EFI_BOOT_SERVICES_DATA'

>> an ACPI table, I would expect an ACPI reclaim region to be the most
>> appropriate. A quick test with QEMU confirms this:
>>
>> ACPI: BGRT 0x000000013A5E0000 000038 (v01 INTEL  EDK2     00000002
>>  01000013)
>>
>> and
>>
>> efi:   0x00013a5e0000-0x00013a5effff [ACPI Reclaim Memory|   |  |  |
>> |  |  |  |   |WB|  |  |  ]
>>
>> So while I agree that we have to fix this, and that checking the BGRT
>> address against the UEFI memory map is the most appropriate course of
>> action, requiring a certain region type is probably not what we want.
>>
>> We have a similar check for ESRT, in efi_mem_desc_lookup(), which
>> looks a bit dodgy tbh, given that it allows any region type (including
>> MMIO), as long as it has the EFI_MEMORY_RUNTIME attribute, which is
>> almost certainly incorrect.
>
> Yes, I had that in mind but it delayed for something else ..
>
>>
>> So what I would like to see is a function that can tell you whether a
>> certain address is covered by a region of a type that is normal
>> memory, and is occupied, i.e.,
>>
>> EFI_RESERVED_TYPE
>> EFI_LOADER_CODE
>> EFI_LOADER_DATA
>> EFI_BOOT_SERVICES_CODE
>> EFI_BOOT_SERVICES_DATA
>> EFI_RUNTIME_SERVICES_CODE
>> EFI_RUNTIME_SERVICES_DATA
>> EFI_ACPI_RECLAIM_MEMORY
>> EFI_ACPI_MEMORY_NVS
>>
>> The EFI_MEMORY_RUNTIME attribute is irrelevant: the firmware itself
>> does not have to read these tables at runtime, so it doesn't matter
>> whether the O/S maps them on its behalf.
>>
>> If you could please stick that in drivers/firmware/efi/efi.c, and
>> rework the patch to use it instead? I will move the ESRT code to it as
>> well once this is merged.
>
> Will think about this, I had some plan to change the desc lookup
> function but it was delayed for something else. For this bgrt issue since
> acpi spec clearly said it is in boot data, can we just check the boot
> data areas?
>

The BGRT *table* does not reside in EFI_BOOT_SERVICES_DATA, but
usually in ACPI reclaim memory. The BGRT *image* it points to must
reside in EFI_BOOT_SERVICES_DATA according to the ACPI spec, but this
region is disjoint from the ACPI table.

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Ard Biesheuvel]

On 8 June 2017 at 14:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 June 2017 at 14:20, Dave Young <dyoung@redhat.com> wrote:
>> On 06/08/17 at 10:02am, Ard Biesheuvel wrote:
>>> On 8 June 2017 at 05:32, Dave Young <dyoung@redhat.com> wrote:
>>> > Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
>>> > (emulated the panic with using same invalid phys addr in a uefi vm)
>>> > There are also a bug in bugzilla.kernel.org:
>>> > https://bugzilla.kernel.org/show_bug.cgi?id=195633
>>> >
>>> > This happens after below commit:
>>> > 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
>>> >
>>> > The root cause is the firmware on those machines provides invalid bgrt
>>> > image addresses.
>>> >
>>> > With original efi bgrt code we initialize bgrt late
>>> > and use ioremap to map the image address. In ioremap code we check the
>>> > address is a valid physical address or not before really map it.
>>> >
>>> > With current new efi bgrt code we moved the initialization to early code
>>> > so we switch to early_memremap which does not check the phys_addr like
>>> > ioremap does. This lead to the early kernel panics.
>>> >
>>> > Fix this by checking the image physical address, if it is not within
>>> > any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
>>> > then the original ioremap checking, according to spec the BGRT data
>>> > should fall into EFI_BOOT_SERVICES_DATA.
>>> >
>>>
>>> Which spec? The UEFI spec does not mention BGRT, and given that it is
>>
>> It is mentioned in ACPI spec, see 6.1 spec section 5.2.22.4
>> http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
>>
>
> I understand that. The reason for asking 'which spec' was your claim
> that 'according to spec the BGRT data should fall into
> EFI_BOOT_SERVICES_DATA'
>
>>> an ACPI table, I would expect an ACPI reclaim region to be the most
>>> appropriate. A quick test with QEMU confirms this:
>>>
>>> ACPI: BGRT 0x000000013A5E0000 000038 (v01 INTEL  EDK2     00000002
>>>  01000013)
>>>
>>> and
>>>
>>> efi:   0x00013a5e0000-0x00013a5effff [ACPI Reclaim Memory|   |  |  |
>>> |  |  |  |   |WB|  |  |  ]
>>>
>>> So while I agree that we have to fix this, and that checking the BGRT
>>> address against the UEFI memory map is the most appropriate course of
>>> action, requiring a certain region type is probably not what we want.
>>>
>>> We have a similar check for ESRT, in efi_mem_desc_lookup(), which
>>> looks a bit dodgy tbh, given that it allows any region type (including
>>> MMIO), as long as it has the EFI_MEMORY_RUNTIME attribute, which is
>>> almost certainly incorrect.
>>
>> Yes, I had that in mind but it delayed for something else ..
>>
>>>
>>> So what I would like to see is a function that can tell you whether a
>>> certain address is covered by a region of a type that is normal
>>> memory, and is occupied, i.e.,
>>>
>>> EFI_RESERVED_TYPE
>>> EFI_LOADER_CODE
>>> EFI_LOADER_DATA
>>> EFI_BOOT_SERVICES_CODE
>>> EFI_BOOT_SERVICES_DATA
>>> EFI_RUNTIME_SERVICES_CODE
>>> EFI_RUNTIME_SERVICES_DATA
>>> EFI_ACPI_RECLAIM_MEMORY
>>> EFI_ACPI_MEMORY_NVS
>>>
>>> The EFI_MEMORY_RUNTIME attribute is irrelevant: the firmware itself
>>> does not have to read these tables at runtime, so it doesn't matter
>>> whether the O/S maps them on its behalf.
>>>
>>> If you could please stick that in drivers/firmware/efi/efi.c, and
>>> rework the patch to use it instead? I will move the ESRT code to it as
>>> well once this is merged.
>>
>> Will think about this, I had some plan to change the desc lookup
>> function but it was delayed for something else. For this bgrt issue since
>> acpi spec clearly said it is in boot data, can we just check the boot
>> data areas?
>>
>
> The BGRT *table* does not reside in EFI_BOOT_SERVICES_DATA, but
> usually in ACPI reclaim memory. The BGRT *image* it points to must
> reside in EFI_BOOT_SERVICES_DATA according to the ACPI spec, but this
> region is disjoint from the ACPI table.

Oops.

So you are validating the image address not the table address.
Apologies for taking some time to catch up :-)

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Ard Biesheuvel]

 On 8 June 2017 at 05:32, Dave Young <dyoung@redhat.com> wrote:
> Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
> (emulated the panic with using same invalid phys addr in a uefi vm)
> There are also a bug in bugzilla.kernel.org:
> https://bugzilla.kernel.org/show_bug.cgi?id=195633
>
> This happens after below commit:
> 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
>
> The root cause is the firmware on those machines provides invalid bgrt
> image addresses.
>
> With original efi bgrt code we initialize bgrt late
> and use ioremap to map the image address. In ioremap code we check the
> address is a valid physical address or not before really map it.
>
> With current new efi bgrt code we moved the initialization to early code
> so we switch to early_memremap which does not check the phys_addr like
> ioremap does. This lead to the early kernel panics.
>
> Fix this by checking the image physical address, if it is not within
> any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
> then the original ioremap checking, according to spec the BGRT data
> should fall into EFI_BOOT_SERVICES_DATA.
>
> [    0.000000] BUG: unable to handle kernel paging request at ffffffffff280001
> [    0.000000] IP: efi_bgrt_init+0xfb/0x153
> [    0.000000] PGD 6e00b067
> [    0.000000] P4D 6e00b067
> [    0.000000] PUD 6e00d067
> [    0.000000] PMD 6e221067
> [    0.000000] PTE 8a08e01800000163
> [    0.000000]
> [    0.000000] Oops: 0009 [#1] SMP
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.12.0-rc4+ #135
> [    0.000000] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> [    0.000000] task: ffffffff9840f4c0 task.stack: ffffffff98400000
> [    0.000000] RIP: 0010:efi_bgrt_init+0xfb/0x153
> [    0.000000] RSP: 0000:ffffffff98403d50 EFLAGS: 00010082
> [    0.000000] RAX: ffffffffff280001 RBX: 0000000000000000 RCX: 0000000000000006
> [    0.000000] RDX: 0a08e01800001000 RSI: 8a08e01800000163 RDI: 000000000000057e
> [    0.000000] RBP: ffffffff98403d68 R08: 0000000000000041 R09: 0000000000000002
> [    0.000000] R10: 0000000000000000 R11: ffff8c063cff8fc6 R12: ffffffff981d1fb2
> [    0.000000] R13: ffffffff986b4fa0 R14: 0000000000000010 R15: 0000000000000000
> [    0.000000] FS:  0000000000000000(0000) GS:ffffffff984db000(0000) knlGS:0000000000000000
> [    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.000000] CR2: ffffffffff280001 CR3: 000000006e00a000 CR4: 00000000000406b0
> [    0.000000] Call Trace:
> [    0.000000]  ? bgrt_init+0xbc/0xbc
> [    0.000000]  acpi_parse_bgrt+0xe/0x12
> [    0.000000]  acpi_table_parse+0x89/0xb8
> [    0.000000]  acpi_boot_init+0x445/0x4e2
> [    0.000000]  ? acpi_parse_x2apic+0x79/0x79
> [    0.000000]  ? dmi_ignore_irq0_timer_override+0x33/0x33
> [    0.000000]  setup_arch+0xb63/0xc82
> [    0.000000]  ? early_idt_handler_array+0x120/0x120
> [    0.000000]  start_kernel+0xb7/0x443
> [    0.000000]  ? early_idt_handler_array+0x120/0x120
> [    0.000000]  x86_64_start_reservations+0x29/0x2b
> [    0.000000]  x86_64_start_kernel+0x154/0x177
> [    0.000000]  secondary_startup_64+0x9f/0x9f
> [    0.000000] Code: 3f ff eb 6c 48 bf 01 00 00 00 18 e0 08 0a be 06 00 00 00 e8 ef 2b fe ff 48 85 c0 75 0e 48 c7 c7 88 09 22 98 e8 e1 31 3f ff eb 45 <66> 44 8b 20 be 06 00 00 00 48 89 c7 8b 58 02 e8 91 2c fe ff 66
> [    0.000000] RIP: efi_bgrt_init+0xfb/0x153 RSP: ffffffff98403d50
> [    0.000000] CR2: ffffffffff280001
> [    0.000000] ---[ end trace 9843d3b7cbcab26a ]---
> [    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>
> Fixes: 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> Reported-by: Maniaxx <tripleshiftone@gmail.com>
> Signed-off-by: Dave Young <dyoung@redhat.com>

Hi Dave,

I'm with the program now :-)

Could you please check your commit log for grammar? And add that the
spec you refer to is the ACPI spec?

> ---
>  drivers/firmware/efi/efi-bgrt.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> --- linux.orig/drivers/firmware/efi/efi-bgrt.c
> +++ linux/drivers/firmware/efi/efi-bgrt.c
> @@ -27,6 +27,31 @@ struct bmp_header {
>         u32 size;
>  } __packed;
>
> +static bool efi_bgrt_addr_valid(u64 addr)
> +{
> +       efi_memory_desc_t *md;
> +
> +       if (!efi_enabled(EFI_MEMMAP)) {
> +               pr_err("EFI_MEMMAP is not enabled.\n");
> +               return true;
> +       }
> +

Given that you already added a check for EFI_BOOT in efi_bgrt_init()
in commit 5d36982a80248 ("efi/bgrt: Skip efi_bgrt_init in case of
non-efi boot"), could we drop this check, and replace the EFI_BOOT
check you added with EFI_MEMMAP?

> +       for_each_efi_memory_desc(md) {
> +               u64 size;
> +               u64 end;
> +
> +               if (md->type != EFI_BOOT_SERVICES_DATA)
> +                       continue;
> +
> +               size = md->num_pages << EFI_PAGE_SHIFT;
> +               end = md->phys_addr + size;
> +               if (addr >= md->phys_addr && addr < end)
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  void __init efi_bgrt_init(struct acpi_table_header *table)
>  {
>         void *image;
> @@ -65,6 +90,10 @@ void __init efi_bgrt_init(struct acpi_ta
>                 goto out;
>         }
>
> +       if (!efi_bgrt_addr_valid(bgrt->image_address)) {
> +               pr_notice("Ignoring BGRT: invalid image address\n");
> +               goto out;
> +       }
>         image = early_memremap(bgrt->image_address, sizeof(bmp_header));
>         if (!image) {
>                 pr_notice("Ignoring BGRT: failed to map image header memory\n");

Re: [PATCH] x86/efi: fix boot panic because of invalid bgrt image address

[Dave Young]

On 06/08/17 at 03:51pm, Ard Biesheuvel wrote:
>  On 8 June 2017 at 05:32, Dave Young <dyoung@redhat.com> wrote:
> > Maniaxx <tripleshiftone@gmail.com> reported kernel boot panic similar to below:
> > (emulated the panic with using same invalid phys addr in a uefi vm)
> > There are also a bug in bugzilla.kernel.org:
> > https://bugzilla.kernel.org/show_bug.cgi?id=195633
> >
> > This happens after below commit:
> > 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> >
> > The root cause is the firmware on those machines provides invalid bgrt
> > image addresses.
> >
> > With original efi bgrt code we initialize bgrt late
> > and use ioremap to map the image address. In ioremap code we check the
> > address is a valid physical address or not before really map it.
> >
> > With current new efi bgrt code we moved the initialization to early code
> > so we switch to early_memremap which does not check the phys_addr like
> > ioremap does. This lead to the early kernel panics.
> >
> > Fix this by checking the image physical address, if it is not within
> > any EFI_BOOT_SERVICES_DATA areas then we just bail out. It is stronger
> > then the original ioremap checking, according to spec the BGRT data
> > should fall into EFI_BOOT_SERVICES_DATA.
> >
> > [    0.000000] BUG: unable to handle kernel paging request at ffffffffff280001
> > [    0.000000] IP: efi_bgrt_init+0xfb/0x153
> > [    0.000000] PGD 6e00b067
> > [    0.000000] P4D 6e00b067
> > [    0.000000] PUD 6e00d067
> > [    0.000000] PMD 6e221067
> > [    0.000000] PTE 8a08e01800000163
> > [    0.000000]
> > [    0.000000] Oops: 0009 [#1] SMP
> > [    0.000000] Modules linked in:
> > [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.12.0-rc4+ #135
> > [    0.000000] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> > [    0.000000] task: ffffffff9840f4c0 task.stack: ffffffff98400000
> > [    0.000000] RIP: 0010:efi_bgrt_init+0xfb/0x153
> > [    0.000000] RSP: 0000:ffffffff98403d50 EFLAGS: 00010082
> > [    0.000000] RAX: ffffffffff280001 RBX: 0000000000000000 RCX: 0000000000000006
> > [    0.000000] RDX: 0a08e01800001000 RSI: 8a08e01800000163 RDI: 000000000000057e
> > [    0.000000] RBP: ffffffff98403d68 R08: 0000000000000041 R09: 0000000000000002
> > [    0.000000] R10: 0000000000000000 R11: ffff8c063cff8fc6 R12: ffffffff981d1fb2
> > [    0.000000] R13: ffffffff986b4fa0 R14: 0000000000000010 R15: 0000000000000000
> > [    0.000000] FS:  0000000000000000(0000) GS:ffffffff984db000(0000) knlGS:0000000000000000
> > [    0.000000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    0.000000] CR2: ffffffffff280001 CR3: 000000006e00a000 CR4: 00000000000406b0
> > [    0.000000] Call Trace:
> > [    0.000000]  ? bgrt_init+0xbc/0xbc
> > [    0.000000]  acpi_parse_bgrt+0xe/0x12
> > [    0.000000]  acpi_table_parse+0x89/0xb8
> > [    0.000000]  acpi_boot_init+0x445/0x4e2
> > [    0.000000]  ? acpi_parse_x2apic+0x79/0x79
> > [    0.000000]  ? dmi_ignore_irq0_timer_override+0x33/0x33
> > [    0.000000]  setup_arch+0xb63/0xc82
> > [    0.000000]  ? early_idt_handler_array+0x120/0x120
> > [    0.000000]  start_kernel+0xb7/0x443
> > [    0.000000]  ? early_idt_handler_array+0x120/0x120
> > [    0.000000]  x86_64_start_reservations+0x29/0x2b
> > [    0.000000]  x86_64_start_kernel+0x154/0x177
> > [    0.000000]  secondary_startup_64+0x9f/0x9f
> > [    0.000000] Code: 3f ff eb 6c 48 bf 01 00 00 00 18 e0 08 0a be 06 00 00 00 e8 ef 2b fe ff 48 85 c0 75 0e 48 c7 c7 88 09 22 98 e8 e1 31 3f ff eb 45 <66> 44 8b 20 be 06 00 00 00 48 89 c7 8b 58 02 e8 91 2c fe ff 66
> > [    0.000000] RIP: efi_bgrt_init+0xfb/0x153 RSP: ffffffff98403d50
> > [    0.000000] CR2: ffffffffff280001
> > [    0.000000] ---[ end trace 9843d3b7cbcab26a ]---
> > [    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
> > [    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
> >
> > Fixes: 7b0a911 efi/x86: Move the EFI BGRT init code to early init code
> > Reported-by: Maniaxx <tripleshiftone@gmail.com>
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> 
> Hi Dave,
> 
> I'm with the program now :-)
> 
> Could you please check your commit log for grammar? And add that the
> spec you refer to is the ACPI spec?

Will do both. My fault, should say it clearly when I wrote the log..

> 
> > ---
> >  drivers/firmware/efi/efi-bgrt.c |   29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > --- linux.orig/drivers/firmware/efi/efi-bgrt.c
> > +++ linux/drivers/firmware/efi/efi-bgrt.c
> > @@ -27,6 +27,31 @@ struct bmp_header {
> >         u32 size;
> >  } __packed;
> >
> > +static bool efi_bgrt_addr_valid(u64 addr)
> > +{
> > +       efi_memory_desc_t *md;
> > +
> > +       if (!efi_enabled(EFI_MEMMAP)) {
> > +               pr_err("EFI_MEMMAP is not enabled.\n");
> > +               return true;
> > +       }
> > +
> 
> Given that you already added a check for EFI_BOOT in efi_bgrt_init()
> in commit 5d36982a80248 ("efi/bgrt: Skip efi_bgrt_init in case of
> non-efi boot"), could we drop this check, and replace the EFI_BOOT
> check you added with EFI_MEMMAP?

Ok, will do.

> 
> > +       for_each_efi_memory_desc(md) {
> > +               u64 size;
> > +               u64 end;
> > +
> > +               if (md->type != EFI_BOOT_SERVICES_DATA)
> > +                       continue;
> > +
> > +               size = md->num_pages << EFI_PAGE_SHIFT;
> > +               end = md->phys_addr + size;
> > +               if (addr >= md->phys_addr && addr < end)
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> >  void __init efi_bgrt_init(struct acpi_table_header *table)
> >  {
> >         void *image;
> > @@ -65,6 +90,10 @@ void __init efi_bgrt_init(struct acpi_ta
> >                 goto out;
> >         }
> >
> > +       if (!efi_bgrt_addr_valid(bgrt->image_address)) {
> > +               pr_notice("Ignoring BGRT: invalid image address\n");
> > +               goto out;
> > +       }
> >         image = early_memremap(bgrt->image_address, sizeof(bmp_header));
> >         if (!image) {
> >                 pr_notice("Ignoring BGRT: failed to map image header memory\n");

Thanks
Dave