[PATCH 0/2] efi/libstub/x86: two more tweaks for the EFI stub startup code

[PATCH 0/2] efi/libstub/x86: two more tweaks for the EFI stub startup code

[Ard Biesheuvel]

Two final cleanups for the x86 startup code, one that helps the compiler
generate better code, by annotating a helper function with the 'const'
function attribute, and one that fixes the misalignment of the stack in
mixed mode.

There are no known issues regarding entering the 32-bit firmware from the
64-bit kernel with the stack misaligned, and the 32-bit kernel does so all
the time, but it is better to comply with the UEFI spec.

Ard Biesheuvel (2):
  efi/libstub/x86: use const attribute for efi_is_64bit()
  efi/libstub/x86: use mandatory 16-byte stack alignment in mixed mode

 arch/x86/boot/compressed/eboot.c        | 14 +++---
 arch/x86/boot/compressed/efi_thunk_64.S | 46 ++++++--------------
 arch/x86/boot/compressed/head_64.S      |  7 ++-
 arch/x86/include/asm/efi.h              |  2 +-
 4 files changed, 23 insertions(+), 46 deletions(-)

-- 
2.20.1

[PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

[Ard Biesheuvel]

Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit()
function with the 'const' attribute, which permits the compiler to
optimize away any redundant calls. Since we have two different entry
points for 32 and 64 bit firmware in the startup code, this also
simplifies the C code since we'll enter it with the efi_is64 variable
already set.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
 arch/x86/boot/compressed/head_64.S |  7 +++----
 arch/x86/include/asm/efi.h         |  2 +-
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 4afd29eb5b34..ab3a40283db7 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -21,16 +21,18 @@
 #include "eboot.h"
 
 static efi_system_table_t *sys_table;
-static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
+extern const bool efi_is64;
 
 __pure efi_system_table_t *efi_system_table(void)
 {
 	return sys_table;
 }
 
-__pure bool efi_is_64bit(void)
+__attribute_const__ bool efi_is_64bit(void)
 {
-	return efi_is64;
+	if (IS_ENABLED(CONFIG_EFI_MIXED))
+		return efi_is64;
+	return IS_ENABLED(CONFIG_X64_64);
 }
 
 static efi_status_t
@@ -710,8 +712,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
  */
 struct boot_params *efi_main(efi_handle_t handle,
 			     efi_system_table_t *sys_table_arg,
-			     struct boot_params *boot_params,
-			     bool is64)
+			     struct boot_params *boot_params)
 {
 	struct desc_ptr *gdt = NULL;
 	struct setup_header *hdr = &boot_params->hdr;
@@ -721,9 +722,6 @@ struct boot_params *efi_main(efi_handle_t handle,
 
 	sys_table = sys_table_arg;
 
-	if (IS_ENABLED(CONFIG_EFI_MIXED))
-		efi_is64 = is64;
-
 	/* Check if we were booted by the EFI firmware */
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		goto fail;
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 44a6bb6964b5..1f1f6c8139b3 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -211,10 +211,9 @@ SYM_FUNC_START(startup_32)
 	movl	efi32_boot_args(%ebp), %edi
 	cmp	$0, %edi
 	jz	1f
-	leal	handover_entry(%ebp), %eax
+	leal	efi64_stub_entry(%ebp), %eax
 	movl	%esi, %edx
 	movl	efi32_boot_args+4(%ebp), %esi
-	movl	$0x0, %ecx
 1:
 #endif
 	pushl	%eax
@@ -242,6 +241,7 @@ SYM_FUNC_START(efi32_stub_entry)
 	movl	%ecx, efi32_boot_args(%ebp)
 	movl	%edx, efi32_boot_args+4(%ebp)
 	sgdtl	efi32_boot_gdt(%ebp)
+	movb	$0, efi_is64(%ebp)
 
 	/* Disable paging */
 	movl	%cr0, %eax
@@ -452,8 +452,6 @@ SYM_CODE_END(startup_64)
 	.org 0x390
 SYM_FUNC_START(efi64_stub_entry)
 SYM_FUNC_START_ALIAS(efi_stub_entry)
-	movq	$1, %rcx
-SYM_INNER_LABEL(handover_entry, SYM_L_LOCAL)
 	and	$~0xf, %rsp			/* realign the stack */
 	call	efi_main
 	movq	%rax,%rsi
@@ -632,6 +630,7 @@ SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
 #ifdef CONFIG_EFI_MIXED
 SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0)
+SYM_DATA(efi_is64, .byte 1)
 #endif
 
 /*
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 9ce697a621cc..86169a24b0d8 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -223,7 +223,7 @@ efi_status_t efi_set_virtual_address_map(unsigned long memory_map_size,
 
 /* arch specific definitions used by the stub code */
 
-__pure bool efi_is_64bit(void);
+__attribute_const__ bool efi_is_64bit(void);
 
 static inline bool efi_is_native(void)
 {
-- 
2.20.1

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

[Arvind Sankar]

On Wed, Jan 08, 2020 at 08:45:01AM +0100, Ard Biesheuvel wrote:
> Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit()
> function with the 'const' attribute, which permits the compiler to
> optimize away any redundant calls. Since we have two different entry
> points for 32 and 64 bit firmware in the startup code, this also
> simplifies the C code since we'll enter it with the efi_is64 variable
> already set.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
>  arch/x86/boot/compressed/head_64.S |  7 +++----
>  arch/x86/include/asm/efi.h         |  2 +-
>  3 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 4afd29eb5b34..ab3a40283db7 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -21,16 +21,18 @@
>  #include "eboot.h"
>  
>  static efi_system_table_t *sys_table;
> -static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
> +extern const bool efi_is64;
>  

Didn't we need to declare this with hidden visibility? Or use the
#pragma GCC visibility push(hidden)?

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

[Ard Biesheuvel]

On Wed, 8 Jan 2020 at 16:23, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Jan 08, 2020 at 08:45:01AM +0100, Ard Biesheuvel wrote:
> > Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit()
> > function with the 'const' attribute, which permits the compiler to
> > optimize away any redundant calls. Since we have two different entry
> > points for 32 and 64 bit firmware in the startup code, this also
> > simplifies the C code since we'll enter it with the efi_is64 variable
> > already set.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
> >  arch/x86/boot/compressed/head_64.S |  7 +++----
> >  arch/x86/include/asm/efi.h         |  2 +-
> >  3 files changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 4afd29eb5b34..ab3a40283db7 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -21,16 +21,18 @@
> >  #include "eboot.h"
> >
> >  static efi_system_table_t *sys_table;
> > -static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
> > +extern const bool efi_is64;
> >
>
> Didn't we need to declare this with hidden visibility? Or use the
> #pragma GCC visibility push(hidden)?

Yes. So this patch depends on

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

[Ard Biesheuvel]

On Wed, 8 Jan 2020 at 16:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 8 Jan 2020 at 16:23, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Wed, Jan 08, 2020 at 08:45:01AM +0100, Ard Biesheuvel wrote:
> > > Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit()
> > > function with the 'const' attribute, which permits the compiler to
> > > optimize away any redundant calls. Since we have two different entry
> > > points for 32 and 64 bit firmware in the startup code, this also
> > > simplifies the C code since we'll enter it with the efi_is64 variable
> > > already set.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
> > >  arch/x86/boot/compressed/head_64.S |  7 +++----
> > >  arch/x86/include/asm/efi.h         |  2 +-
> > >  3 files changed, 10 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > > index 4afd29eb5b34..ab3a40283db7 100644
> > > --- a/arch/x86/boot/compressed/eboot.c
> > > +++ b/arch/x86/boot/compressed/eboot.c
> > > @@ -21,16 +21,18 @@
> > >  #include "eboot.h"
> > >
> > >  static efi_system_table_t *sys_table;
> > > -static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
> > > +extern const bool efi_is64;
> > >
> >
> > Didn't we need to declare this with hidden visibility? Or use the
> > #pragma GCC visibility push(hidden)?
>
> Yes. So this patch depends on

https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?h=next&id=c71339946177f235aa1f750b2dc556ede3288c23

which is part of the open PR to the TIP maintainers.

Re: [PATCH 1/2] efi/libstub/x86: use const attribute for efi_is_64bit()

[Arvind Sankar]

On Wed, Jan 08, 2020 at 04:25:41PM +0100, Ard Biesheuvel wrote:
> On Wed, 8 Jan 2020 at 16:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Wed, 8 Jan 2020 at 16:23, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Wed, Jan 08, 2020 at 08:45:01AM +0100, Ard Biesheuvel wrote:
> > > > Reshuffle the x86 stub code a bit so that we can tag the efi_is_64bit()
> > > > function with the 'const' attribute, which permits the compiler to
> > > > optimize away any redundant calls. Since we have two different entry
> > > > points for 32 and 64 bit firmware in the startup code, this also
> > > > simplifies the C code since we'll enter it with the efi_is64 variable
> > > > already set.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >  arch/x86/boot/compressed/eboot.c   | 14 ++++++--------
> > > >  arch/x86/boot/compressed/head_64.S |  7 +++----
> > > >  arch/x86/include/asm/efi.h         |  2 +-
> > > >  3 files changed, 10 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > > > index 4afd29eb5b34..ab3a40283db7 100644
> > > > --- a/arch/x86/boot/compressed/eboot.c
> > > > +++ b/arch/x86/boot/compressed/eboot.c
> > > > @@ -21,16 +21,18 @@
> > > >  #include "eboot.h"
> > > >
> > > >  static efi_system_table_t *sys_table;
> > > > -static bool efi_is64 = IS_ENABLED(CONFIG_X86_64);
> > > > +extern const bool efi_is64;
> > > >
> > >
> > > Didn't we need to declare this with hidden visibility? Or use the
> > > #pragma GCC visibility push(hidden)?
> >
> > Yes. So this patch depends on
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?h=next&id=c71339946177f235aa1f750b2dc556ede3288c23
> 
> which is part of the open PR to the TIP maintainers.

Ah sorry.

[PATCH 2/2] efi/libstub/x86: use mandatory 16-byte stack alignment in mixed mode

[Ard Biesheuvel]

Reduce the stack frame of the EFI stub's mixed mode thunk routine by
8 bytes, by moving the GDT and return addresses to EBP and EBX, which
we need to preserve anyway, since their top halves will be cleared by
the call into 32-bit firmware code. Doing so results in the UEFI code
being entered with a 16 byte aligned stack, as mandated by the UEFI
spec, fixing the last occurrence in the 64-bit kernel where we violate
this requirement.

Also, move the saved GDT from a global variable to an unused part of the
stack frame, and touch up some other parts of the code.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/efi_thunk_64.S | 46 ++++++--------------
 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/arch/x86/boot/compressed/efi_thunk_64.S b/arch/x86/boot/compressed/efi_thunk_64.S
index d040ff5458e5..8fb7f6799c52 100644
--- a/arch/x86/boot/compressed/efi_thunk_64.S
+++ b/arch/x86/boot/compressed/efi_thunk_64.S
@@ -27,12 +27,9 @@ SYM_FUNC_START(__efi64_thunk)
 	push	%rbp
 	push	%rbx
 
-	subq	$8, %rsp
-	leaq	1f(%rip), %rax
-	movl	%eax, 4(%rsp)
-	leaq	efi_gdt64(%rip), %rax
-	movl	%eax, (%rsp)
-	movl	%eax, 2(%rax)		/* Fixup the gdt base address */
+	leaq	1f(%rip), %rbp
+	leaq	efi_gdt64(%rip), %rbx
+	movl	%ebx, 2(%rbx)		/* Fixup the gdt base address */
 
 	movl	%ds, %eax
 	push	%rax
@@ -48,12 +45,10 @@ SYM_FUNC_START(__efi64_thunk)
 	movl	%esi, 0x0(%rsp)
 	movl	%edx, 0x4(%rsp)
 	movl	%ecx, 0x8(%rsp)
-	movq	%r8, %rsi
-	movl	%esi, 0xc(%rsp)
-	movq	%r9, %rsi
-	movl	%esi,  0x10(%rsp)
+	movl	%r8d, 0xc(%rsp)
+	movl	%r9d, 0x10(%rsp)
 
-	sgdt	save_gdt(%rip)
+	sgdt	0x14(%rsp)
 
 	/*
 	 * Switch to gdt with 32-bit segments. This is the firmware GDT
@@ -68,11 +63,10 @@ SYM_FUNC_START(__efi64_thunk)
 	pushq	%rax
 	lretq
 
-1:	addq	$32, %rsp
+1:	lgdt	0x14(%rsp)
+	addq	$32, %rsp
 	movq	%rdi, %rax
 
-	lgdt	save_gdt(%rip)
-
 	pop	%rbx
 	movl	%ebx, %ss
 	pop	%rbx
@@ -83,15 +77,9 @@ SYM_FUNC_START(__efi64_thunk)
 	/*
 	 * Convert 32-bit status code into 64-bit.
 	 */
-	test	%rax, %rax
-	jz	1f
-	movl	%eax, %ecx
-	andl	$0x0fffffff, %ecx
-	andl	$0xf0000000, %eax
-	shl	$32, %rax
-	or	%rcx, %rax
-1:
-	addq	$8, %rsp
+	roll	$1, %eax
+	rorq	$1, %rax
+
 	pop	%rbx
 	pop	%rbp
 	ret
@@ -135,9 +123,7 @@ SYM_FUNC_START_LOCAL(efi_enter32)
 	 */
 	cli
 
-	movl	56(%esp), %eax
-	movl	%eax, 2(%eax)
-	lgdtl	(%eax)
+	lgdtl	(%ebx)
 
 	movl	%cr4, %eax
 	btsl	$(X86_CR4_PAE_BIT), %eax
@@ -154,9 +140,8 @@ SYM_FUNC_START_LOCAL(efi_enter32)
 	xorl	%eax, %eax
 	lldt	%ax
 
-	movl	60(%esp), %eax
 	pushl	$__KERNEL_CS
-	pushl	%eax
+	pushl	%ebp
 
 	/* Enable paging */
 	movl	%cr0, %eax
@@ -172,11 +157,6 @@ SYM_DATA_START(efi32_boot_gdt)
 	.quad	0
 SYM_DATA_END(efi32_boot_gdt)
 
-SYM_DATA_START_LOCAL(save_gdt)
-	.word	0
-	.quad	0
-SYM_DATA_END(save_gdt)
-
 SYM_DATA_START(efi_gdt64)
 	.word	efi_gdt64_end - efi_gdt64
 	.long	0			/* Filled out by user */
-- 
2.20.1