[v3.1,2/4] efi/arm: preserve early mapping of UEFI memory map longer for BGRT

Message ID 20180709234229.20181-3-takahiro.akashi@linaro.org
State Superseded
Headers show
Series
  • arm64: kexec,kdump: fix boot failures on acpi-only system
Related show

Commit Message

AKASHI Takahiro July 9, 2018, 11:42 p.m.
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>


The BGRT code validates the contents of the table against the UEFI
memory map, and so it expects it to be mapped when the code runs.

On ARM, this is currently not the case, since we tear down the early
mapping after efi_init() completes, and only create the permanent
mapping in arm_enable_runtime_services(), which executes as an early
initcall, but still leaves a window where the UEFI memory map is not
mapped.

So move the call to efi_memmap_unmap() from efi_init() to
arm_enable_runtime_services().

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

---
 drivers/firmware/efi/arm-init.c    | 1 -
 drivers/firmware/efi/arm-runtime.c | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.0

Comments

James Morse July 10, 2018, 5:57 p.m. | #1
Hi Ard,

On 10/07/18 00:42, AKASHI Takahiro wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> 

> The BGRT code validates the contents of the table against the UEFI

> memory map, and so it expects it to be mapped when the code runs.

> 

> On ARM, this is currently not the case, since we tear down the early

> mapping after efi_init() completes, and only create the permanent

> mapping in arm_enable_runtime_services(), which executes as an early

> initcall, but still leaves a window where the UEFI memory map is not

> mapped.

> 

> So move the call to efi_memmap_unmap() from efi_init() to

> arm_enable_runtime_services().


I don't have a machine that generates a BGRT, but I can see that efi_mem_type()
call in efi_bgrt_init() would cause the same problems we have with kexec and acpi.


> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c

> index b5214c143fee..388a929baf95 100644

> --- a/drivers/firmware/efi/arm-init.c

> +++ b/drivers/firmware/efi/arm-init.c

> @@ -259,7 +259,6 @@ void __init efi_init(void)

>  

>  	reserve_regions();

>  	efi_esrt_init();

> -	efi_memmap_unmap();

>  

>  	memblock_reserve(params.mmap & PAGE_MASK,

>  			 PAGE_ALIGN(params.mmap_size +

> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c

> index 5889cbea60b8..59a8c0ec94d5 100644

> --- a/drivers/firmware/efi/arm-runtime.c

> +++ b/drivers/firmware/efi/arm-runtime.c

> @@ -115,6 +115,8 @@ static int __init arm_enable_runtime_services(void)

>  		return 0;

>  	}

>  

> +	efi_memmap_unmap();


This can get called twice if uefi_init() fails after setting the EFI_BOOT flag,
but this can only happen if the system table signature is wrong, (or we're out
of memory really early).

I think this is harmless as we end up passing NULL to early_memunmap() which
WARN()s and returns as its outside the fixmap range. Its just more noise on
systems with a corrupt efi system table.

Acked-by: James Morse <james.morse@arm.com>



Thanks,

James
Ard Biesheuvel July 10, 2018, 6:39 p.m. | #2
On 10 July 2018 at 19:57, James Morse <james.morse@arm.com> wrote:
> Hi Ard,

>

> On 10/07/18 00:42, AKASHI Takahiro wrote:

>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>

>> The BGRT code validates the contents of the table against the UEFI

>> memory map, and so it expects it to be mapped when the code runs.

>>

>> On ARM, this is currently not the case, since we tear down the early

>> mapping after efi_init() completes, and only create the permanent

>> mapping in arm_enable_runtime_services(), which executes as an early

>> initcall, but still leaves a window where the UEFI memory map is not

>> mapped.

>>

>> So move the call to efi_memmap_unmap() from efi_init() to

>> arm_enable_runtime_services().

>

> I don't have a machine that generates a BGRT, but I can see that efi_mem_type()

> call in efi_bgrt_init() would cause the same problems we have with kexec and acpi.

>


I'm not sure I follow. The BGRT table only contains natively aligned
fields, so the alignment faults should not occur when accessing this
table after kexec. The issue addressed by this patch is that
efi_mem_type() bails when called while EFI_MEMMAP is cleared.

>

>> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c

>> index b5214c143fee..388a929baf95 100644

>> --- a/drivers/firmware/efi/arm-init.c

>> +++ b/drivers/firmware/efi/arm-init.c

>> @@ -259,7 +259,6 @@ void __init efi_init(void)

>>

>>       reserve_regions();

>>       efi_esrt_init();

>> -     efi_memmap_unmap();

>>

>>       memblock_reserve(params.mmap & PAGE_MASK,

>>                        PAGE_ALIGN(params.mmap_size +

>> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c

>> index 5889cbea60b8..59a8c0ec94d5 100644

>> --- a/drivers/firmware/efi/arm-runtime.c

>> +++ b/drivers/firmware/efi/arm-runtime.c

>> @@ -115,6 +115,8 @@ static int __init arm_enable_runtime_services(void)

>>               return 0;

>>       }

>>

>> +     efi_memmap_unmap();

>

> This can get called twice if uefi_init() fails after setting the EFI_BOOT flag,

> but this can only happen if the system table signature is wrong, (or we're out

> of memory really early).

>


I guess we should check the EFI_MEMMAP attribute here as well then.

> I think this is harmless as we end up passing NULL to early_memunmap() which

> WARN()s and returns as its outside the fixmap range. Its just more noise on

> systems with a corrupt efi system table.

>

> Acked-by: James Morse <james.morse@arm.com>

>


Thanks James
Will Deacon July 12, 2018, 1:32 p.m. | #3
On Tue, Jul 10, 2018 at 08:39:16PM +0200, Ard Biesheuvel wrote:
> On 10 July 2018 at 19:57, James Morse <james.morse@arm.com> wrote:

> > Hi Ard,

> >

> > On 10/07/18 00:42, AKASHI Takahiro wrote:

> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >>

> >> The BGRT code validates the contents of the table against the UEFI

> >> memory map, and so it expects it to be mapped when the code runs.

> >>

> >> On ARM, this is currently not the case, since we tear down the early

> >> mapping after efi_init() completes, and only create the permanent

> >> mapping in arm_enable_runtime_services(), which executes as an early

> >> initcall, but still leaves a window where the UEFI memory map is not

> >> mapped.

> >>

> >> So move the call to efi_memmap_unmap() from efi_init() to

> >> arm_enable_runtime_services().

> >

> > I don't have a machine that generates a BGRT, but I can see that efi_mem_type()

> > call in efi_bgrt_init() would cause the same problems we have with kexec and acpi.

> >

> 

> I'm not sure I follow. The BGRT table only contains natively aligned

> fields, so the alignment faults should not occur when accessing this

> table after kexec. The issue addressed by this patch is that

> efi_mem_type() bails when called while EFI_MEMMAP is cleared.

> 

> >

> >> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c

> >> index b5214c143fee..388a929baf95 100644

> >> --- a/drivers/firmware/efi/arm-init.c

> >> +++ b/drivers/firmware/efi/arm-init.c

> >> @@ -259,7 +259,6 @@ void __init efi_init(void)

> >>

> >>       reserve_regions();

> >>       efi_esrt_init();

> >> -     efi_memmap_unmap();

> >>

> >>       memblock_reserve(params.mmap & PAGE_MASK,

> >>                        PAGE_ALIGN(params.mmap_size +

> >> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c

> >> index 5889cbea60b8..59a8c0ec94d5 100644

> >> --- a/drivers/firmware/efi/arm-runtime.c

> >> +++ b/drivers/firmware/efi/arm-runtime.c

> >> @@ -115,6 +115,8 @@ static int __init arm_enable_runtime_services(void)

> >>               return 0;

> >>       }

> >>

> >> +     efi_memmap_unmap();

> >

> > This can get called twice if uefi_init() fails after setting the EFI_BOOT flag,

> > but this can only happen if the system table signature is wrong, (or we're out

> > of memory really early).

> >

> 

> I guess we should check the EFI_MEMMAP attribute here as well then.


Do you plan to spin a new version of this patch?

Will
Ard Biesheuvel July 12, 2018, 2:14 p.m. | #4
On 12 July 2018 at 15:32, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jul 10, 2018 at 08:39:16PM +0200, Ard Biesheuvel wrote:

>> On 10 July 2018 at 19:57, James Morse <james.morse@arm.com> wrote:

>> > Hi Ard,

>> >

>> > On 10/07/18 00:42, AKASHI Takahiro wrote:

>> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> >>

>> >> The BGRT code validates the contents of the table against the UEFI

>> >> memory map, and so it expects it to be mapped when the code runs.

>> >>

>> >> On ARM, this is currently not the case, since we tear down the early

>> >> mapping after efi_init() completes, and only create the permanent

>> >> mapping in arm_enable_runtime_services(), which executes as an early

>> >> initcall, but still leaves a window where the UEFI memory map is not

>> >> mapped.

>> >>

>> >> So move the call to efi_memmap_unmap() from efi_init() to

>> >> arm_enable_runtime_services().

>> >

>> > I don't have a machine that generates a BGRT, but I can see that efi_mem_type()

>> > call in efi_bgrt_init() would cause the same problems we have with kexec and acpi.

>> >

>>

>> I'm not sure I follow. The BGRT table only contains natively aligned

>> fields, so the alignment faults should not occur when accessing this

>> table after kexec. The issue addressed by this patch is that

>> efi_mem_type() bails when called while EFI_MEMMAP is cleared.

>>

>> >

>> >> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c

>> >> index b5214c143fee..388a929baf95 100644

>> >> --- a/drivers/firmware/efi/arm-init.c

>> >> +++ b/drivers/firmware/efi/arm-init.c

>> >> @@ -259,7 +259,6 @@ void __init efi_init(void)

>> >>

>> >>       reserve_regions();

>> >>       efi_esrt_init();

>> >> -     efi_memmap_unmap();

>> >>

>> >>       memblock_reserve(params.mmap & PAGE_MASK,

>> >>                        PAGE_ALIGN(params.mmap_size +

>> >> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c

>> >> index 5889cbea60b8..59a8c0ec94d5 100644

>> >> --- a/drivers/firmware/efi/arm-runtime.c

>> >> +++ b/drivers/firmware/efi/arm-runtime.c

>> >> @@ -115,6 +115,8 @@ static int __init arm_enable_runtime_services(void)

>> >>               return 0;

>> >>       }

>> >>

>> >> +     efi_memmap_unmap();

>> >

>> > This can get called twice if uefi_init() fails after setting the EFI_BOOT flag,

>> > but this can only happen if the system table signature is wrong, (or we're out

>> > of memory really early).

>> >

>>

>> I guess we should check the EFI_MEMMAP attribute here as well then.

>

> Do you plan to spin a new version of this patch?

>


Either that or fold in the hunk below.


--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -110,7 +110,7 @@ static int __init arm_enable_runtime_services(void)
 {
        u64 mapsize;

-       if (!efi_enabled(EFI_BOOT)) {
+       if (!efi_enabled(EFI_BOOT) || !efi_enabled(EFI_MEMMAP)) {
                pr_info("EFI services will not be available.\n");
                return 0;
        }

Patch

diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index b5214c143fee..388a929baf95 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -259,7 +259,6 @@  void __init efi_init(void)
 
 	reserve_regions();
 	efi_esrt_init();
-	efi_memmap_unmap();
 
 	memblock_reserve(params.mmap & PAGE_MASK,
 			 PAGE_ALIGN(params.mmap_size +
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 5889cbea60b8..59a8c0ec94d5 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -115,6 +115,8 @@  static int __init arm_enable_runtime_services(void)
 		return 0;
 	}
 
+	efi_memmap_unmap();
+
 	if (efi_runtime_disabled()) {
 		pr_info("EFI runtime services will be disabled.\n");
 		return 0;