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

[Daniel Kiper]

On Thu, Sep 15, 2022 at 04:03:36PM +0200, Ard Biesheuvel wrote:
> 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.

After checking the PE/COFF spec it seems to me the grub_coff_image_header
should be renamed to grub_pe_image_header. Then everything should be OK.

Daniel