On Thu, Sep 15, 2022 at 04:03:36PM +0200, Ard Biesheuvel wrote:
> On Thu, 15 Sept 2022 at 14:21, Leif Lindholm <quic_llind...@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 <a...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
