[PATCH 0/5] Minimize the need to move the kernel in the EFI stub

[PATCH 0/5] Minimize the need to move the kernel in the EFI stub

[Arvind Sankar]

This series adds the ability to use the entire PE image space for
decompression, provides the preferred address to the PE loader via the
header, and finally restricts efi_relocate_kernel to cases where we
really need it rather than whenever we were loaded at something other
than preferred address.

Based on tip:efi/core + the cleanup series just posted [1]
[1] https://lore.kernel.org/linux-efi/20200301230436.2246909-1-nivedita@alum.mit.edu/

Arvind Sankar (5):
  x86/boot/compressed/32: Save the output address instead of
    recalculating it
  efi/x86: Decompress at start of PE image load address
  efi/x86: Add kernel preferred address to PE header
  efi/x86: Remove extra headroom for setup block
  efi/x86: Don't relocate the kernel unless necessary

 arch/x86/boot/compressed/head_32.S      | 42 +++++++++++++++--------
 arch/x86/boot/compressed/head_64.S      | 38 +++++++++++++++++++--
 arch/x86/boot/header.S                  |  6 ++--
 arch/x86/boot/tools/build.c             | 44 ++++++++++++++++++-------
 drivers/firmware/efi/libstub/x86-stub.c | 32 +++++++++++++++---
 5 files changed, 127 insertions(+), 35 deletions(-)

-- 
2.24.1

[PATCH 1/5] x86/boot/compressed/32: Save the output address instead of recalculating it

[Arvind Sankar]

In preparation for being able to decompress starting at a different
address than startup_32, save the calculated output address instead of
recalculating it later.

We now keep track of three addresses:
	%edx: startup_32 as we were loaded by bootloader
	%ebx: new location of compressed kernel
	%ebp: start of decompression buffer

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 46bbe7ab4adf..894182500606 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -75,11 +75,11 @@ SYM_FUNC_START(startup_32)
  */
 	leal	(BP_scratch+4)(%esi), %esp
 	call	1f
-1:	popl	%ebp
-	subl	$1b, %ebp
+1:	popl	%edx
+	subl	$1b, %edx
 
 	/* Load new GDT */
-	leal	gdt(%ebp), %eax
+	leal	gdt(%edx), %eax
 	movl	%eax, 2(%eax)
 	lgdt	(%eax)
 
@@ -92,13 +92,14 @@ SYM_FUNC_START(startup_32)
 	movl	%eax, %ss
 
 /*
- * %ebp contains the address we are loaded at by the boot loader and %ebx
+ * %edx contains the address we are loaded at by the boot loader and %ebx
  * contains the address where we should move the kernel image temporarily
- * for safe in-place decompression.
+ * for safe in-place decompression. %ebp contains the address that the kernel
+ * will be decompressed to.
  */
 
 #ifdef CONFIG_RELOCATABLE
-	movl	%ebp, %ebx
+	movl	%edx, %ebx
 	movl	BP_kernel_alignment(%esi), %eax
 	decl	%eax
 	addl    %eax, %ebx
@@ -110,10 +111,10 @@ SYM_FUNC_START(startup_32)
 	movl	$LOAD_PHYSICAL_ADDR, %ebx
 1:
 
+	movl	%ebx, %ebp	// Save the output address for later
 	/* Target address to relocate to for decompression */
-	movl    BP_init_size(%esi), %eax
-	subl    $_end, %eax
-	addl    %eax, %ebx
+	addl    BP_init_size(%esi), %ebx
+	subl    $_end, %ebx
 
 	/* Set up the stack */
 	leal	boot_stack_end(%ebx), %esp
@@ -127,7 +128,7 @@ SYM_FUNC_START(startup_32)
  * where decompression in place becomes safe.
  */
 	pushl	%esi
-	leal	(_bss-4)(%ebp), %esi
+	leal	(_bss-4)(%edx), %esi
 	leal	(_bss-4)(%ebx), %edi
 	movl	$(_bss - startup_32), %ecx
 	shrl	$2, %ecx
@@ -196,9 +197,7 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
 				/* push arguments for extract_kernel: */
 	pushl	$z_output_len	/* decompressed length, end of relocs */
 
-	leal	_end(%ebx), %eax
-	subl    BP_init_size(%esi), %eax
-	pushl	%eax		/* output address */
+	pushl	%ebp		/* output address */
 
 	pushl	$z_input_len	/* input_len */
 	leal	input_data(%ebx), %eax
-- 
2.24.1

Re: [PATCH 1/5] x86/boot/compressed/32: Save the output address instead of recalculating it

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 00:05, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> In preparation for being able to decompress starting at a different
> address than startup_32, save the calculated output address instead of
> recalculating it later.
>

Could you expand this a bit? Are you talking about *running* the
decompressor code at another offset? Or about the space it uses. I
think I know but I'd like to be sure :-)


> We now keep track of three addresses:
>         %edx: startup_32 as we were loaded by bootloader
>         %ebx: new location of compressed kernel
>         %ebp: start of decompression buffer
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/head_32.S | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
> index 46bbe7ab4adf..894182500606 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -75,11 +75,11 @@ SYM_FUNC_START(startup_32)
>   */
>         leal    (BP_scratch+4)(%esi), %esp
>         call    1f
> -1:     popl    %ebp
> -       subl    $1b, %ebp
> +1:     popl    %edx
> +       subl    $1b, %edx
>
>         /* Load new GDT */
> -       leal    gdt(%ebp), %eax
> +       leal    gdt(%edx), %eax
>         movl    %eax, 2(%eax)
>         lgdt    (%eax)
>
> @@ -92,13 +92,14 @@ SYM_FUNC_START(startup_32)
>         movl    %eax, %ss
>
>  /*
> - * %ebp contains the address we are loaded at by the boot loader and %ebx
> + * %edx contains the address we are loaded at by the boot loader and %ebx
>   * contains the address where we should move the kernel image temporarily
> - * for safe in-place decompression.
> + * for safe in-place decompression. %ebp contains the address that the kernel
> + * will be decompressed to.
>   */
>
>  #ifdef CONFIG_RELOCATABLE
> -       movl    %ebp, %ebx
> +       movl    %edx, %ebx
>         movl    BP_kernel_alignment(%esi), %eax
>         decl    %eax
>         addl    %eax, %ebx
> @@ -110,10 +111,10 @@ SYM_FUNC_START(startup_32)
>         movl    $LOAD_PHYSICAL_ADDR, %ebx
>  1:
>
> +       movl    %ebx, %ebp      // Save the output address for later
>         /* Target address to relocate to for decompression */
> -       movl    BP_init_size(%esi), %eax
> -       subl    $_end, %eax
> -       addl    %eax, %ebx
> +       addl    BP_init_size(%esi), %ebx
> +       subl    $_end, %ebx
>
>         /* Set up the stack */
>         leal    boot_stack_end(%ebx), %esp
> @@ -127,7 +128,7 @@ SYM_FUNC_START(startup_32)
>   * where decompression in place becomes safe.
>   */
>         pushl   %esi
> -       leal    (_bss-4)(%ebp), %esi
> +       leal    (_bss-4)(%edx), %esi
>         leal    (_bss-4)(%ebx), %edi
>         movl    $(_bss - startup_32), %ecx
>         shrl    $2, %ecx
> @@ -196,9 +197,7 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
>                                 /* push arguments for extract_kernel: */
>         pushl   $z_output_len   /* decompressed length, end of relocs */
>
> -       leal    _end(%ebx), %eax
> -       subl    BP_init_size(%esi), %eax
> -       pushl   %eax            /* output address */
> +       pushl   %ebp            /* output address */
>
>         pushl   $z_input_len    /* input_len */
>         leal    input_data(%ebx), %eax
> --
> 2.24.1
>

[PATCH 2/5] efi/x86: Decompress at start of PE image load address

[Arvind Sankar]

When booted via PE loader, define image_offset to hold the offset of
startup_32 from the start of the PE image, and use it as the start of
the decompression buffer.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S      | 17 +++++++++++
 arch/x86/boot/compressed/head_64.S      | 38 +++++++++++++++++++++++--
 drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++--
 3 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 894182500606..98b224f5a025 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -100,6 +100,19 @@ SYM_FUNC_START(startup_32)
 
 #ifdef CONFIG_RELOCATABLE
 	movl	%edx, %ebx
+
+#ifdef CONFIG_EFI_STUB
+/*
+ * If we were loaded via the EFI LoadImage service, startup_32 will be at an
+ * offset to the start of the space allocated for the image. efi_pe_entry will
+ * setup image_offset to tell us where the image actually starts, so that we
+ * can use the full available buffer.
+ *	image_offset = startup_32 - image_base
+ * Otherwise image_offset will be zero and have no effect on the calculations.
+ */
+	subl    image_offset(%edx), %ebx
+#endif
+
 	movl	BP_kernel_alignment(%esi), %eax
 	decl	%eax
 	addl    %eax, %ebx
@@ -226,6 +239,10 @@ SYM_DATA_START_LOCAL(gdt)
 	.quad	0x00cf92000000ffff	/* __KERNEL_DS */
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
+#ifdef CONFIG_EFI_STUB
+SYM_DATA(image_offset, .long 0)
+#endif
+
 /*
  * Stack and heap for uncompression
  */
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 5d8338a693ce..1a4ea8738df0 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -99,6 +99,19 @@ SYM_FUNC_START(startup_32)
 
 #ifdef CONFIG_RELOCATABLE
 	movl	%ebp, %ebx
+
+#ifdef CONFIG_EFI_STUB
+/*
+ * If we were loaded via the EFI LoadImage service, startup_32 will be at an
+ * offset to the start of the space allocated for the image. efi_pe_entry will
+ * setup image_offset to tell us where the image actually starts, so that we
+ * can use the full available buffer.
+ *	image_offset = startup_32 - image_base
+ * Otherwise image_offset will be zero and have no effect on the calculations.
+ */
+	subl    image_offset(%ebp), %ebx
+#endif
+
 	movl	BP_kernel_alignment(%esi), %eax
 	decl	%eax
 	addl	%eax, %ebx
@@ -111,9 +124,8 @@ SYM_FUNC_START(startup_32)
 1:
 
 	/* Target address to relocate to for decompression */
-	movl	BP_init_size(%esi), %eax
-	subl	$_end, %eax
-	addl	%eax, %ebx
+	addl	BP_init_size(%esi), %ebx
+	subl	$_end, %ebx
 
 /*
  * Prepare for entering 64 bit mode
@@ -299,6 +311,20 @@ SYM_CODE_START(startup_64)
 	/* Start with the delta to where the kernel will run at. */
 #ifdef CONFIG_RELOCATABLE
 	leaq	startup_32(%rip) /* - $startup_32 */, %rbp
+
+#ifdef CONFIG_EFI_STUB
+/*
+ * If we were loaded via the EFI LoadImage service, startup_32 will be at an
+ * offset to the start of the space allocated for the image. efi_pe_entry will
+ * setup image_offset to tell us where the image actually starts, so that we
+ * can use the full available buffer.
+ *	image_offset = startup_32 - image_base
+ * Otherwise image_offset will be zero and have no effect on the calculations.
+ */
+	movl    image_offset(%rip), %eax
+	subq	%rax, %rbp
+#endif
+
 	movl	BP_kernel_alignment(%rsi), %eax
 	decl	%eax
 	addq	%rax, %rbp
@@ -647,6 +673,10 @@ SYM_DATA_START_LOCAL(gdt)
 	.quad   0x0000000000000000	/* TS continued */
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
+#ifdef CONFIG_EFI_STUB
+SYM_DATA(image_offset, .long 0)
+#endif
+
 #ifdef CONFIG_EFI_MIXED
 SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0, 0)
 SYM_DATA(efi_is64, .byte 1)
@@ -712,6 +742,8 @@ SYM_FUNC_START(efi32_pe_entry)
 	movl	-4(%ebp), %esi			// loaded_image
 	movl	LI32_image_base(%esi), %esi	// loaded_image->image_base
 	movl	%ebx, %ebp			// startup_32 for efi32_pe_stub_entry
+	subl	%esi, %ebx
+	movl	%ebx, image_offset(%ebp)	// save image_offset
 	jmp	efi32_pe_stub_entry
 
 2:	popl	%edi				// restore callee-save registers
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 7f3e97c2aad3..0c4a6352cfd3 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -19,6 +19,7 @@
 
 static efi_system_table_t *sys_table;
 extern const bool efi_is64;
+extern u32 image_offset;
 
 __pure efi_system_table_t *efi_system_table(void)
 {
@@ -364,6 +365,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	struct boot_params *boot_params;
 	struct setup_header *hdr;
 	efi_loaded_image_t *image;
+	void *image_base;
 	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
 	int options_size = 0;
 	efi_status_t status;
@@ -384,7 +386,10 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 		efi_exit(handle, status);
 	}
 
-	hdr = &((struct boot_params *)efi_table_attr(image, image_base))->hdr;
+	image_base = efi_table_attr(image, image_base);
+	image_offset = (void *)startup_32 - image_base;
+
+	hdr = &((struct boot_params *)image_base)->hdr;
 	above4g = hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G;
 
 	status = efi_allocate_pages(0x4000, (unsigned long *)&boot_params,
@@ -399,7 +404,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	hdr = &boot_params->hdr;
 
 	/* Copy the second sector to boot_params */
-	memcpy(&hdr->jump, efi_table_attr(image, image_base) + 512, 512);
+	memcpy(&hdr->jump, image_base + 512, 512);
 
 	/*
 	 * Fill out some of the header fields ourselves because the
@@ -726,7 +731,7 @@ unsigned long efi_main(efi_handle_t handle,
 	 * If the kernel isn't already loaded at the preferred load
 	 * address, relocate it.
 	 */
-	if (bzimage_addr != hdr->pref_address) {
+	if (bzimage_addr - image_offset != hdr->pref_address) {
 		status = efi_relocate_kernel(&bzimage_addr,
 					     hdr->init_size, hdr->init_size,
 					     hdr->pref_address,
@@ -736,6 +741,7 @@ unsigned long efi_main(efi_handle_t handle,
 			efi_printk("efi_relocate_kernel() failed!\n");
 			goto fail;
 		}
+		image_offset = 0;
 	}
 
 	/*
-- 
2.24.1

Re: [PATCH 2/5] efi/x86: Decompress at start of PE image load address

[Mika Penttilä]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]



On 2.3.2020 1.05, Arvind Sankar wrote:
> When booted via PE loader, define image_offset to hold the offset of
> startup_32 from the start of the PE image, and use it as the start of
> the decompression buffer.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/head_32.S      | 17 +++++++++++
>  arch/x86/boot/compressed/head_64.S      | 38 +++++++++++++++++++++++--
>  drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++--
>  3 files changed, 61 insertions(+), 6 deletions(-)

...
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -19,6 +19,7 @@
>  
>  static efi_system_table_t *sys_table;
>  extern const bool efi_is64;
> +extern u32 image_offset;
>  
>  __pure efi_system_table_t *efi_system_table(void)
>  {
> @@ -364,6 +365,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>  	struct boot_params *boot_params;
>  	struct setup_header *hdr;
>  	efi_loaded_image_t *image;
> +	void *image_base;
>  	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
>  	int options_size = 0;
>  	efi_status_t status;
> @@ -384,7 +386,10 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>  		efi_exit(handle, status);
>  	}
>  
> -	hdr = &((struct boot_params *)efi_table_attr(image, image_base))->hdr;
> +	image_base = efi_table_attr(image, image_base);
> +	image_offset = (void *)startup_32 - image_base;

startup_32 == 0, so maybe something like

leaq	startup_32(%rip) - image_base

should be used ?


> +
> +	hdr = &((struct boot_params *)image_base)->hdr;
>  	above4g = hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G;
>  
>  	status = efi_allocate_pages(0x4000, (unsigned long *)&boot_params,
> @@ -399,7 +404,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>  	hdr = &boot_params->hdr;
>  
>  	/* Copy the second sector to boot_params */
> -	memcpy(&hdr->jump, efi_table_attr(image, image_base) + 512, 512);
> +	memcpy(&hdr->jump, image_base + 512, 512);
>  
>  	/*
>  	 * Fill out some of the header fields ourselves because the
> @@ -726,7 +731,7 @@ unsigned long efi_main(efi_handle_t handle,
>  	 * If the kernel isn't already loaded at the preferred load
>  	 * address, relocate it.
>  	 */
> -	if (bzimage_addr != hdr->pref_address) {
> +	if (bzimage_addr - image_offset != hdr->pref_address) {
>  		status = efi_relocate_kernel(&bzimage_addr,
>  					     hdr->init_size, hdr->init_size,
>  					     hdr->pref_address,
> @@ -736,6 +741,7 @@ unsigned long efi_main(efi_handle_t handle,
>  			efi_printk("efi_relocate_kernel() failed!\n");
>  			goto fail;
>  		}
> +		image_offset = 0;
>  	}
>  
>  	/*


[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 3157 bytes --]

Re: [PATCH 2/5] efi/x86: Decompress at start of PE image load address

[Arvind Sankar]

On Tue, Mar 03, 2020 at 06:28:20AM +0000, Mika Penttilä wrote:
> 
> 
> On 2.3.2020 1.05, Arvind Sankar wrote:
> > When booted via PE loader, define image_offset to hold the offset of
> > startup_32 from the start of the PE image, and use it as the start of
> > the decompression buffer.
> >
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > ---
> >  arch/x86/boot/compressed/head_32.S      | 17 +++++++++++
> >  arch/x86/boot/compressed/head_64.S      | 38 +++++++++++++++++++++++--
> >  drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++--
> >  3 files changed, 61 insertions(+), 6 deletions(-)
> 
> ...
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -19,6 +19,7 @@
> >  
> >  static efi_system_table_t *sys_table;
> >  extern const bool efi_is64;
> > +extern u32 image_offset;
> >  
> >  __pure efi_system_table_t *efi_system_table(void)
> >  {
> > @@ -364,6 +365,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> >  	struct boot_params *boot_params;
> >  	struct setup_header *hdr;
> >  	efi_loaded_image_t *image;
> > +	void *image_base;
> >  	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
> >  	int options_size = 0;
> >  	efi_status_t status;
> > @@ -384,7 +386,10 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> >  		efi_exit(handle, status);
> >  	}
> >  
> > -	hdr = &((struct boot_params *)efi_table_attr(image, image_base))->hdr;
> > +	image_base = efi_table_attr(image, image_base);
> > +	image_offset = (void *)startup_32 - image_base;
> 
> startup_32 == 0, so maybe something like
> 
> leaq	startup_32(%rip) - image_base
> 
> should be used ?
> 

That's what it already uses. All the files in this directory are
compiled to be position-independent, so it uses rip-relative addressing
on 64-bit and GOT-relative addressing on 32-bit.

Re: [PATCH 2/5] efi/x86: Decompress at start of PE image load address

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 00:05, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> When booted via PE loader, define image_offset to hold the offset of
> startup_32 from the start of the PE image, and use it as the start of
> the decompression buffer.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/head_32.S      | 17 +++++++++++
>  arch/x86/boot/compressed/head_64.S      | 38 +++++++++++++++++++++++--
>  drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++--
>  3 files changed, 61 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
> index 894182500606..98b224f5a025 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -100,6 +100,19 @@ SYM_FUNC_START(startup_32)
>
>  #ifdef CONFIG_RELOCATABLE
>         movl    %edx, %ebx
> +
> +#ifdef CONFIG_EFI_STUB
> +/*
> + * If we were loaded via the EFI LoadImage service, startup_32 will be at an
> + * offset to the start of the space allocated for the image. efi_pe_entry will
> + * setup image_offset to tell us where the image actually starts, so that we
> + * can use the full available buffer.
> + *     image_offset = startup_32 - image_base
> + * Otherwise image_offset will be zero and have no effect on the calculations.
> + */
> +       subl    image_offset(%edx), %ebx
> +#endif
> +
>         movl    BP_kernel_alignment(%esi), %eax
>         decl    %eax
>         addl    %eax, %ebx
> @@ -226,6 +239,10 @@ SYM_DATA_START_LOCAL(gdt)
>         .quad   0x00cf92000000ffff      /* __KERNEL_DS */
>  SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
>
> +#ifdef CONFIG_EFI_STUB
> +SYM_DATA(image_offset, .long 0)
> +#endif
> +
>  /*
>   * Stack and heap for uncompression
>   */
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index 5d8338a693ce..1a4ea8738df0 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -99,6 +99,19 @@ SYM_FUNC_START(startup_32)
>
>  #ifdef CONFIG_RELOCATABLE
>         movl    %ebp, %ebx
> +
> +#ifdef CONFIG_EFI_STUB
> +/*
> + * If we were loaded via the EFI LoadImage service, startup_32 will be at an
> + * offset to the start of the space allocated for the image. efi_pe_entry will
> + * setup image_offset to tell us where the image actually starts, so that we
> + * can use the full available buffer.
> + *     image_offset = startup_32 - image_base
> + * Otherwise image_offset will be zero and have no effect on the calculations.
> + */
> +       subl    image_offset(%ebp), %ebx
> +#endif
> +
>         movl    BP_kernel_alignment(%esi), %eax
>         decl    %eax
>         addl    %eax, %ebx
> @@ -111,9 +124,8 @@ SYM_FUNC_START(startup_32)
>  1:
>
>         /* Target address to relocate to for decompression */
> -       movl    BP_init_size(%esi), %eax
> -       subl    $_end, %eax
> -       addl    %eax, %ebx
> +       addl    BP_init_size(%esi), %ebx
> +       subl    $_end, %ebx
>
>  /*
>   * Prepare for entering 64 bit mode
> @@ -299,6 +311,20 @@ SYM_CODE_START(startup_64)
>         /* Start with the delta to where the kernel will run at. */
>  #ifdef CONFIG_RELOCATABLE
>         leaq    startup_32(%rip) /* - $startup_32 */, %rbp
> +
> +#ifdef CONFIG_EFI_STUB
> +/*
> + * If we were loaded via the EFI LoadImage service, startup_32 will be at an
> + * offset to the start of the space allocated for the image. efi_pe_entry will
> + * setup image_offset to tell us where the image actually starts, so that we
> + * can use the full available buffer.
> + *     image_offset = startup_32 - image_base
> + * Otherwise image_offset will be zero and have no effect on the calculations.
> + */
> +       movl    image_offset(%rip), %eax
> +       subq    %rax, %rbp
> +#endif
> +
>         movl    BP_kernel_alignment(%rsi), %eax
>         decl    %eax
>         addq    %rax, %rbp
> @@ -647,6 +673,10 @@ SYM_DATA_START_LOCAL(gdt)
>         .quad   0x0000000000000000      /* TS continued */
>  SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
>
> +#ifdef CONFIG_EFI_STUB
> +SYM_DATA(image_offset, .long 0)
> +#endif
> +
>  #ifdef CONFIG_EFI_MIXED
>  SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0, 0)
>  SYM_DATA(efi_is64, .byte 1)
> @@ -712,6 +742,8 @@ SYM_FUNC_START(efi32_pe_entry)
>         movl    -4(%ebp), %esi                  // loaded_image
>         movl    LI32_image_base(%esi), %esi     // loaded_image->image_base
>         movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry
> +       subl    %esi, %ebx
> +       movl    %ebx, image_offset(%ebp)        // save image_offset

So I guess we are assigning image_offset here because we need it to be
set before we get to efi_pe_entry() ?

I think that deserves a comment.

>         jmp     efi32_pe_stub_entry
>
>  2:     popl    %edi                            // restore callee-save registers
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 7f3e97c2aad3..0c4a6352cfd3 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -19,6 +19,7 @@
>
>  static efi_system_table_t *sys_table;
>  extern const bool efi_is64;
> +extern u32 image_offset;
>
>  __pure efi_system_table_t *efi_system_table(void)
>  {
> @@ -364,6 +365,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>         struct boot_params *boot_params;
>         struct setup_header *hdr;
>         efi_loaded_image_t *image;
> +       void *image_base;
>         efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
>         int options_size = 0;
>         efi_status_t status;
> @@ -384,7 +386,10 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>                 efi_exit(handle, status);
>         }
>
> -       hdr = &((struct boot_params *)efi_table_attr(image, image_base))->hdr;
> +       image_base = efi_table_attr(image, image_base);
> +       image_offset = (void *)startup_32 - image_base;
> +
> +       hdr = &((struct boot_params *)image_base)->hdr;
>         above4g = hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G;
>
>         status = efi_allocate_pages(0x4000, (unsigned long *)&boot_params,
> @@ -399,7 +404,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>         hdr = &boot_params->hdr;
>
>         /* Copy the second sector to boot_params */
> -       memcpy(&hdr->jump, efi_table_attr(image, image_base) + 512, 512);
> +       memcpy(&hdr->jump, image_base + 512, 512);
>
>         /*
>          * Fill out some of the header fields ourselves because the
> @@ -726,7 +731,7 @@ unsigned long efi_main(efi_handle_t handle,
>          * If the kernel isn't already loaded at the preferred load
>          * address, relocate it.
>          */
> -       if (bzimage_addr != hdr->pref_address) {
> +       if (bzimage_addr - image_offset != hdr->pref_address) {
>                 status = efi_relocate_kernel(&bzimage_addr,
>                                              hdr->init_size, hdr->init_size,
>                                              hdr->pref_address,
> @@ -736,6 +741,7 @@ unsigned long efi_main(efi_handle_t handle,
>                         efi_printk("efi_relocate_kernel() failed!\n");
>                         goto fail;
>                 }
> +               image_offset = 0;

Again, this could do with a comment why this should be 0x0 for the
relocated image. It may all seem super obvious now, but our future
selves are probably not as smart as we are today :-)

>         }
>
>         /*
> --
> 2.24.1
>

[PATCH 3/5] efi/x86: Add kernel preferred address to PE header

[Arvind Sankar]

Store the kernel's link address as ImageBase in the PE header. Note that
the PE specification requires the ImageBase to be 64k aligned. The
preferred address should almost always satisfy that, except for 32-bit
kernel if the configuration has been customized.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/header.S | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 4ee25e28996f..0d8d2cb28fd9 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -138,10 +138,12 @@ optional_header:
 #endif
 
 extra_header_fields:
+	# PE specification requires ImageBase to be 64k-aligned
+	.set	ImageBase, (LOAD_PHYSICAL_ADDR+0xffff) & ~0xffff
 #ifdef CONFIG_X86_32
-	.long	0				# ImageBase
+	.long	ImageBase			# ImageBase
 #else
-	.quad	0				# ImageBase
+	.quad	ImageBase			# ImageBase
 #endif
 	.long	0x20				# SectionAlignment
 	.long	0x20				# FileAlignment
-- 
2.24.1

Re: [PATCH 3/5] efi/x86: Add kernel preferred address to PE header

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 00:05, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Store the kernel's link address as ImageBase in the PE header. Note that
> the PE specification requires the ImageBase to be 64k aligned. The
> preferred address should almost always satisfy that, except for 32-bit
> kernel if the configuration has been customized.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/header.S | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 4ee25e28996f..0d8d2cb28fd9 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -138,10 +138,12 @@ optional_header:
>  #endif
>
>  extra_header_fields:
> +       # PE specification requires ImageBase to be 64k-aligned
> +       .set    ImageBase, (LOAD_PHYSICAL_ADDR+0xffff) & ~0xffff

Could you call this image_base please, and put some spaces around the +

>  #ifdef CONFIG_X86_32
> -       .long   0                               # ImageBase
> +       .long   ImageBase                       # ImageBase
>  #else
> -       .quad   0                               # ImageBase
> +       .quad   ImageBase                       # ImageBase
>  #endif
>         .long   0x20                            # SectionAlignment
>         .long   0x20                            # FileAlignment
> --
> 2.24.1
>

[PATCH 4/5] efi/x86: Remove extra headroom for setup block

[Arvind Sankar]

commit 223e3ee56f77 ("efi/x86: add headroom to decompressor BSS to
account for setup block") added headroom to the PE image to account for
the setup block, which wasn't used for the decompression buffer.

Now that we decompress from the start of the image, this is no longer
required.

Add a check to make sure that the head section of the compressed kernel
won't overwrite itself while relocating. This is only for
future-proofing as with current limits on the setup and the actual size
of the head section, this can never happen.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/tools/build.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 90d403dfec80..3d03ad753ed5 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -65,6 +65,8 @@ unsigned long efi_pe_entry;
 unsigned long efi32_pe_entry;
 unsigned long kernel_info;
 unsigned long startup_64;
+unsigned long _ehead;
+unsigned long _end;
 
 /*----------------------------------------------------------------------*/
 
@@ -232,7 +234,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
 {
 	unsigned int pe_header;
 	unsigned int text_sz = file_sz - text_start;
-	unsigned int bss_sz = init_sz + text_start - file_sz;
+	unsigned int bss_sz = init_sz - file_sz;
 
 	pe_header = get_unaligned_le32(&buf[0x3c]);
 
@@ -259,7 +261,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
 	put_unaligned_le32(file_sz - 512 + bss_sz, &buf[pe_header + 0x1c]);
 
 	/* Size of image */
-	put_unaligned_le32(init_sz + text_start, &buf[pe_header + 0x50]);
+	put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);
 
 	/*
 	 * Address of entry point for PE/COFF executable
@@ -360,6 +362,8 @@ static void parse_zoffset(char *fname)
 		PARSE_ZOFS(p, efi32_pe_entry);
 		PARSE_ZOFS(p, kernel_info);
 		PARSE_ZOFS(p, startup_64);
+		PARSE_ZOFS(p, _ehead);
+		PARSE_ZOFS(p, _end);
 
 		p = strchr(p, '\n');
 		while (p && (*p == '\r' || *p == '\n'))
@@ -444,6 +448,26 @@ int main(int argc, char ** argv)
 	put_unaligned_le32(sys_size, &buf[0x1f4]);
 
 	init_sz = get_unaligned_le32(&buf[0x260]);
+#ifdef CONFIG_EFI_STUB
+	/*
+	 * The decompression buffer will start at ImageBase. When relocating
+	 * the compressed kernel to its end, we must ensure that the head
+	 * section does not get overwritten.  The head section occupies
+	 * [i, i + _ehead), and the destination is [init_sz - _end, init_sz).
+	 *
+	 * At present these should never overlap, because i is at most 32k
+	 * because of SETUP_SECT_MAX, _ehead is less than 1k, and the
+	 * calculation of INIT_SIZE in boot/header.S ensures that
+	 * init_sz - _end is at least 64k.
+	 *
+	 * For future-proofing, increase init_sz if necessary.
+	 */
+
+	if (init_sz - _end < i + _ehead) {
+		init_sz = (i + _ehead + _end + 4095) & ~4095;
+		put_unaligned_le32(init_sz, &buf[0x260]);
+	}
+#endif
 	update_pecoff_text(setup_sectors * 512, i + (sys_size * 16), init_sz);
 
 	efi_stub_entry_update();
-- 
2.24.1

Re: [PATCH 4/5] efi/x86: Remove extra headroom for setup block

[Mika Penttilä]

[-- Attachment #1: Type: text/plain, Size: 3587 bytes --]



On 2.3.2020 1.05, Arvind Sankar wrote:
> commit 223e3ee56f77 ("efi/x86: add headroom to decompressor BSS to
> account for setup block") added headroom to the PE image to account for
> the setup block, which wasn't used for the decompression buffer.
>
> Now that we decompress from the start of the image, this is no longer
> required.
>
> Add a check to make sure that the head section of the compressed kernel
> won't overwrite itself while relocating. This is only for
> future-proofing as with current limits on the setup and the actual size
> of the head section, this can never happen.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

To make clear, the kernel (head_32.s and head_64.s) still relocates
itself to the end of the buffer and does in-place decompression. So this
is just to make init sz smaller.


> ---
>  arch/x86/boot/tools/build.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 90d403dfec80..3d03ad753ed5 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -65,6 +65,8 @@ unsigned long efi_pe_entry;
>  unsigned long efi32_pe_entry;
>  unsigned long kernel_info;
>  unsigned long startup_64;
> +unsigned long _ehead;
> +unsigned long _end;
>  
>  /*----------------------------------------------------------------------*/
>  
> @@ -232,7 +234,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
>  {
>  	unsigned int pe_header;
>  	unsigned int text_sz = file_sz - text_start;
> -	unsigned int bss_sz = init_sz + text_start - file_sz;
> +	unsigned int bss_sz = init_sz - file_sz;
>  
>  	pe_header = get_unaligned_le32(&buf[0x3c]);
>  
> @@ -259,7 +261,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
>  	put_unaligned_le32(file_sz - 512 + bss_sz, &buf[pe_header + 0x1c]);
>  
>  	/* Size of image */
> -	put_unaligned_le32(init_sz + text_start, &buf[pe_header + 0x50]);
> +	put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);
>  
>  	/*
>  	 * Address of entry point for PE/COFF executable
> @@ -360,6 +362,8 @@ static void parse_zoffset(char *fname)
>  		PARSE_ZOFS(p, efi32_pe_entry);
>  		PARSE_ZOFS(p, kernel_info);
>  		PARSE_ZOFS(p, startup_64);
> +		PARSE_ZOFS(p, _ehead);
> +		PARSE_ZOFS(p, _end);
>  
>  		p = strchr(p, '\n');
>  		while (p && (*p == '\r' || *p == '\n'))
> @@ -444,6 +448,26 @@ int main(int argc, char ** argv)
>  	put_unaligned_le32(sys_size, &buf[0x1f4]);
>  
>  	init_sz = get_unaligned_le32(&buf[0x260]);
> +#ifdef CONFIG_EFI_STUB
> +	/*
> +	 * The decompression buffer will start at ImageBase. When relocating
> +	 * the compressed kernel to its end, we must ensure that the head
> +	 * section does not get overwritten.  The head section occupies
> +	 * [i, i + _ehead), and the destination is [init_sz - _end, init_sz).
> +	 *
> +	 * At present these should never overlap, because i is at most 32k
> +	 * because of SETUP_SECT_MAX, _ehead is less than 1k, and the
> +	 * calculation of INIT_SIZE in boot/header.S ensures that
> +	 * init_sz - _end is at least 64k.
> +	 *
> +	 * For future-proofing, increase init_sz if necessary.
> +	 */
> +
> +	if (init_sz - _end < i + _ehead) {
> +		init_sz = (i + _ehead + _end + 4095) & ~4095;
> +		put_unaligned_le32(init_sz, &buf[0x260]);
> +	}
> +#endif
>  	update_pecoff_text(setup_sectors * 512, i + (sys_size * 16), init_sz);
>  
>  	efi_stub_entry_update();


[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 3157 bytes --]

Re: [PATCH 4/5] efi/x86: Remove extra headroom for setup block

[Arvind Sankar]

On Mon, Mar 02, 2020 at 04:21:30AM +0000, Mika Penttilä wrote:
> 
> 
> On 2.3.2020 1.05, Arvind Sankar wrote:
> > commit 223e3ee56f77 ("efi/x86: add headroom to decompressor BSS to
> > account for setup block") added headroom to the PE image to account for
> > the setup block, which wasn't used for the decompression buffer.
> >
> > Now that we decompress from the start of the image, this is no longer
> > required.
> >
> > Add a check to make sure that the head section of the compressed kernel
> > won't overwrite itself while relocating. This is only for
> > future-proofing as with current limits on the setup and the actual size
> > of the head section, this can never happen.
> >
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> 
> To make clear, the kernel (head_32.s and head_64.s) still relocates
> itself to the end of the buffer and does in-place decompression. So this
> is just to make init sz smaller.
> 
> 

Not init_size itself, but it reduces the size allocated for the PE
image. Do you want me to update the comment to make that clearer?

[PATCH 5/5] efi/x86: Don't relocate the kernel unless necessary

[Arvind Sankar]

Add alignment slack to the PE image size, so that we can realign the
decompression buffer within the space allocated for the image.

Only relocate the kernel if it has been loaded at an unsuitable address:
* Below LOAD_PHYSICAL_ADDR, or
* Above 64T for 64-bit and 512MiB for 32-bit

For 32-bit, the upper limit is conservative, but the exact limit can be
difficult to calculate.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/tools/build.c             | 16 ++++++----------
 drivers/firmware/efi/libstub/x86-stub.c | 22 +++++++++++++++++++---
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 3d03ad753ed5..db528961c283 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -238,21 +238,17 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
 
 	pe_header = get_unaligned_le32(&buf[0x3c]);
 
-#ifdef CONFIG_EFI_MIXED
 	/*
-	 * In mixed mode, we will execute startup_32() at whichever offset in
-	 * memory it happened to land when the PE/COFF loader loaded the image,
-	 * which may be misaligned with respect to the kernel_alignment field
-	 * in the setup header.
+	 * The PE/COFF loader may load the image at an address which is
+	 * misaligned with respect to the kernel_alignment field in the setup
+	 * header.
 	 *
-	 * In order for startup_32 to safely execute in place at this offset,
-	 * we need to ensure that the CONFIG_PHYSICAL_ALIGN aligned allocation
-	 * it creates for the page tables does not extend beyond the declared
-	 * size of the image in the PE/COFF header. So add the required slack.
+	 * In order to avoid relocating the kernel to correct the misalignment,
+	 * add slack to allow the buffer to be aligned within the declared size
+	 * of the image.
 	 */
 	bss_sz	+= CONFIG_PHYSICAL_ALIGN;
 	init_sz	+= CONFIG_PHYSICAL_ALIGN;
-#endif
 
 	/*
 	 * Size of code: Subtract the size of the first sector (512 bytes)
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 0c4a6352cfd3..957feeacdd8f 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -717,6 +717,7 @@ unsigned long efi_main(efi_handle_t handle,
 			     struct boot_params *boot_params)
 {
 	unsigned long bzimage_addr = (unsigned long)startup_32;
+	unsigned long buffer_start, buffer_end;
 	struct setup_header *hdr = &boot_params->hdr;
 	efi_status_t status;
 	unsigned long cmdline_paddr;
@@ -728,10 +729,25 @@ unsigned long efi_main(efi_handle_t handle,
 		efi_exit(handle, EFI_INVALID_PARAMETER);
 
 	/*
-	 * If the kernel isn't already loaded at the preferred load
-	 * address, relocate it.
+	 * If the kernel isn't already loaded at a suitable address,
+	 * relocate it.
+	 * It must be loaded above LOAD_PHYSICAL_ADDR.
+	 * The maximum address for 64-bit is 1 << 46 for 4-level paging.
+	 * For 32-bit, the maximum address is complicated to figure out, for
+	 * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
+	 * KASLR uses.
+	 * Also relocate it if image_offset is zero, i.e. we weren't loaded by
+	 * LoadImage, but we are not aligned correctly.
 	 */
-	if (bzimage_addr - image_offset != hdr->pref_address) {
+	buffer_start = ALIGN(bzimage_addr - image_offset,
+			     hdr->kernel_alignment);
+	buffer_end = buffer_start + hdr->init_size;
+
+	if (buffer_start < LOAD_PHYSICAL_ADDR
+	    || IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE
+	    || IS_ENABLED(CONFIG_X86_64) && buffer_end > 1ull << 46
+	    || image_offset == 0 && !IS_ALIGNED(bzimage_addr,
+						hdr->kernel_alignment)) {
 		status = efi_relocate_kernel(&bzimage_addr,
 					     hdr->init_size, hdr->init_size,
 					     hdr->pref_address,
-- 
2.24.1

Re: [PATCH 5/5] efi/x86: Don't relocate the kernel unless necessary

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 00:05, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Add alignment slack to the PE image size, so that we can realign the
> decompression buffer within the space allocated for the image.
>
> Only relocate the kernel if it has been loaded at an unsuitable address:
> * Below LOAD_PHYSICAL_ADDR, or
> * Above 64T for 64-bit and 512MiB for 32-bit
>
> For 32-bit, the upper limit is conservative, but the exact limit can be
> difficult to calculate.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/tools/build.c             | 16 ++++++----------
>  drivers/firmware/efi/libstub/x86-stub.c | 22 +++++++++++++++++++---
>  2 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 3d03ad753ed5..db528961c283 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -238,21 +238,17 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
>
>         pe_header = get_unaligned_le32(&buf[0x3c]);
>
> -#ifdef CONFIG_EFI_MIXED
>         /*
> -        * In mixed mode, we will execute startup_32() at whichever offset in
> -        * memory it happened to land when the PE/COFF loader loaded the image,
> -        * which may be misaligned with respect to the kernel_alignment field
> -        * in the setup header.
> +        * The PE/COFF loader may load the image at an address which is
> +        * misaligned with respect to the kernel_alignment field in the setup
> +        * header.
>          *
> -        * In order for startup_32 to safely execute in place at this offset,
> -        * we need to ensure that the CONFIG_PHYSICAL_ALIGN aligned allocation
> -        * it creates for the page tables does not extend beyond the declared
> -        * size of the image in the PE/COFF header. So add the required slack.
> +        * In order to avoid relocating the kernel to correct the misalignment,
> +        * add slack to allow the buffer to be aligned within the declared size
> +        * of the image.
>          */
>         bss_sz  += CONFIG_PHYSICAL_ALIGN;
>         init_sz += CONFIG_PHYSICAL_ALIGN;
> -#endif
>
>         /*
>          * Size of code: Subtract the size of the first sector (512 bytes)
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 0c4a6352cfd3..957feeacdd8f 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -717,6 +717,7 @@ unsigned long efi_main(efi_handle_t handle,
>                              struct boot_params *boot_params)
>  {
>         unsigned long bzimage_addr = (unsigned long)startup_32;
> +       unsigned long buffer_start, buffer_end;
>         struct setup_header *hdr = &boot_params->hdr;
>         efi_status_t status;
>         unsigned long cmdline_paddr;
> @@ -728,10 +729,25 @@ unsigned long efi_main(efi_handle_t handle,
>                 efi_exit(handle, EFI_INVALID_PARAMETER);
>
>         /*
> -        * If the kernel isn't already loaded at the preferred load
> -        * address, relocate it.
> +        * If the kernel isn't already loaded at a suitable address,
> +        * relocate it.
> +        * It must be loaded above LOAD_PHYSICAL_ADDR.
> +        * The maximum address for 64-bit is 1 << 46 for 4-level paging.
> +        * For 32-bit, the maximum address is complicated to figure out, for
> +        * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
> +        * KASLR uses.
> +        * Also relocate it if image_offset is zero, i.e. we weren't loaded by
> +        * LoadImage, but we are not aligned correctly.
>          */
> -       if (bzimage_addr - image_offset != hdr->pref_address) {
> +       buffer_start = ALIGN(bzimage_addr - image_offset,
> +                            hdr->kernel_alignment);
> +       buffer_end = buffer_start + hdr->init_size;
> +
> +       if (buffer_start < LOAD_PHYSICAL_ADDR
> +           || IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE
> +           || IS_ENABLED(CONFIG_X86_64) && buffer_end > 1ull << 46
> +           || image_offset == 0 && !IS_ALIGNED(bzimage_addr,
> +                                               hdr->kernel_alignment)) {

Could you please
- put the || at the end
- put () around the terms
- add a #define for 1ull << 46


>                 status = efi_relocate_kernel(&bzimage_addr,
>                                              hdr->init_size, hdr->init_size,
>                                              hdr->pref_address,
> --
> 2.24.1
>

[PATCH v2 0/5] Minimize the need to move the kernel in the EFI stub

[Arvind Sankar]

This series adds the ability to use the entire PE image space for
decompression, provides the preferred address to the PE loader via the
header, and finally restricts efi_relocate_kernel to cases where we
really need it rather than whenever we were loaded at something other
than preferred address.

Based on tip:efi/core + the cleanup series [1]
[1] https://lore.kernel.org/linux-efi/20200301230436.2246909-1-nivedita@alum.mit.edu/

Changes from v1
- clarify a few comments
- cleanups to code formatting

Arvind Sankar (5):
  x86/boot/compressed/32: Save the output address instead of
    recalculating it
  efi/x86: Decompress at start of PE image load address
  efi/x86: Add kernel preferred address to PE header
  efi/x86: Remove extra headroom for setup block
  efi/x86: Don't relocate the kernel unless necessary

 arch/x86/boot/compressed/head_32.S      | 42 +++++++++++++++-------
 arch/x86/boot/compressed/head_64.S      | 42 ++++++++++++++++++++--
 arch/x86/boot/header.S                  |  6 ++--
 arch/x86/boot/tools/build.c             | 44 ++++++++++++++++-------
 drivers/firmware/efi/libstub/x86-stub.c | 48 ++++++++++++++++++++++---
 5 files changed, 147 insertions(+), 35 deletions(-)

Range-diff against v1:
1:  0cdb6bf27a24 ! 1:  2ecbf60b9ecd x86/boot/compressed/32: Save the output address instead of recalculating it
    @@ Metadata
      ## Commit message ##
         x86/boot/compressed/32: Save the output address instead of recalculating it
     
    -    In preparation for being able to decompress starting at a different
    -    address than startup_32, save the calculated output address instead of
    -    recalculating it later.
    +    In preparation for being able to decompress into a buffer starting at a
    +    different address than startup_32, save the calculated output address
    +    instead of recalculating it later.
     
         We now keep track of three addresses:
                 %edx: startup_32 as we were loaded by bootloader
2:  d4df840752ac ! 2:  e2bdbe6cb692 efi/x86: Decompress at start of PE image load address
    @@ arch/x86/boot/compressed/head_64.S: SYM_FUNC_START(efi32_pe_entry)
      	movl	-4(%ebp), %esi			// loaded_image
      	movl	LI32_image_base(%esi), %esi	// loaded_image->image_base
      	movl	%ebx, %ebp			// startup_32 for efi32_pe_stub_entry
    ++	/*
    ++	 * We need to set the image_offset variable here since startup_32 will
    ++	 * use it before we get to the 64-bit efi_pe_entry in C code.
    ++	 */
     +	subl	%esi, %ebx
     +	movl	%ebx, image_offset(%ebp)	// save image_offset
      	jmp	efi32_pe_stub_entry
    @@ drivers/firmware/efi/libstub/x86-stub.c: unsigned long efi_main(efi_handle_t han
      			efi_printk("efi_relocate_kernel() failed!\n");
      			goto fail;
      		}
    ++		/*
    ++		 * Now that we've copied the kernel elsewhere, we no longer
    ++		 * have a setup block before startup_32, so reset image_offset
    ++		 * to zero in case it was set earlier.
    ++		 */
     +		image_offset = 0;
      	}
      
3:  4bae68f25b90 ! 3:  ea840f78f138 efi/x86: Add kernel preferred address to PE header
    @@ arch/x86/boot/header.S: optional_header:
      
      extra_header_fields:
     +	# PE specification requires ImageBase to be 64k-aligned
    -+	.set	ImageBase, (LOAD_PHYSICAL_ADDR+0xffff) & ~0xffff
    ++	.set	image_base, (LOAD_PHYSICAL_ADDR + 0xffff) & ~0xffff
      #ifdef CONFIG_X86_32
     -	.long	0				# ImageBase
    -+	.long	ImageBase			# ImageBase
    ++	.long	image_base			# ImageBase
      #else
     -	.quad	0				# ImageBase
    -+	.quad	ImageBase			# ImageBase
    ++	.quad	image_base			# ImageBase
      #endif
      	.long	0x20				# SectionAlignment
      	.long	0x20				# FileAlignment
4:  2330a25c6b0f ! 4:  c25a9b507d6d efi/x86: Remove extra headroom for setup block
    @@ Commit message
         account for setup block") added headroom to the PE image to account for
         the setup block, which wasn't used for the decompression buffer.
     
    -    Now that we decompress from the start of the image, this is no longer
    -    required.
    +    Now that the decompression buffer is located at the start of the image,
    +    and includes the setup block, this is no longer required.
     
         Add a check to make sure that the head section of the compressed kernel
         won't overwrite itself while relocating. This is only for
5:  2081f91cbe75 ! 5:  d3dc3af1c7b8 efi/x86: Don't relocate the kernel unless necessary
    @@ arch/x86/boot/tools/build.c: static void update_pecoff_text(unsigned int text_st
      	 * Size of code: Subtract the size of the first sector (512 bytes)
     
      ## drivers/firmware/efi/libstub/x86-stub.c ##
    +@@
    + 
    + #include "efistub.h"
    + 
    ++/* Maximum physical address for 64-bit kernel with 4-level paging */
    ++#define MAXMEM_X86_64_4LEVEL (1ull << 46)
    ++
    + static efi_system_table_t *sys_table;
    + extern const bool efi_is64;
    + extern u32 image_offset;
     @@ drivers/firmware/efi/libstub/x86-stub.c: unsigned long efi_main(efi_handle_t handle,
      			     struct boot_params *boot_params)
      {
    @@ drivers/firmware/efi/libstub/x86-stub.c: unsigned long efi_main(efi_handle_t han
     -	 * address, relocate it.
     +	 * If the kernel isn't already loaded at a suitable address,
     +	 * relocate it.
    ++	 *
     +	 * It must be loaded above LOAD_PHYSICAL_ADDR.
    -+	 * The maximum address for 64-bit is 1 << 46 for 4-level paging.
    ++	 *
    ++	 * The maximum address for 64-bit is 1 << 46 for 4-level paging. This
    ++	 * is defined as the macro MAXMEM, but unfortunately that is not a
    ++	 * compile-time constant if 5-level paging is configured, so we instead
    ++	 * define our own macro for use here.
    ++	 *
     +	 * For 32-bit, the maximum address is complicated to figure out, for
     +	 * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
     +	 * KASLR uses.
    ++	 *
     +	 * Also relocate it if image_offset is zero, i.e. we weren't loaded by
     +	 * LoadImage, but we are not aligned correctly.
      	 */
     -	if (bzimage_addr - image_offset != hdr->pref_address) {
    ++
     +	buffer_start = ALIGN(bzimage_addr - image_offset,
     +			     hdr->kernel_alignment);
     +	buffer_end = buffer_start + hdr->init_size;
     +
    -+	if (buffer_start < LOAD_PHYSICAL_ADDR
    -+	    || IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE
    -+	    || IS_ENABLED(CONFIG_X86_64) && buffer_end > 1ull << 46
    -+	    || image_offset == 0 && !IS_ALIGNED(bzimage_addr,
    -+						hdr->kernel_alignment)) {
    ++	if ((buffer_start < LOAD_PHYSICAL_ADDR)				     ||
    ++	    (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE)    ||
    ++	    (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
    ++	    (image_offset == 0 && !IS_ALIGNED(bzimage_addr,
    ++					      hdr->kernel_alignment))) {
      		status = efi_relocate_kernel(&bzimage_addr,
      					     hdr->init_size, hdr->init_size,
      					     hdr->pref_address,
-- 
2.24.1

[PATCH v2 1/5] x86/boot/compressed/32: Save the output address instead of recalculating it

[Arvind Sankar]

In preparation for being able to decompress into a buffer starting at a
different address than startup_32, save the calculated output address
instead of recalculating it later.

We now keep track of three addresses:
	%edx: startup_32 as we were loaded by bootloader
	%ebx: new location of compressed kernel
	%ebp: start of decompression buffer

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 46bbe7ab4adf..894182500606 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -75,11 +75,11 @@ SYM_FUNC_START(startup_32)
  */
 	leal	(BP_scratch+4)(%esi), %esp
 	call	1f
-1:	popl	%ebp
-	subl	$1b, %ebp
+1:	popl	%edx
+	subl	$1b, %edx
 
 	/* Load new GDT */
-	leal	gdt(%ebp), %eax
+	leal	gdt(%edx), %eax
 	movl	%eax, 2(%eax)
 	lgdt	(%eax)
 
@@ -92,13 +92,14 @@ SYM_FUNC_START(startup_32)
 	movl	%eax, %ss
 
 /*
- * %ebp contains the address we are loaded at by the boot loader and %ebx
+ * %edx contains the address we are loaded at by the boot loader and %ebx
  * contains the address where we should move the kernel image temporarily
- * for safe in-place decompression.
+ * for safe in-place decompression. %ebp contains the address that the kernel
+ * will be decompressed to.
  */
 
 #ifdef CONFIG_RELOCATABLE
-	movl	%ebp, %ebx
+	movl	%edx, %ebx
 	movl	BP_kernel_alignment(%esi), %eax
 	decl	%eax
 	addl    %eax, %ebx
@@ -110,10 +111,10 @@ SYM_FUNC_START(startup_32)
 	movl	$LOAD_PHYSICAL_ADDR, %ebx
 1:
 
+	movl	%ebx, %ebp	// Save the output address for later
 	/* Target address to relocate to for decompression */
-	movl    BP_init_size(%esi), %eax
-	subl    $_end, %eax
-	addl    %eax, %ebx
+	addl    BP_init_size(%esi), %ebx
+	subl    $_end, %ebx
 
 	/* Set up the stack */
 	leal	boot_stack_end(%ebx), %esp
@@ -127,7 +128,7 @@ SYM_FUNC_START(startup_32)
  * where decompression in place becomes safe.
  */
 	pushl	%esi
-	leal	(_bss-4)(%ebp), %esi
+	leal	(_bss-4)(%edx), %esi
 	leal	(_bss-4)(%ebx), %edi
 	movl	$(_bss - startup_32), %ecx
 	shrl	$2, %ecx
@@ -196,9 +197,7 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
 				/* push arguments for extract_kernel: */
 	pushl	$z_output_len	/* decompressed length, end of relocs */
 
-	leal	_end(%ebx), %eax
-	subl    BP_init_size(%esi), %eax
-	pushl	%eax		/* output address */
+	pushl	%ebp		/* output address */
 
 	pushl	$z_input_len	/* input_len */
 	leal	input_data(%ebx), %eax
-- 
2.24.1

[PATCH v2 2/5] efi/x86: Decompress at start of PE image load address

[Arvind Sankar]

When booted via PE loader, define image_offset to hold the offset of
startup_32 from the start of the PE image, and use it as the start of
the decompression buffer.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S      | 17 ++++++++++
 arch/x86/boot/compressed/head_64.S      | 42 +++++++++++++++++++++++--
 drivers/firmware/efi/libstub/x86-stub.c | 17 ++++++++--
 3 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 894182500606..98b224f5a025 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -100,6 +100,19 @@ SYM_FUNC_START(startup_32)
 
 #ifdef CONFIG_RELOCATABLE
 	movl	%edx, %ebx
+
+#ifdef CONFIG_EFI_STUB
+/*
+ * If we were loaded via the EFI LoadImage service, startup_32 will be at an
+ * offset to the start of the space allocated for the image. efi_pe_entry will
+ * setup image_offset to tell us where the image actually starts, so that we
+ * can use the full available buffer.
+ *	image_offset = startup_32 - image_base
+ * Otherwise image_offset will be zero and have no effect on the calculations.
+ */
+	subl    image_offset(%edx), %ebx
+#endif
+
 	movl	BP_kernel_alignment(%esi), %eax
 	decl	%eax
 	addl    %eax, %ebx
@@ -226,6 +239,10 @@ SYM_DATA_START_LOCAL(gdt)
 	.quad	0x00cf92000000ffff	/* __KERNEL_DS */
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
+#ifdef CONFIG_EFI_STUB
+SYM_DATA(image_offset, .long 0)
+#endif
+
 /*
  * Stack and heap for uncompression
  */
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 5d8338a693ce..ae79b50840ad 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -99,6 +99,19 @@ SYM_FUNC_START(startup_32)
 
 #ifdef CONFIG_RELOCATABLE
 	movl	%ebp, %ebx
+
+#ifdef CONFIG_EFI_STUB
+/*
+ * If we were loaded via the EFI LoadImage service, startup_32 will be at an
+ * offset to the start of the space allocated for the image. efi_pe_entry will
+ * setup image_offset to tell us where the image actually starts, so that we
+ * can use the full available buffer.
+ *	image_offset = startup_32 - image_base
+ * Otherwise image_offset will be zero and have no effect on the calculations.
+ */
+	subl    image_offset(%ebp), %ebx
+#endif
+
 	movl	BP_kernel_alignment(%esi), %eax
 	decl	%eax
 	addl	%eax, %ebx
@@ -111,9 +124,8 @@ SYM_FUNC_START(startup_32)
 1:
 
 	/* Target address to relocate to for decompression */
-	movl	BP_init_size(%esi), %eax
-	subl	$_end, %eax
-	addl	%eax, %ebx
+	addl	BP_init_size(%esi), %ebx
+	subl	$_end, %ebx
 
 /*
  * Prepare for entering 64 bit mode
@@ -299,6 +311,20 @@ SYM_CODE_START(startup_64)
 	/* Start with the delta to where the kernel will run at. */
 #ifdef CONFIG_RELOCATABLE
 	leaq	startup_32(%rip) /* - $startup_32 */, %rbp
+
+#ifdef CONFIG_EFI_STUB
+/*
+ * If we were loaded via the EFI LoadImage service, startup_32 will be at an
+ * offset to the start of the space allocated for the image. efi_pe_entry will
+ * setup image_offset to tell us where the image actually starts, so that we
+ * can use the full available buffer.
+ *	image_offset = startup_32 - image_base
+ * Otherwise image_offset will be zero and have no effect on the calculations.
+ */
+	movl    image_offset(%rip), %eax
+	subq	%rax, %rbp
+#endif
+
 	movl	BP_kernel_alignment(%rsi), %eax
 	decl	%eax
 	addq	%rax, %rbp
@@ -647,6 +673,10 @@ SYM_DATA_START_LOCAL(gdt)
 	.quad   0x0000000000000000	/* TS continued */
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
+#ifdef CONFIG_EFI_STUB
+SYM_DATA(image_offset, .long 0)
+#endif
+
 #ifdef CONFIG_EFI_MIXED
 SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0, 0)
 SYM_DATA(efi_is64, .byte 1)
@@ -712,6 +742,12 @@ SYM_FUNC_START(efi32_pe_entry)
 	movl	-4(%ebp), %esi			// loaded_image
 	movl	LI32_image_base(%esi), %esi	// loaded_image->image_base
 	movl	%ebx, %ebp			// startup_32 for efi32_pe_stub_entry
+	/*
+	 * We need to set the image_offset variable here since startup_32 will
+	 * use it before we get to the 64-bit efi_pe_entry in C code.
+	 */
+	subl	%esi, %ebx
+	movl	%ebx, image_offset(%ebp)	// save image_offset
 	jmp	efi32_pe_stub_entry
 
 2:	popl	%edi				// restore callee-save registers
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 7f3e97c2aad3..e71b8421e088 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -19,6 +19,7 @@
 
 static efi_system_table_t *sys_table;
 extern const bool efi_is64;
+extern u32 image_offset;
 
 __pure efi_system_table_t *efi_system_table(void)
 {
@@ -364,6 +365,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	struct boot_params *boot_params;
 	struct setup_header *hdr;
 	efi_loaded_image_t *image;
+	void *image_base;
 	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
 	int options_size = 0;
 	efi_status_t status;
@@ -384,7 +386,10 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 		efi_exit(handle, status);
 	}
 
-	hdr = &((struct boot_params *)efi_table_attr(image, image_base))->hdr;
+	image_base = efi_table_attr(image, image_base);
+	image_offset = (void *)startup_32 - image_base;
+
+	hdr = &((struct boot_params *)image_base)->hdr;
 	above4g = hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G;
 
 	status = efi_allocate_pages(0x4000, (unsigned long *)&boot_params,
@@ -399,7 +404,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	hdr = &boot_params->hdr;
 
 	/* Copy the second sector to boot_params */
-	memcpy(&hdr->jump, efi_table_attr(image, image_base) + 512, 512);
+	memcpy(&hdr->jump, image_base + 512, 512);
 
 	/*
 	 * Fill out some of the header fields ourselves because the
@@ -726,7 +731,7 @@ unsigned long efi_main(efi_handle_t handle,
 	 * If the kernel isn't already loaded at the preferred load
 	 * address, relocate it.
 	 */
-	if (bzimage_addr != hdr->pref_address) {
+	if (bzimage_addr - image_offset != hdr->pref_address) {
 		status = efi_relocate_kernel(&bzimage_addr,
 					     hdr->init_size, hdr->init_size,
 					     hdr->pref_address,
@@ -736,6 +741,12 @@ unsigned long efi_main(efi_handle_t handle,
 			efi_printk("efi_relocate_kernel() failed!\n");
 			goto fail;
 		}
+		/*
+		 * Now that we've copied the kernel elsewhere, we no longer
+		 * have a setup block before startup_32, so reset image_offset
+		 * to zero in case it was set earlier.
+		 */
+		image_offset = 0;
 	}
 
 	/*
-- 
2.24.1

[PATCH v2 3/5] efi/x86: Add kernel preferred address to PE header

[Arvind Sankar]

Store the kernel's link address as ImageBase in the PE header. Note that
the PE specification requires the ImageBase to be 64k aligned. The
preferred address should almost always satisfy that, except for 32-bit
kernel if the configuration has been customized.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/header.S | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 4ee25e28996f..736b3a0c9454 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -138,10 +138,12 @@ optional_header:
 #endif
 
 extra_header_fields:
+	# PE specification requires ImageBase to be 64k-aligned
+	.set	image_base, (LOAD_PHYSICAL_ADDR + 0xffff) & ~0xffff
 #ifdef CONFIG_X86_32
-	.long	0				# ImageBase
+	.long	image_base			# ImageBase
 #else
-	.quad	0				# ImageBase
+	.quad	image_base			# ImageBase
 #endif
 	.long	0x20				# SectionAlignment
 	.long	0x20				# FileAlignment
-- 
2.24.1

[PATCH v2 4/5] efi/x86: Remove extra headroom for setup block

[Arvind Sankar]

commit 223e3ee56f77 ("efi/x86: add headroom to decompressor BSS to
account for setup block") added headroom to the PE image to account for
the setup block, which wasn't used for the decompression buffer.

Now that the decompression buffer is located at the start of the image,
and includes the setup block, this is no longer required.

Add a check to make sure that the head section of the compressed kernel
won't overwrite itself while relocating. This is only for
future-proofing as with current limits on the setup and the actual size
of the head section, this can never happen.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/tools/build.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 90d403dfec80..3d03ad753ed5 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -65,6 +65,8 @@ unsigned long efi_pe_entry;
 unsigned long efi32_pe_entry;
 unsigned long kernel_info;
 unsigned long startup_64;
+unsigned long _ehead;
+unsigned long _end;
 
 /*----------------------------------------------------------------------*/
 
@@ -232,7 +234,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
 {
 	unsigned int pe_header;
 	unsigned int text_sz = file_sz - text_start;
-	unsigned int bss_sz = init_sz + text_start - file_sz;
+	unsigned int bss_sz = init_sz - file_sz;
 
 	pe_header = get_unaligned_le32(&buf[0x3c]);
 
@@ -259,7 +261,7 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
 	put_unaligned_le32(file_sz - 512 + bss_sz, &buf[pe_header + 0x1c]);
 
 	/* Size of image */
-	put_unaligned_le32(init_sz + text_start, &buf[pe_header + 0x50]);
+	put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);
 
 	/*
 	 * Address of entry point for PE/COFF executable
@@ -360,6 +362,8 @@ static void parse_zoffset(char *fname)
 		PARSE_ZOFS(p, efi32_pe_entry);
 		PARSE_ZOFS(p, kernel_info);
 		PARSE_ZOFS(p, startup_64);
+		PARSE_ZOFS(p, _ehead);
+		PARSE_ZOFS(p, _end);
 
 		p = strchr(p, '\n');
 		while (p && (*p == '\r' || *p == '\n'))
@@ -444,6 +448,26 @@ int main(int argc, char ** argv)
 	put_unaligned_le32(sys_size, &buf[0x1f4]);
 
 	init_sz = get_unaligned_le32(&buf[0x260]);
+#ifdef CONFIG_EFI_STUB
+	/*
+	 * The decompression buffer will start at ImageBase. When relocating
+	 * the compressed kernel to its end, we must ensure that the head
+	 * section does not get overwritten.  The head section occupies
+	 * [i, i + _ehead), and the destination is [init_sz - _end, init_sz).
+	 *
+	 * At present these should never overlap, because i is at most 32k
+	 * because of SETUP_SECT_MAX, _ehead is less than 1k, and the
+	 * calculation of INIT_SIZE in boot/header.S ensures that
+	 * init_sz - _end is at least 64k.
+	 *
+	 * For future-proofing, increase init_sz if necessary.
+	 */
+
+	if (init_sz - _end < i + _ehead) {
+		init_sz = (i + _ehead + _end + 4095) & ~4095;
+		put_unaligned_le32(init_sz, &buf[0x260]);
+	}
+#endif
 	update_pecoff_text(setup_sectors * 512, i + (sys_size * 16), init_sz);
 
 	efi_stub_entry_update();
-- 
2.24.1

Re: [PATCH v2 4/5] efi/x86: Remove extra headroom for setup block

[Mike Lothian]

Hi

This patch has been causing issues for me since switching to GCC 10.1:

  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND  objtool
  CHK     include/generated/compile.h
  HOSTCC  arch/x86/boot/tools/build
/usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: linker defined: multiple definition of '_end'
/usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: /tmp/ccEkW0jM.o: previous definition here
collect2: error: ld returned 1 exit status
make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
make: *** [arch/x86/Makefile:303: bzImage] Error 2

Cheers

Mike

Re: [PATCH v2 4/5] efi/x86: Remove extra headroom for setup block

[Arvind Sankar]

On Mon, May 11, 2020 at 06:01:49PM +0100, Mike Lothian wrote:
> Hi
> 
> This patch has been causing issues for me since switching to GCC 10.1:
> 
>   CALL    scripts/checksyscalls.sh
>   CALL    scripts/atomic/check-atomics.sh
>   DESCEND  objtool
>   CHK     include/generated/compile.h
>   HOSTCC  arch/x86/boot/tools/build
> /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: linker defined: multiple definition of '_end'
> /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: /tmp/ccEkW0jM.o: previous definition here
> collect2: error: ld returned 1 exit status
> make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
> make: *** [arch/x86/Makefile:303: bzImage] Error 2
> 
> Cheers
> 
> Mike

I'm not getting an error even with gcc 10 for some reason, but I can see
that it is busted. It's using the linker-defined _end symbol which is
just pass the end of the .bss.

Does adding "static" to the declaration of _end fix your error?

Re: [PATCH v2 4/5] efi/x86: Remove extra headroom for setup block

[Ard Biesheuvel]

On Mon, 11 May 2020 at 20:36, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, May 11, 2020 at 06:01:49PM +0100, Mike Lothian wrote:
> > Hi
> >
> > This patch has been causing issues for me since switching to GCC 10.1:
> >
> >   CALL    scripts/checksyscalls.sh
> >   CALL    scripts/atomic/check-atomics.sh
> >   DESCEND  objtool
> >   CHK     include/generated/compile.h
> >   HOSTCC  arch/x86/boot/tools/build
> > /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: linker defined: multiple definition of '_end'
> > /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: /tmp/ccEkW0jM.o: previous definition here
> > collect2: error: ld returned 1 exit status
> > make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
> > make: *** [arch/x86/Makefile:303: bzImage] Error 2
> >
> > Cheers
> >
> > Mike
>
> I'm not getting an error even with gcc 10 for some reason, but I can see
> that it is busted. It's using the linker-defined _end symbol which is
> just pass the end of the .bss.
>
> Does adding "static" to the declaration of _end fix your error?

This is in a host tool, so it depends on the builtin linker script the
toolchain decides to use. This is risky, though, as it may be using
PROVIDE() for _end, which means that in cases where it doesn't break,
other references to _end that may exist will be linked to the wrong
symbol. I don't think 'build' should be expected to do anything
interesting with its own representation in memory, but better fix it
nonetheless.

Arvind: mind sending a fix for this, please?

Re: [PATCH v2 4/5] efi/x86: Remove extra headroom for setup block

[Arvind Sankar]

On Mon, May 11, 2020 at 11:13:00PM +0200, Ard Biesheuvel wrote:
> On Mon, 11 May 2020 at 20:36, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Mon, May 11, 2020 at 06:01:49PM +0100, Mike Lothian wrote:
> > > Hi
> > >
> > > This patch has been causing issues for me since switching to GCC 10.1:
> > >
> > >   CALL    scripts/checksyscalls.sh
> > >   CALL    scripts/atomic/check-atomics.sh
> > >   DESCEND  objtool
> > >   CHK     include/generated/compile.h
> > >   HOSTCC  arch/x86/boot/tools/build
> > > /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: linker defined: multiple definition of '_end'
> > > /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld: /tmp/ccEkW0jM.o: previous definition here
> > > collect2: error: ld returned 1 exit status
> > > make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
> > > make: *** [arch/x86/Makefile:303: bzImage] Error 2
> > >
> > > Cheers
> > >
> > > Mike
> >
> > I'm not getting an error even with gcc 10 for some reason, but I can see
> > that it is busted. It's using the linker-defined _end symbol which is
> > just pass the end of the .bss.
> >
> > Does adding "static" to the declaration of _end fix your error?
> 
> This is in a host tool, so it depends on the builtin linker script the
> toolchain decides to use. This is risky, though, as it may be using
> PROVIDE() for _end, which means that in cases where it doesn't break,
> other references to _end that may exist will be linked to the wrong
> symbol. I don't think 'build' should be expected to do anything
> interesting with its own representation in memory, but better fix it
> nonetheless.

Right, _end _is_ getting redefined in my system linker script too: I can
see with objdump that the final _end symbol in my version of build is
actually pointing beyond the .bss. But my toolchain doesn't report an
error for some reason.

> 
> Arvind: mind sending a fix for this, please?

Yeah, I have one ready -- was just waiting to hear back if "static" did
fix it, but I can send it out now.

[PATCH] x86/boot: Mark global variables as static

[Arvind Sankar]

Mike Lothian reports that after commit
  964124a97b97 ("efi/x86: Remove extra headroom for setup block")
gcc 10.1.0 fails with

  HOSTCC  arch/x86/boot/tools/build
  /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
  error: linker defined: multiple definition of '_end'
  /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
  /tmp/ccEkW0jM.o: previous definition here
  collect2: error: ld returned 1 exit status
  make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
  make: *** [arch/x86/Makefile:303: bzImage] Error 2

The issue is with the _end variable that was added, to hold the end of
the compressed kernel from zoffsets.h (ZO__end). The name clashes with
the linker-defined _end symbol that indicates the end of the build
program itself.

Even when there is no compile-time error, this causes build to use
memory past the end of its .bss section.

To solve this, mark _end as static, and for symmetry, mark the rest of
the variables that keep track of symbols from the compressed kernel as
static as well.

Fixes: 964124a97b97 ("efi/x86: Remove extra headroom for setup block")
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/tools/build.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 8f8c8e386cea..c8b8c1a8d1fc 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -59,14 +59,14 @@ u8 buf[SETUP_SECT_MAX*512];
 #define PECOFF_COMPAT_RESERVE 0x0
 #endif
 
-unsigned long efi32_stub_entry;
-unsigned long efi64_stub_entry;
-unsigned long efi_pe_entry;
-unsigned long efi32_pe_entry;
-unsigned long kernel_info;
-unsigned long startup_64;
-unsigned long _ehead;
-unsigned long _end;
+static unsigned long efi32_stub_entry;
+static unsigned long efi64_stub_entry;
+static unsigned long efi_pe_entry;
+static unsigned long efi32_pe_entry;
+static unsigned long kernel_info;
+static unsigned long startup_64;
+static unsigned long _ehead;
+static unsigned long _end;
 
 /*----------------------------------------------------------------------*/
 
-- 
2.26.2

Re: [PATCH] x86/boot: Mark global variables as static

[Mike Lothian]

Feel free to add my tested by


On Mon, 11 May 2020 at 23:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Mike Lothian reports that after commit
>   964124a97b97 ("efi/x86: Remove extra headroom for setup block")
> gcc 10.1.0 fails with
>
>   HOSTCC  arch/x86/boot/tools/build
>   /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
>   error: linker defined: multiple definition of '_end'
>   /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
>   /tmp/ccEkW0jM.o: previous definition here
>   collect2: error: ld returned 1 exit status
>   make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
>   make: *** [arch/x86/Makefile:303: bzImage] Error 2
>
> The issue is with the _end variable that was added, to hold the end of
> the compressed kernel from zoffsets.h (ZO__end). The name clashes with
> the linker-defined _end symbol that indicates the end of the build
> program itself.
>
> Even when there is no compile-time error, this causes build to use
> memory past the end of its .bss section.
>
> To solve this, mark _end as static, and for symmetry, mark the rest of
> the variables that keep track of symbols from the compressed kernel as
> static as well.
>
> Fixes: 964124a97b97 ("efi/x86: Remove extra headroom for setup block")
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/tools/build.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 8f8c8e386cea..c8b8c1a8d1fc 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -59,14 +59,14 @@ u8 buf[SETUP_SECT_MAX*512];
>  #define PECOFF_COMPAT_RESERVE 0x0
>  #endif
>
> -unsigned long efi32_stub_entry;
> -unsigned long efi64_stub_entry;
> -unsigned long efi_pe_entry;
> -unsigned long efi32_pe_entry;
> -unsigned long kernel_info;
> -unsigned long startup_64;
> -unsigned long _ehead;
> -unsigned long _end;
> +static unsigned long efi32_stub_entry;
> +static unsigned long efi64_stub_entry;
> +static unsigned long efi_pe_entry;
> +static unsigned long efi32_pe_entry;
> +static unsigned long kernel_info;
> +static unsigned long startup_64;
> +static unsigned long _ehead;
> +static unsigned long _end;
>
>  /*----------------------------------------------------------------------*/
>
> --
> 2.26.2
>

Re: [PATCH] x86/boot: Mark global variables as static

[Ard Biesheuvel]

On Tue, 12 May 2020 at 01:12, Mike Lothian <mike@fireburn.co.uk> wrote:
>
> Feel free to add my tested by
>
>
> On Mon, 11 May 2020 at 23:58, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > Mike Lothian reports that after commit
> >   964124a97b97 ("efi/x86: Remove extra headroom for setup block")
> > gcc 10.1.0 fails with
> >
> >   HOSTCC  arch/x86/boot/tools/build
> >   /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
> >   error: linker defined: multiple definition of '_end'
> >   /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
> >   /tmp/ccEkW0jM.o: previous definition here
> >   collect2: error: ld returned 1 exit status
> >   make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
> >   make: *** [arch/x86/Makefile:303: bzImage] Error 2
> >
> > The issue is with the _end variable that was added, to hold the end of
> > the compressed kernel from zoffsets.h (ZO__end). The name clashes with
> > the linker-defined _end symbol that indicates the end of the build
> > program itself.
> >
> > Even when there is no compile-time error, this causes build to use
> > memory past the end of its .bss section.
> >
> > To solve this, mark _end as static, and for symmetry, mark the rest of
> > the variables that keep track of symbols from the compressed kernel as
> > static as well.
> >
> > Fixes: 964124a97b97 ("efi/x86: Remove extra headroom for setup block")
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Thanks, I'll queue this as a fix.


> > ---
> >  arch/x86/boot/tools/build.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> > index 8f8c8e386cea..c8b8c1a8d1fc 100644
> > --- a/arch/x86/boot/tools/build.c
> > +++ b/arch/x86/boot/tools/build.c
> > @@ -59,14 +59,14 @@ u8 buf[SETUP_SECT_MAX*512];
> >  #define PECOFF_COMPAT_RESERVE 0x0
> >  #endif
> >
> > -unsigned long efi32_stub_entry;
> > -unsigned long efi64_stub_entry;
> > -unsigned long efi_pe_entry;
> > -unsigned long efi32_pe_entry;
> > -unsigned long kernel_info;
> > -unsigned long startup_64;
> > -unsigned long _ehead;
> > -unsigned long _end;
> > +static unsigned long efi32_stub_entry;
> > +static unsigned long efi64_stub_entry;
> > +static unsigned long efi_pe_entry;
> > +static unsigned long efi32_pe_entry;
> > +static unsigned long kernel_info;
> > +static unsigned long startup_64;
> > +static unsigned long _ehead;
> > +static unsigned long _end;
> >
> >  /*----------------------------------------------------------------------*/
> >
> > --
> > 2.26.2
> >

[tip: efi/urgent] x86/boot: Mark global variables as static

[tip-bot2 for Arvind Sankar]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     e78d334a5470ead861590ec83158f3b17bd6c807
Gitweb:        https://git.kernel.org/tip/e78d334a5470ead861590ec83158f3b17bd6c807
Author:        Arvind Sankar <nivedita@alum.mit.edu>
AuthorDate:    Mon, 11 May 2020 18:58:49 -04:00
Committer:     Ard Biesheuvel <ardb@kernel.org>
CommitterDate: Thu, 14 May 2020 11:11:20 +02:00

x86/boot: Mark global variables as static

Mike Lothian reports that after commit
  964124a97b97 ("efi/x86: Remove extra headroom for setup block")
gcc 10.1.0 fails with

  HOSTCC  arch/x86/boot/tools/build
  /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
  error: linker defined: multiple definition of '_end'
  /usr/lib/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../x86_64-pc-linux-gnu/bin/ld:
  /tmp/ccEkW0jM.o: previous definition here
  collect2: error: ld returned 1 exit status
  make[1]: *** [scripts/Makefile.host:103: arch/x86/boot/tools/build] Error 1
  make: *** [arch/x86/Makefile:303: bzImage] Error 2

The issue is with the _end variable that was added, to hold the end of
the compressed kernel from zoffsets.h (ZO__end). The name clashes with
the linker-defined _end symbol that indicates the end of the build
program itself.

Even when there is no compile-time error, this causes build to use
memory past the end of its .bss section.

To solve this, mark _end as static, and for symmetry, mark the rest of
the variables that keep track of symbols from the compressed kernel as
static as well.

Fixes: 964124a97b97 ("efi/x86: Remove extra headroom for setup block")
Reported-by: Mike Lothian <mike@fireburn.co.uk>
Tested-by: Mike Lothian <mike@fireburn.co.uk>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Link: https://lore.kernel.org/r/20200511225849.1311869-1-nivedita@alum.mit.edu
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/tools/build.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 8f8c8e3..c8b8c1a 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -59,14 +59,14 @@ u8 buf[SETUP_SECT_MAX*512];
 #define PECOFF_COMPAT_RESERVE 0x0
 #endif
 
-unsigned long efi32_stub_entry;
-unsigned long efi64_stub_entry;
-unsigned long efi_pe_entry;
-unsigned long efi32_pe_entry;
-unsigned long kernel_info;
-unsigned long startup_64;
-unsigned long _ehead;
-unsigned long _end;
+static unsigned long efi32_stub_entry;
+static unsigned long efi64_stub_entry;
+static unsigned long efi_pe_entry;
+static unsigned long efi32_pe_entry;
+static unsigned long kernel_info;
+static unsigned long startup_64;
+static unsigned long _ehead;
+static unsigned long _end;
 
 /*----------------------------------------------------------------------*/
 

[PATCH v2 5/5] efi/x86: Don't relocate the kernel unless necessary

[Arvind Sankar]

Add alignment slack to the PE image size, so that we can realign the
decompression buffer within the space allocated for the image.

Only relocate the kernel if it has been loaded at an unsuitable address:
* Below LOAD_PHYSICAL_ADDR, or
* Above 64T for 64-bit and 512MiB for 32-bit

For 32-bit, the upper limit is conservative, but the exact limit can be
difficult to calculate.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/tools/build.c             | 16 +++++-------
 drivers/firmware/efi/libstub/x86-stub.c | 33 ++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 3d03ad753ed5..db528961c283 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -238,21 +238,17 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
 
 	pe_header = get_unaligned_le32(&buf[0x3c]);
 
-#ifdef CONFIG_EFI_MIXED
 	/*
-	 * In mixed mode, we will execute startup_32() at whichever offset in
-	 * memory it happened to land when the PE/COFF loader loaded the image,
-	 * which may be misaligned with respect to the kernel_alignment field
-	 * in the setup header.
+	 * The PE/COFF loader may load the image at an address which is
+	 * misaligned with respect to the kernel_alignment field in the setup
+	 * header.
 	 *
-	 * In order for startup_32 to safely execute in place at this offset,
-	 * we need to ensure that the CONFIG_PHYSICAL_ALIGN aligned allocation
-	 * it creates for the page tables does not extend beyond the declared
-	 * size of the image in the PE/COFF header. So add the required slack.
+	 * In order to avoid relocating the kernel to correct the misalignment,
+	 * add slack to allow the buffer to be aligned within the declared size
+	 * of the image.
 	 */
 	bss_sz	+= CONFIG_PHYSICAL_ALIGN;
 	init_sz	+= CONFIG_PHYSICAL_ALIGN;
-#endif
 
 	/*
 	 * Size of code: Subtract the size of the first sector (512 bytes)
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index e71b8421e088..fbc4354f534c 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -17,6 +17,9 @@
 
 #include "efistub.h"
 
+/* Maximum physical address for 64-bit kernel with 4-level paging */
+#define MAXMEM_X86_64_4LEVEL (1ull << 46)
+
 static efi_system_table_t *sys_table;
 extern const bool efi_is64;
 extern u32 image_offset;
@@ -717,6 +720,7 @@ unsigned long efi_main(efi_handle_t handle,
 			     struct boot_params *boot_params)
 {
 	unsigned long bzimage_addr = (unsigned long)startup_32;
+	unsigned long buffer_start, buffer_end;
 	struct setup_header *hdr = &boot_params->hdr;
 	efi_status_t status;
 	unsigned long cmdline_paddr;
@@ -728,10 +732,33 @@ unsigned long efi_main(efi_handle_t handle,
 		efi_exit(handle, EFI_INVALID_PARAMETER);
 
 	/*
-	 * If the kernel isn't already loaded at the preferred load
-	 * address, relocate it.
+	 * If the kernel isn't already loaded at a suitable address,
+	 * relocate it.
+	 *
+	 * It must be loaded above LOAD_PHYSICAL_ADDR.
+	 *
+	 * The maximum address for 64-bit is 1 << 46 for 4-level paging. This
+	 * is defined as the macro MAXMEM, but unfortunately that is not a
+	 * compile-time constant if 5-level paging is configured, so we instead
+	 * define our own macro for use here.
+	 *
+	 * For 32-bit, the maximum address is complicated to figure out, for
+	 * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
+	 * KASLR uses.
+	 *
+	 * Also relocate it if image_offset is zero, i.e. we weren't loaded by
+	 * LoadImage, but we are not aligned correctly.
 	 */
-	if (bzimage_addr - image_offset != hdr->pref_address) {
+
+	buffer_start = ALIGN(bzimage_addr - image_offset,
+			     hdr->kernel_alignment);
+	buffer_end = buffer_start + hdr->init_size;
+
+	if ((buffer_start < LOAD_PHYSICAL_ADDR)				     ||
+	    (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE)    ||
+	    (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
+	    (image_offset == 0 && !IS_ALIGNED(bzimage_addr,
+					      hdr->kernel_alignment))) {
 		status = efi_relocate_kernel(&bzimage_addr,
 					     hdr->init_size, hdr->init_size,
 					     hdr->pref_address,
-- 
2.24.1

Re: [PATCH v2 5/5] efi/x86: Don't relocate the kernel unless necessary

[Ard Biesheuvel]

On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Add alignment slack to the PE image size, so that we can realign the
> decompression buffer within the space allocated for the image.
>
> Only relocate the kernel if it has been loaded at an unsuitable address:
> * Below LOAD_PHYSICAL_ADDR, or
> * Above 64T for 64-bit and 512MiB for 32-bit
>
> For 32-bit, the upper limit is conservative, but the exact limit can be
> difficult to calculate.
>

Could we get rid of the call to efi_low_alloc_above() in
efi_relocate_kernel(), and just allocate top down with the right
alignment? I'd like to get rid of efi_low_alloc() et al if we can.



> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/tools/build.c             | 16 +++++-------
>  drivers/firmware/efi/libstub/x86-stub.c | 33 ++++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 3d03ad753ed5..db528961c283 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -238,21 +238,17 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
>
>         pe_header = get_unaligned_le32(&buf[0x3c]);
>
> -#ifdef CONFIG_EFI_MIXED
>         /*
> -        * In mixed mode, we will execute startup_32() at whichever offset in
> -        * memory it happened to land when the PE/COFF loader loaded the image,
> -        * which may be misaligned with respect to the kernel_alignment field
> -        * in the setup header.
> +        * The PE/COFF loader may load the image at an address which is
> +        * misaligned with respect to the kernel_alignment field in the setup
> +        * header.
>          *
> -        * In order for startup_32 to safely execute in place at this offset,
> -        * we need to ensure that the CONFIG_PHYSICAL_ALIGN aligned allocation
> -        * it creates for the page tables does not extend beyond the declared
> -        * size of the image in the PE/COFF header. So add the required slack.
> +        * In order to avoid relocating the kernel to correct the misalignment,
> +        * add slack to allow the buffer to be aligned within the declared size
> +        * of the image.
>          */
>         bss_sz  += CONFIG_PHYSICAL_ALIGN;
>         init_sz += CONFIG_PHYSICAL_ALIGN;
> -#endif
>
>         /*
>          * Size of code: Subtract the size of the first sector (512 bytes)
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index e71b8421e088..fbc4354f534c 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -17,6 +17,9 @@
>
>  #include "efistub.h"
>
> +/* Maximum physical address for 64-bit kernel with 4-level paging */
> +#define MAXMEM_X86_64_4LEVEL (1ull << 46)
> +
>  static efi_system_table_t *sys_table;
>  extern const bool efi_is64;
>  extern u32 image_offset;
> @@ -717,6 +720,7 @@ unsigned long efi_main(efi_handle_t handle,
>                              struct boot_params *boot_params)
>  {
>         unsigned long bzimage_addr = (unsigned long)startup_32;
> +       unsigned long buffer_start, buffer_end;
>         struct setup_header *hdr = &boot_params->hdr;
>         efi_status_t status;
>         unsigned long cmdline_paddr;
> @@ -728,10 +732,33 @@ unsigned long efi_main(efi_handle_t handle,
>                 efi_exit(handle, EFI_INVALID_PARAMETER);
>
>         /*
> -        * If the kernel isn't already loaded at the preferred load
> -        * address, relocate it.
> +        * If the kernel isn't already loaded at a suitable address,
> +        * relocate it.
> +        *
> +        * It must be loaded above LOAD_PHYSICAL_ADDR.
> +        *
> +        * The maximum address for 64-bit is 1 << 46 for 4-level paging. This
> +        * is defined as the macro MAXMEM, but unfortunately that is not a
> +        * compile-time constant if 5-level paging is configured, so we instead
> +        * define our own macro for use here.
> +        *
> +        * For 32-bit, the maximum address is complicated to figure out, for
> +        * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
> +        * KASLR uses.
> +        *
> +        * Also relocate it if image_offset is zero, i.e. we weren't loaded by
> +        * LoadImage, but we are not aligned correctly.
>          */
> -       if (bzimage_addr - image_offset != hdr->pref_address) {
> +
> +       buffer_start = ALIGN(bzimage_addr - image_offset,
> +                            hdr->kernel_alignment);
> +       buffer_end = buffer_start + hdr->init_size;
> +
> +       if ((buffer_start < LOAD_PHYSICAL_ADDR)                              ||
> +           (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE)    ||
> +           (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
> +           (image_offset == 0 && !IS_ALIGNED(bzimage_addr,
> +                                             hdr->kernel_alignment))) {
>                 status = efi_relocate_kernel(&bzimage_addr,
>                                              hdr->init_size, hdr->init_size,
>                                              hdr->pref_address,
> --
> 2.24.1
>

Re: [PATCH v2 5/5] efi/x86: Don't relocate the kernel unless necessary

[Arvind Sankar]

On Wed, Mar 04, 2020 at 12:08:33AM +0100, Ard Biesheuvel wrote:
> On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > Add alignment slack to the PE image size, so that we can realign the
> > decompression buffer within the space allocated for the image.
> >
> > Only relocate the kernel if it has been loaded at an unsuitable address:
> > * Below LOAD_PHYSICAL_ADDR, or
> > * Above 64T for 64-bit and 512MiB for 32-bit
> >
> > For 32-bit, the upper limit is conservative, but the exact limit can be
> > difficult to calculate.
> >
> 
> Could we get rid of the call to efi_low_alloc_above() in
> efi_relocate_kernel(), and just allocate top down with the right
> alignment? I'd like to get rid of efi_low_alloc() et al if we can.
> 

But we don't have a top-down allocator, do we? ALLOCATE_MAX_ADDRESS
guarantees the maximum, but it doesn't guarantee that you'll be as high
as possible.

Re: [PATCH v2 5/5] efi/x86: Don't relocate the kernel unless necessary

[Ard Biesheuvel]

On Wed, 4 Mar 2020 at 00:35, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Mar 04, 2020 at 12:08:33AM +0100, Ard Biesheuvel wrote:
> > On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > Add alignment slack to the PE image size, so that we can realign the
> > > decompression buffer within the space allocated for the image.
> > >
> > > Only relocate the kernel if it has been loaded at an unsuitable address:
> > > * Below LOAD_PHYSICAL_ADDR, or
> > > * Above 64T for 64-bit and 512MiB for 32-bit
> > >
> > > For 32-bit, the upper limit is conservative, but the exact limit can be
> > > difficult to calculate.
> > >
> >
> > Could we get rid of the call to efi_low_alloc_above() in
> > efi_relocate_kernel(), and just allocate top down with the right
> > alignment? I'd like to get rid of efi_low_alloc() et al if we can.
> >
>
> But we don't have a top-down allocator, do we? ALLOCATE_MAX_ADDRESS
> guarantees the maximum, but it doesn't guarantee that you'll be as high
> as possible.

Good point. We do have a top-down allocator in practice, but it is not
guaranteed by the API.

Re: [PATCH v2 0/5] Minimize the need to move the kernel in the EFI stub

[Ard Biesheuvel]

On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> This series adds the ability to use the entire PE image space for
> decompression, provides the preferred address to the PE loader via the
> header, and finally restricts efi_relocate_kernel to cases where we
> really need it rather than whenever we were loaded at something other
> than preferred address.
>
> Based on tip:efi/core + the cleanup series [1]
> [1] https://lore.kernel.org/linux-efi/20200301230436.2246909-1-nivedita@alum.mit.edu/
>
> Changes from v1
> - clarify a few comments
> - cleanups to code formatting
>
> Arvind Sankar (5):
>   x86/boot/compressed/32: Save the output address instead of
>     recalculating it
>   efi/x86: Decompress at start of PE image load address
>   efi/x86: Add kernel preferred address to PE header
>   efi/x86: Remove extra headroom for setup block
>   efi/x86: Don't relocate the kernel unless necessary
>

Thanks. I have queued these up in efi/next, along with your mixed mode cleanups.


>  arch/x86/boot/compressed/head_32.S      | 42 +++++++++++++++-------
>  arch/x86/boot/compressed/head_64.S      | 42 ++++++++++++++++++++--
>  arch/x86/boot/header.S                  |  6 ++--
>  arch/x86/boot/tools/build.c             | 44 ++++++++++++++++-------
>  drivers/firmware/efi/libstub/x86-stub.c | 48 ++++++++++++++++++++++---
>  5 files changed, 147 insertions(+), 35 deletions(-)
>
> Range-diff against v1:
> 1:  0cdb6bf27a24 ! 1:  2ecbf60b9ecd x86/boot/compressed/32: Save the output address instead of recalculating it
>     @@ Metadata
>       ## Commit message ##
>          x86/boot/compressed/32: Save the output address instead of recalculating it
>
>     -    In preparation for being able to decompress starting at a different
>     -    address than startup_32, save the calculated output address instead of
>     -    recalculating it later.
>     +    In preparation for being able to decompress into a buffer starting at a
>     +    different address than startup_32, save the calculated output address
>     +    instead of recalculating it later.
>
>          We now keep track of three addresses:
>                  %edx: startup_32 as we were loaded by bootloader
> 2:  d4df840752ac ! 2:  e2bdbe6cb692 efi/x86: Decompress at start of PE image load address
>     @@ arch/x86/boot/compressed/head_64.S: SYM_FUNC_START(efi32_pe_entry)
>         movl    -4(%ebp), %esi                  // loaded_image
>         movl    LI32_image_base(%esi), %esi     // loaded_image->image_base
>         movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry
>     ++  /*
>     ++   * We need to set the image_offset variable here since startup_32 will
>     ++   * use it before we get to the 64-bit efi_pe_entry in C code.
>     ++   */
>      +  subl    %esi, %ebx
>      +  movl    %ebx, image_offset(%ebp)        // save image_offset
>         jmp     efi32_pe_stub_entry
>     @@ drivers/firmware/efi/libstub/x86-stub.c: unsigned long efi_main(efi_handle_t han
>                         efi_printk("efi_relocate_kernel() failed!\n");
>                         goto fail;
>                 }
>     ++          /*
>     ++           * Now that we've copied the kernel elsewhere, we no longer
>     ++           * have a setup block before startup_32, so reset image_offset
>     ++           * to zero in case it was set earlier.
>     ++           */
>      +          image_offset = 0;
>         }
>
> 3:  4bae68f25b90 ! 3:  ea840f78f138 efi/x86: Add kernel preferred address to PE header
>     @@ arch/x86/boot/header.S: optional_header:
>
>       extra_header_fields:
>      +  # PE specification requires ImageBase to be 64k-aligned
>     -+  .set    ImageBase, (LOAD_PHYSICAL_ADDR+0xffff) & ~0xffff
>     ++  .set    image_base, (LOAD_PHYSICAL_ADDR + 0xffff) & ~0xffff
>       #ifdef CONFIG_X86_32
>      -  .long   0                               # ImageBase
>     -+  .long   ImageBase                       # ImageBase
>     ++  .long   image_base                      # ImageBase
>       #else
>      -  .quad   0                               # ImageBase
>     -+  .quad   ImageBase                       # ImageBase
>     ++  .quad   image_base                      # ImageBase
>       #endif
>         .long   0x20                            # SectionAlignment
>         .long   0x20                            # FileAlignment
> 4:  2330a25c6b0f ! 4:  c25a9b507d6d efi/x86: Remove extra headroom for setup block
>     @@ Commit message
>          account for setup block") added headroom to the PE image to account for
>          the setup block, which wasn't used for the decompression buffer.
>
>     -    Now that we decompress from the start of the image, this is no longer
>     -    required.
>     +    Now that the decompression buffer is located at the start of the image,
>     +    and includes the setup block, this is no longer required.
>
>          Add a check to make sure that the head section of the compressed kernel
>          won't overwrite itself while relocating. This is only for
> 5:  2081f91cbe75 ! 5:  d3dc3af1c7b8 efi/x86: Don't relocate the kernel unless necessary
>     @@ arch/x86/boot/tools/build.c: static void update_pecoff_text(unsigned int text_st
>          * Size of code: Subtract the size of the first sector (512 bytes)
>
>       ## drivers/firmware/efi/libstub/x86-stub.c ##
>     +@@
>     +
>     + #include "efistub.h"
>     +
>     ++/* Maximum physical address for 64-bit kernel with 4-level paging */
>     ++#define MAXMEM_X86_64_4LEVEL (1ull << 46)
>     ++
>     + static efi_system_table_t *sys_table;
>     + extern const bool efi_is64;
>     + extern u32 image_offset;
>      @@ drivers/firmware/efi/libstub/x86-stub.c: unsigned long efi_main(efi_handle_t handle,
>                              struct boot_params *boot_params)
>       {
>     @@ drivers/firmware/efi/libstub/x86-stub.c: unsigned long efi_main(efi_handle_t han
>      -   * address, relocate it.
>      +   * If the kernel isn't already loaded at a suitable address,
>      +   * relocate it.
>     ++   *
>      +   * It must be loaded above LOAD_PHYSICAL_ADDR.
>     -+   * The maximum address for 64-bit is 1 << 46 for 4-level paging.
>     ++   *
>     ++   * The maximum address for 64-bit is 1 << 46 for 4-level paging. This
>     ++   * is defined as the macro MAXMEM, but unfortunately that is not a
>     ++   * compile-time constant if 5-level paging is configured, so we instead
>     ++   * define our own macro for use here.
>     ++   *
>      +   * For 32-bit, the maximum address is complicated to figure out, for
>      +   * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
>      +   * KASLR uses.
>     ++   *
>      +   * Also relocate it if image_offset is zero, i.e. we weren't loaded by
>      +   * LoadImage, but we are not aligned correctly.
>          */
>      -  if (bzimage_addr - image_offset != hdr->pref_address) {
>     ++
>      +  buffer_start = ALIGN(bzimage_addr - image_offset,
>      +                       hdr->kernel_alignment);
>      +  buffer_end = buffer_start + hdr->init_size;
>      +
>     -+  if (buffer_start < LOAD_PHYSICAL_ADDR
>     -+      || IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE
>     -+      || IS_ENABLED(CONFIG_X86_64) && buffer_end > 1ull << 46
>     -+      || image_offset == 0 && !IS_ALIGNED(bzimage_addr,
>     -+                                          hdr->kernel_alignment)) {
>     ++  if ((buffer_start < LOAD_PHYSICAL_ADDR)                              ||
>     ++      (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE)    ||
>     ++      (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
>     ++      (image_offset == 0 && !IS_ALIGNED(bzimage_addr,
>     ++                                        hdr->kernel_alignment))) {
>                 status = efi_relocate_kernel(&bzimage_addr,
>                                              hdr->init_size, hdr->init_size,
>                                              hdr->pref_address,
> --
> 2.24.1
>