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/[email protected]/
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
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 <[email protected]>
---
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
On Mon, 2 Mar 2020 at 00:05, Arvind Sankar <[email protected]> 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 <[email protected]>
> ---
> 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
>
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/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <[email protected]> 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/[email protected]/
>
> 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
>
On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Wed, Mar 04, 2020 at 12:08:33AM +0100, Ard Biesheuvel wrote:
> On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <[email protected]> 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.
On Wed, 4 Mar 2020 at 00:35, Arvind Sankar <[email protected]> wrote:
>
> On Wed, Mar 04, 2020 at 12:08:33AM +0100, Ard Biesheuvel wrote:
> > On Tue, 3 Mar 2020 at 23:12, Arvind Sankar <[email protected]> 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.
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
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?
On Mon, 11 May 2020 at 20:36, Arvind Sankar <[email protected]> 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?
On Mon, May 11, 2020 at 11:13:00PM +0200, Ard Biesheuvel wrote:
> On Mon, 11 May 2020 at 20:36, Arvind Sankar <[email protected]> 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.
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 <[email protected]>
---
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
Feel free to add my tested by
On Mon, 11 May 2020 at 23:58, Arvind Sankar <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Tue, 12 May 2020 at 01:12, Mike Lothian <[email protected]> wrote:
>
> Feel free to add my tested by
>
>
> On Mon, 11 May 2020 at 23:58, Arvind Sankar <[email protected]> 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 <[email protected]>
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
> >
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 <[email protected]>
AuthorDate: Mon, 11 May 2020 18:58:49 -04:00
Committer: Ard Biesheuvel <[email protected]>
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 <[email protected]>
Tested-by: Mike Lothian <[email protected]>
Signed-off-by: Arvind Sankar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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;
/*----------------------------------------------------------------------*/