Re: [PATCH v3 2/8] efi: move MS-DOS stub out of generic PE header definition

[Heinrich Schuchardt]

On 8/18/22 16:51, 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,
> use the GRUB_PE32_MSDOS_STUB_SIZE macro explicitly to reference the
> COFF header in the only place in the code where we rely on this.
>
> 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 | 5 +++--
>   include/grub/efi/pe32.h  | 5 +----
>   2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index e8a976a22f15..8bef81663853 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -302,7 +302,7 @@ grub_addr_t
>   grub_efi_modules_addr (void)
>   {
>     grub_efi_loaded_image_t *image;
> -  struct grub_pe32_header *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 +313,8 @@ grub_efi_modules_addr (void)
>     if (! image)
>       return 0;
>   
> -  header = image->image_base;
> +  header = (struct grub_coff_image_header *) ((char *) image->image_base
> +                                             + GRUB_PE32_MSDOS_STUB_SIZE);

As you described above the offset to the PE-COFF header isn't a constant.

Please, cast image->image_base to struct IMAGE_DOS_HEADER * and use the 
value of e_lfanew instead of GRUB_PE32_MSDOS_STUB_SIZE here.

You can copy the definjtion of struct IMAGE_DOS_HEADER from gnu-efi's 
/usr/include/efi/x86_64/pe.h.

Best regards

Heinrich

>     coff_header = &(header->coff_header);
>     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..a2da4b318c85 100644
> --- a/include/grub/efi/pe32.h
> +++ b/include/grub/efi/pe32.h
> @@ -254,11 +254,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];
>