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

[Ard Biesheuvel]

On Mon, 5 Sept 2022 at 00:12, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 2 Sept 2022 at 12:02, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > On 8/18/22 16:51, 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 allows 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 <ardb@kernel.org>
> > > ---
> > >   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 7c0f17cf933d..56ba8d0a6ea3 100644
> > > --- a/grub-core/loader/arm64/linux.c
> > > +++ b/grub-core/loader/arm64/linux.c
> > > @@ -63,6 +63,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);
> > > +
> > > +      if (grub_file_read (file, &lh->coff_image_header, sizeof(struct 
> > > grub_coff_image_header))
> > > +         != sizeof(struct grub_coff_image_header))
> > > +       return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF 
> > > image header");
> > > +    }
> >
> > Why do we use multiple reads? We need the whole binary for booting.
>
> We need multiple reads because the PE/COFF header describes the
> mapping from file offsets to memory offsets - even if Linux kernels
> generally use a 1:1 mapping between the location of a PE/COFF section
> in the file and in memory, the PE/COFF spec does not require this at
> all.
>
> So reading the whole file into memory might require multiple
> memcpy/memmove calls to rearrange the copied file in memory so it
> matches the section layout as the header describes it.
>
> Therefore, reading just the header is a reasonable thing to do here.
> And note that the second read only happens when the header layout
> deviates from the expected default.
>
> > Reading the complete file into memory once should be good enough. But
> > that seems to be more a question of overall design.
> >
> > In grub_efi_modules_addr() the same logic is needed.
>
> Not really. grub_efi_modules_addr() is only used to load GRUB's own
> PE/COFF image, and its layout is known a priori. So having this logic
> there is essentially dead code.
>
> > It is not ARM
> > specific. Please, move it to a common code module.
> >
>
> That makes sense - I'll move it to an appropriate common source file.
>

Actually, this is not as straight-forward as it may seem: struct
linux_arch_kernel_header is used in this function, which is specific
to linux so it does not belong in the generic EFI part of the code.

I am going to leave this as-is for the time being.