Re: [PATCH v2 5/8] linux/arm: account for COFF headers appearing at unexpected offsets

[Ard Biesheuvel]

On Fri, 9 Apr 2021 at 08:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/9/21 8:12 AM, Ard Biesheuvel wrote:
> > On Thu, 8 Apr 2021 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
> >>> The way we load the Linux and PE/COFF image headers depends on a fixed
> >>> placement of the COFF header at offset 0x40 into the file. This is a
> >>> reasonable default, given that this is where Linux emits it today.
> >>> However, in order to comply with the PE/COFF spec, which permits this
> >>> header to appear anywhere in the file, let's ensure that we read the
> >>> header from where it actually appears in the file if it is not located
> >>> at offset 0x40.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> >>> ---
> >>>    grub-core/loader/arm64/linux.c | 15 +++++++++++++++
> >>>    1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/grub-core/loader/arm64/linux.c 
> >>> b/grub-core/loader/arm64/linux.c
> >>> index 915b6ad7292d..28ff8584a3b5 100644
> >>> --- a/grub-core/loader/arm64/linux.c
> >>> +++ b/grub-core/loader/arm64/linux.c
> >>> @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t 
> >>> file,
> >>>      grub_dprintf ("linux", "UEFI stub kernel:\n");
> >>>      grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> >>>
> >>> +  /*
> >>> +   * The PE/COFF spec permits the COFF header to appear anywhere in the 
> >>> file, so
> >>> +   * we need to double check whether it was where we expected it, and if 
> >>> not, we
> >>> +   * must load it from the correct offset into the coff_image_header 
> >>> field of
> >>> +   * struct linux_arch_kernel_header.
> >>> +   */
> >>> +  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) 
> >>> &lh->coff_image_header)
> >>> +    {
> >>> +      grub_file_seek (file, lh->hdr_offset);
> >>
> >> Isn't this overly complicated? Why don't we first read the whole file
> >> into memory and then analyze it instead of using multiple accesses which
> >> only slows down the process?
> >>
> >
> > Given that the condition will never hold in practice, as the offset is
> > always going to be 0x40, this change is not expected to affect
> > performance at all.
>
> The PE COFF specification let's you specify any value. The linux command
> can be used to launch arbitrary EFI binaries if they have the Linux
> magic 'ARM\x64' in the right place.
>
> What I never understood is why the linux command is checking this Linux
> magic field at all instead of running any EFI binary thrown at it.
>

I don't disagree with you on that. The question is whether it should
be in scope for this change to fix all of that.