Re: [PATCH v2 3/8] efi: move MS-DOS stub out of generic PE header definition

[Heinrich Schuchardt]

On 4/9/21 8:10 AM, Ard Biesheuvel wrote:
> On Thu, 8 Apr 2021 at 20:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
> > > The PE/COFF spec permits the COFF signature and file header to appear
> > > anywhere in the file, and the actual offset is recorded in 4 byte
> > > little endian field at offset 0x3c of the image.
> > >
> > > When GRUB is emitted as a PE/COFF binary, we reuse the 128 byte MS-DOS
> > > stub (even for non-x86 architectures), putting the COFF signature and
> > > file header at offset 0x80. However, other PE/COFF images may use
> > > different values, and non-x86 Linux kernels use an offset of 0x40
> > > instead.
> > >
> > > So let's get rid of the grub_pe32_header struct from pe32.h, given that
> > > it does not represent anything defined by the PE/COFF spec. Instead,
> > > use the GRUB_PE32_MSDOS_STUB_SIZE macro explicitly to reference the
> > > COFF header in the only place in the code where we rely on this.
> > >
> > > The remaining fields are moved into a struct grub_coff_image_header,
> > > which we will use later to access COFF header fields of arbitrary
> > > images (and which may therefore appear at different offsets)
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > ---
> > >    grub-core/kern/efi/efi.c | 5 +++--
> > >    include/grub/efi/pe32.h  | 5 +----
> > >    2 files changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > > index e0165e74c587..9e5a72be538d 100644
> > > --- a/grub-core/kern/efi/efi.c
> > > +++ b/grub-core/kern/efi/efi.c
> > > @@ -282,7 +282,7 @@ grub_addr_t
> > >    grub_efi_modules_addr (void)
> > >    {
> > >      grub_efi_loaded_image_t *image;
> > > -  struct grub_pe32_header *header;
> > > +  struct grub_coff_image_header *header;
> > >      struct grub_pe32_coff_header *coff_header;
> > >      struct grub_pe32_section_table *sections;
> > >      struct grub_pe32_section_table *section;
> > > @@ -293,7 +293,8 @@ grub_efi_modules_addr (void)
> > >      if (! image)
> > >        return 0;
> > >
> > > -  header = image->image_base;
> > > +  header = (struct grub_coff_image_header *) ((char *) image->image_base
> > > +                                           + GRUB_PE32_MSDOS_STUB_SIZE);
> >
> > After checking that the file starts with the letters 'MZ' we can find at
> > file offset 0x3c the position of the 'PE\0\0' indicating the start of
> > the COFF header. This avoids relying upon any fixed offset and conforms
> > to the PE Format specification.
>
> I think you might be missing the point here. This is not about
> arbitrary PE/COFF images that GRUB loads, this is about the PE/COFF
> image that is constructed by the GRUB build system to carry GRUB
> itself: what would be the point of extracting the offset from the
> image if we always put the PE header at the same offset?

We should not assume that a file /boot/grub/x86_64-efi/core.efi contains
a GRUB EFI binary. We should check the header. If we can't find 'MZ' at
offset 0 and 'PE\0\0' at the position indicated by the offset at 0x3c,
it is definitively not a valid EFI binary.

I would prefer one common function for checking EFI headers instead of
grub_arch_efi_linux_check_image() per architecture.

Best regards

Heinrich

> In any case, I don't care deeply one way or the other, so feel free to
> propose an alternative to this patch.
>
>
> > See
> > https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#ms-dos-stub-image-only
> >
> > Best regards
> >
> > Heinrich
> >
> > >      coff_header = &(header->coff_header);
> > >      sections
> > >        = (struct grub_pe32_section_table *) ((char *) coff_header
> > > diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> > > index 0ed8781f0376..a2da4b318c85 100644
> > > --- a/include/grub/efi/pe32.h
> > > +++ b/include/grub/efi/pe32.h
> > > @@ -254,11 +254,8 @@ struct grub_pe32_section_table
> > >
> > >    #define GRUB_PE32_SIGNATURE_SIZE 4
> > >
> > > -struct grub_pe32_header
> > > +struct grub_coff_image_header
> > >    {
> > > -  /* This should be filled in with GRUB_PE32_MSDOS_STUB.  */
> > > -  grub_uint8_t msdos_stub[GRUB_PE32_MSDOS_STUB_SIZE];
> > > -
> > >      /* This is always PE\0\0.  */
> > >      char signature[GRUB_PE32_SIGNATURE_SIZE];