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

[Leif Lindholm]

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
                                              );
?

/
    Leif

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