efi: assorted fixes

PATCH In review Ard Biesheuvel 2 participants 7 messages Feb 28, 2020

Subsystems (3)

Arm (32-bit) arm64 EFI

lore

Share & tools

lore

Ard Biesheuvel

author maintainer

2020-02-28 10:02 UTC #0 lore raw

cover efi: assorted fixes

Some fixes for the code that is currentely queued up in efi/next for v5.7

Patch #3 fixes a boot regression for non-EFI boot in linux-next, so that will be sent onwards reasonably quickly.

Ard Biesheuvel (3):
efi/arm: clean EFI stub exit code from cache instead of avoiding it efi/arm64: clean EFI stub exit code from cache instead of avoiding it
efi: mark all EFI runtime services as unsupported on non-EFI boot

 arch/arm/boot/compressed/head.S | 18 ++++++++----------
 arch/arm64/kernel/efi-entry.S   | 18 +++++++++---------
 arch/arm64/kernel/image-vars.h  |  2 +-
 drivers/firmware/efi/efi.c      |  6 +++---
 4 files changed, 21 insertions(+), 23 deletions(-)

Signature · 1 line

2.17.1

Ard Biesheuvel

author maintainer

2020-02-28 10:02 UTC #1 lore raw

PATCH 1/3 efi/arm: clean EFI stub exit code from cache instead of avoiding it

Commit c7225494b ("efi/arm: Work around missing cache maintenance in decompressor handover") modified the handover code written in assembler to work around the missing cache maintenance of the piece of code that is executed after the MMU and caches are turned off. Due to the fact that this sequence incorporates a subroutine call, cleaning that code from the cache is not a matter of simply passing the start and end of the currently running subroutine into cache_clean_flush(), which is why instead, the code jumps across into the cleaned copy of the image.

However, this assumes that this copy is executable, and this means we expect EFI_LOADER_DATA regions to be executable as well, which is not a reasonable assumption to make, even if this is true for most UEFI implementations today.

So change this back, and add a cache_clean_flush() call to cover the remaining code in the subroutine, and any code it may execute in the context of cache_off().

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/boot/compressed/head.S | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

arch/arm/boot/compressed/head.S +8 ~~-10~~

index 36ffbeecd30b..04f77214f050 100644
@@ -1452,13 +1452,11 @@ ENTRY(efi_enter_kernel)
 
 		@ The PE/COFF loader might not have cleaned the code we are
 		@ running beyond the PoU, and so calling cache_off below from
-		@ inside the PE/COFF loader allocated region is unsafe. Let's
-		@ assume our own zImage relocation code did a better job, and
-		@ jump into its version of this routine before proceeding.
-		ldr	r1, .Ljmp
-		sub	r1, r7, r1
-		mov	pc, r1				@ no mode switch
-0:
+		@ inside the PE/COFF loader allocated region is unsafe unless
+		@ we explicitly clean it to the PoC.
+		adr	r0, call_cache_fn		@ region of code we will
+		adr	r1, 0f				@ run with MMU off
+		bl	cache_clean_flush
 		bl	cache_off
 
 		@ Set parameters for booting zImage according to boot protocol

@@ -1467,10 +1465,10 @@ ENTRY(efi_enter_kernel)
 		mov	r0, #0
 		mov	r1, #0xFFFFFFFF
 		mov	r2, r4
-		b	__efi_start
+		add	r7, r7, #(__efi_start - start)
+		mov	pc, r7				@ no mode switch
 ENDPROC(efi_enter_kernel)
-		.align	2
-.Ljmp:		.long	start - 0b
+0:
 #endif
 
 		.align

Signature · 1 line

2.17.1

Ard Biesheuvel

author maintainer

2020-02-28 10:02 UTC #2 lore raw

PATCH 2/3 efi/arm64: clean EFI stub exit code from cache instead of avoiding it

In reply to Ard Biesheuvel

Commit 9f9223778 ("efi/libstub/arm: Make efi_entry() an ordinary PE/COFF entrypoint") modified the handover code written in assembler, and for maintainability, aligned the logic with the logic used in the 32-bit ARM version, which is to avoid cache maintenance on the remaining instructions in the subroutine that will be executed with the MMU and caches off, and instead, branch into the relocated copy of the kernel image.

So change this back, and add a __flush_dcache_area() call to cover the remaining code in the subroutine.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/efi-entry.S  | 18 +++++++++---------
 arch/arm64/kernel/image-vars.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

arch/arm64/kernel/efi-entry.S +9 -9

index 4cfd03c35c49..d5dee064975f 100644
@@ -19,7 +19,8 @@ ENTRY(efi_enter_kernel)
 	 * point stored in x0. Save those values in registers which are
 	 * callee preserved.
 	 */
-	mov	x19, x0			// relocated Image address
+	ldr	w2, =stext_offset
+	add	x19, x0, x2		// relocated Image entrypoint
 	mov	x20, x1			// DTB address
 
 	/*

@@ -29,15 +30,14 @@ ENTRY(efi_enter_kernel)
 	ldr	w1, =kernel_size
 	bl	__flush_dcache_area
 	ic	ialluis
-	dsb	sy
 
 	/*
-	 * Jump across, into the copy of the image that we just cleaned
-	 * to the PoC, so that we can safely disable the MMU and caches.
+	 * Flush the remainder of this routine to the PoC
+	 * so that we can safely disable the MMU and caches.
 	 */
-	ldr	w0, .Ljmp
-	sub	x0, x19, w0, sxtw
-	br	x0
+	adr	x0, 0f
+	ldr	w1, 3f
+	bl	__flush_dcache_area
 0:
 	/* Turn off Dcache and MMU */
 	mrs	x0, CurrentEL

@@ -63,6 +63,6 @@ ENTRY(efi_enter_kernel)
 	mov	x1, xzr
 	mov	x2, xzr
 	mov	x3, xzr
-	b	stext
+	br	x19
 ENDPROC(efi_enter_kernel)
-.Ljmp:	.long	_text - 0b
+3:	.long	. - 0b

arch/arm64/kernel/image-vars.h +1 -1

index 9a7aef0d6f70..28bf98f84adf 100644
@@ -13,6 +13,7 @@
 #ifdef CONFIG_EFI
 
 __efistub_kernel_size		= _edata - _text;
+__efistub_stext_offset		= stext - _text;
 
 
 /*

@@ -43,7 +44,6 @@ __efistub___memset		= __pi_memset;
 #endif
 
 __efistub__text			= _text;
-__efistub_stext			= stext;
 __efistub__end			= _end;
 __efistub__edata		= _edata;
 __efistub_screen_info		= screen_info;

Signature · 1 line

2.17.1

Mark Rutland

reviewer

2020-02-28 11:14 UTC #3 lore raw

Re: [PATCH efi-next 2/3] efi/arm64: clean EFI stub exit code from cache instead of avoiding it

Quoted 36 lines · Ard Biesheuvel

Commit 9f9223778 ("efi/libstub/arm: Make efi_entry() an ordinary PE/COFF entrypoint") modified the handover code written in assembler, and for maintainability, aligned the logic with the logic used in the 32-bit ARM version, which is to avoid cache maintenance on the remaining instructions in the subroutine that will be executed with the MMU and caches off, and instead, branch into the relocated copy of the kernel image.
However, this assumes that this copy is executable, and this means we expect EFI_LOADER_DATA regions to be executable as well, which is not a reasonable assumption to make, even if this is true for most UEFI implementations today.
So change this back, and add a __flush_dcache_area() call to cover the remaining code in the subroutine.
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/efi-entry.S  | 18 +++++++++---------
 arch/arm64/kernel/image-vars.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)
arch/arm64/kernel/efi-entry.S +2 -1
index 4cfd03c35c49..d5dee064975f 100644
@@ -19,7 +19,8 @@ ENTRY(efi_enter_kernel)
 	 * point stored in x0. Save those values in registers which are
 	 * callee preserved.
 	 */
-	mov	x19, x0			// relocated Image address
+	ldr	w2, =stext_offset
+	add	x19, x0, x2		// relocated Image entrypoint
 	mov	x20, x1			// DTB address
 
 	/*

arch/arm64/kernel/efi-entry.S ENTRY(efi_enter_kernel) · line 35

Quoted 9 lines

arch/arm64/kernel/efi-entry.S +1 -3

@@ -29,15 +30,14 @@ ENTRY(efi_enter_kernel)
 	ldr	w1, =kernel_size
 	bl	__flush_dcache_area
 	ic	ialluis
-	dsb	sy
 
 	/*
-	 * Jump across, into the copy of the image that we just cleaned
-	 * to the PoC, so that we can safely disable the MMU and caches.
+	 * Flush the remainder of this routine to the PoC

Minor nit, but could we please say 'Clean' rather than 'Flush' here?

Even better, we now have __clean_dcache_area_poc(), and can use that too.

Thanks,
Mark.

Quoted 43 lines

arch/arm64/kernel/efi-entry.S +6 -5

+	 * so that we can safely disable the MMU and caches.
 	 */
-	ldr	w0, .Ljmp
-	sub	x0, x19, w0, sxtw
-	br	x0
+	adr	x0, 0f
+	ldr	w1, 3f
+	bl	__flush_dcache_area
 0:
 	/* Turn off Dcache and MMU */
 	mrs	x0, CurrentEL

@@ -63,6 +63,6 @@ ENTRY(efi_enter_kernel)
 	mov	x1, xzr
 	mov	x2, xzr
 	mov	x3, xzr
-	b	stext
+	br	x19
 ENDPROC(efi_enter_kernel)
-.Ljmp:	.long	_text - 0b
+3:	.long	. - 0b

arch/arm64/kernel/image-vars.h +1 -1

index 9a7aef0d6f70..28bf98f84adf 100644
@@ -13,6 +13,7 @@
 #ifdef CONFIG_EFI
 
 __efistub_kernel_size		= _edata - _text;
+__efistub_stext_offset		= stext - _text;
 
 
 /*

@@ -43,7 +44,6 @@ __efistub___memset		= __pi_memset;
 #endif
 
 __efistub__text			= _text;
-__efistub_stext			= stext;
 __efistub__end			= _end;
 __efistub__edata		= _edata;
 __efistub_screen_info		= screen_info;

Signature · 1 line

2.17.1

Mark Rutland

reviewer

2020-02-28 11:19 UTC #4 lore raw

Re: [PATCH efi-next 2/3] efi/arm64: clean EFI stub exit code from cache instead of avoiding it

Quoted 49 lines · Mark Rutland

Quoted 36 lines · Ard Biesheuvel
Commit 9f9223778 ("efi/libstub/arm: Make efi_entry() an ordinary PE/COFF entrypoint") modified the handover code written in assembler, and for maintainability, aligned the logic with the logic used in the 32-bit ARM version, which is to avoid cache maintenance on the remaining instructions in the subroutine that will be executed with the MMU and caches off, and instead, branch into the relocated copy of the kernel image.
However, this assumes that this copy is executable, and this means we expect EFI_LOADER_DATA regions to be executable as well, which is not a reasonable assumption to make, even if this is true for most UEFI implementations today.
So change this back, and add a __flush_dcache_area() call to cover the remaining code in the subroutine.
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/efi-entry.S  | 18 +++++++++---------
 arch/arm64/kernel/image-vars.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)
arch/arm64/kernel/efi-entry.S +2 -1
index 4cfd03c35c49..d5dee064975f 100644
@@ -19,7 +19,8 @@ ENTRY(efi_enter_kernel)
 	 * point stored in x0. Save those values in registers which are
 	 * callee preserved.
 	 */
-	mov	x19, x0			// relocated Image address
+	ldr	w2, =stext_offset
+	add	x19, x0, x2		// relocated Image entrypoint
 	mov	x20, x1			// DTB address
 
 	/*
arch/arm64/kernel/efi-entry.S ENTRY(efi_enter_kernel) · line 35
Quoted 9 lines
arch/arm64/kernel/efi-entry.S +1 -3
@@ -29,15 +30,14 @@ ENTRY(efi_enter_kernel)
 	ldr	w1, =kernel_size
 	bl	__flush_dcache_area
 	ic	ialluis
-	dsb	sy
 
 	/*
-	 * Jump across, into the copy of the image that we just cleaned
-	 * to the PoC, so that we can safely disable the MMU and caches.
+	 * Flush the remainder of this routine to the PoC
Minor nit, but could we please say 'Clean' rather than 'Flush' here?

Quoted 2 lines

Even better, we now have __clean_dcache_area_poc(), and can use that too.

... or if that's better as a subsequent cleanup for consistency, that'd also be fine, and needn't block this patch.

Thanks,
Mark.

Ard Biesheuvel

author maintainer

2020-02-28 11:21 UTC #5 lore raw

Re: [PATCH efi-next 2/3] efi/arm64: clean EFI stub exit code from cache instead of avoiding it

Quoted 51 lines · Mark Rutland

Quoted 49 lines · Mark Rutland
Quoted 36 lines · Ard Biesheuvel
Commit 9f9223778 ("efi/libstub/arm: Make efi_entry() an ordinary PE/COFF entrypoint") modified the handover code written in assembler, and for maintainability, aligned the logic with the logic used in the 32-bit ARM version, which is to avoid cache maintenance on the remaining instructions in the subroutine that will be executed with the MMU and caches off, and instead, branch into the relocated copy of the kernel image.
However, this assumes that this copy is executable, and this means we expect EFI_LOADER_DATA regions to be executable as well, which is not a reasonable assumption to make, even if this is true for most UEFI implementations today.
So change this back, and add a __flush_dcache_area() call to cover the remaining code in the subroutine.
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/efi-entry.S  | 18 +++++++++---------
 arch/arm64/kernel/image-vars.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)
arch/arm64/kernel/efi-entry.S +2 -1
index 4cfd03c35c49..d5dee064975f 100644
@@ -19,7 +19,8 @@ ENTRY(efi_enter_kernel)
     * point stored in x0. Save those values in registers which are
     * callee preserved.
     */
-   mov     x19, x0                 // relocated Image address
+   ldr     w2, =stext_offset
+   add     x19, x0, x2             // relocated Image entrypoint
    mov     x20, x1                 // DTB address
 
    /*
arch/arm64/kernel/efi-entry.S ENTRY(efi_enter_kernel) · line 35
Quoted 9 lines
arch/arm64/kernel/efi-entry.S +1 -3
@@ -29,15 +30,14 @@ ENTRY(efi_enter_kernel)
    ldr     w1, =kernel_size
    bl      __flush_dcache_area
    ic      ialluis
-   dsb     sy
 
    /*
-    * Jump across, into the copy of the image that we just cleaned
-    * to the PoC, so that we can safely disable the MMU and caches.
+    * Flush the remainder of this routine to the PoC
Minor nit, but could we please say 'Clean' rather than 'Flush' here?

Quoted 6 lines

Quoted 2 lines
Even better, we now have __clean_dcache_area_poc(), and can use that too.
... or if that's better as a subsequent cleanup for consistency, that'd also be fine, and needn't block this patch.

Thanks Mark. That sounds like a worthwhile improvement to fold in. I'll need to add the __efistub_ alias for it, but I'm touching image-vars.h in this patch anyway.

Ard Biesheuvel

author maintainer

2020-02-28 10:02 UTC #6 lore raw

PATCH 3/3 efi: mark all EFI runtime services as unsupported on non-EFI boot

In reply to Ard Biesheuvel

Recent changes to the way we deal with EFI runtime services that are marked as unsupported by the firmware resulted in a regression for non-EFI boot. The problem is that all EFI runtime services are marked as available by default, and any non-NULL checks on the EFI service function pointers (which will be non-NULL even for runtime services that are unsupported on an EFI boot) were replaced with checks against the mask stored in efi.runtime_supported_mask.

When doing a non-EFI boot, this check against the mask will return a false positive, given the fact that all runtime services are marked as enabled by default. Since we dropped the non-NULL check of the runtime service function pointer in favor of the mask check, we will now unconditionally dereference the function pointer, even if it is NULL, and go boom.

So let's ensure that the mask reflects reality on a non-EFI boot, which is that all EFI runtime services are unsupported.

Reported-by David Hildenbrand <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/firmware/efi/efi.c +3 -3

index 41269a95ff85..d1746a579c99 100644
@@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
 {
 	int error;
 
-	if (!efi_enabled(EFI_BOOT))
-		return 0;
-
 	if (!efi_enabled(EFI_RUNTIME_SERVICES))
 		efi.runtime_supported_mask = 0;
 
+	if (!efi_enabled(EFI_BOOT))
+		return 0;
+
 	if (efi.runtime_supported_mask) {
 		/*
 		 * Since we process only one efi_runtime_service() at a time, an

Signature · 1 line

2.17.1