Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200301230537.2247550-1-nivedita@alum.mit.edu> <20200301230537.2247550-3-nivedita@alum.mit.edu>
In-Reply-To: <20200301230537.2247550-3-nivedita@alum.mit.edu>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Tue, 3 Mar 2020 20:08:36 +0100
Message-ID: <CAKv+Gu-f=mZwxAyLOzkrtSFPpxsKweRU2rKBUwTK0_r7s6gZPQ@mail.gmail.com>
Subject: Re: [PATCH 2/5] efi/x86: Decompress at start of PE image load address
To:     Arvind Sankar <nivedita@alum.mit.edu>
Cc:     linux-efi <linux-efi@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>