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. > > 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 > > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel