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

[Ard Biesheuvel <ardb@kernel.org>]

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?

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];
> >
> >
>