Re: [PATCH v4 1/6] efi: move MS-DOS stub out of generic PE header definition

[Ard Biesheuvel]

On Thu, 15 Sept 2022 at 14:21, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Thu, Sep 08, 2022 at 15:30:12 +0200, 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,
> > introduce a minimal struct grub_msdos_image_header type based on the
> > PE/COFF spec's description of the image header, and use the offset
> > recorded at file position 0x3c to discover the actual location of the PE
> > signature and the COFF image header.
> >
> > 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 <ardb@kernel.org>
> > ---
> >  grub-core/kern/efi/efi.c |  8 ++++++--
> >  include/grub/efi/pe32.h  | 16 ++++++++++++----
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > index e8a976a22f15..f85587d66635 100644
> > --- a/grub-core/kern/efi/efi.c
> > +++ b/grub-core/kern/efi/efi.c
> > @@ -302,7 +302,8 @@ grub_addr_t
> >  grub_efi_modules_addr (void)
> >  {
> >    grub_efi_loaded_image_t *image;
> > -  struct grub_pe32_header *header;
> > +  struct grub_msdos_image_header *dos_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;
> > @@ -313,7 +314,10 @@ grub_efi_modules_addr (void)
> >    if (! image)
> >      return 0;
> >
> > -  header = image->image_base;
> > +  dos_header = (struct grub_msdos_image_header *)image->image_base;
> > +
> > +  header = (struct grub_coff_image_header *) ((char *) dos_header
> > +                                              + 
> > dos_header->pe_signature_offset);
> >    coff_header = &(header->coff_header);
>
> This is where I get semantically confused.
> We now have a coff_image_header called header (pointing at the space
> for the PE\0\0 signature, which comes immediately before the COFF
> header, and isn't part of it).
>
> And then we have a pe32_coff_header called coff_header, pointing at
> the machine field (the start) of the COFF header.
>
> Since "header" is no longer referring to an image header, could we
> chuck out the signature as well from the structure and add
> GRUB_PE32_SIGNATURE_SIZE when calculating?
>
> I.e. drop "header" altogether and do:
>
>   dos_header = (struct grub_msdos_image_header *)image->image_base;
>
>   coff_header = (struct grub_coff_image_header *) ((char *) dos_header
>                                               + 
> dos_header->pe_signature_offset
>                                               + GRUB_PE32_SIGNATURE_SIZE
>                                               );
> ?
>

I suppose that should work, but it does mean we have to add  a 'char
signature[GRUB_PE32_SIGNATURE_SIZE];' member to the existing structs
that incorporate grub_coff_image_header, along with adding
GRUB_PE32_SIGNATURE_SIZE every time we refer to the PE header offset.


> >    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..6688d96c0046 100644
> > --- a/include/grub/efi/pe32.h
> > +++ b/include/grub/efi/pe32.h
> > @@ -48,6 +48,17 @@
> >
> >  #define GRUB_PE32_MAGIC                      0x5a4d
> >
> > +struct grub_msdos_image_header
> > +{
> > +  /* This is always 'MZ'. (GRUB_PE32_MAGIC)  */
> > +  grub_uint16_t msdos_magic;
> > +
> > +  grub_uint16_t reserved[29];
> > +
> > +  /* The file offset of the PE signature and COFF image header. */
> > +  grub_uint32_t pe_signature_offset;
> > +};
> > +
> >  /* According to the spec, the minimal alignment is 512 bytes...
> >     But some examples (such as EFI drivers in the Intel
> >     Sample Implementation) use 32 bytes (0x20) instead, and it seems
> > @@ -254,11 +265,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];
> >
> > --
> > 2.35.1
> >