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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 4/9/21 8:10 AM, Ard Biesheuvel wrote:
> On Thu, 8 Apr 2021 at 20:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/25/20 2:49 PM, 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 <ard.biesheuvel@arm.com>
>>> ---
>>>    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 e0165e74c587..9e5a72be538d 100644
>>> --- a/grub-core/kern/efi/efi.c
>>> +++ b/grub-core/kern/efi/efi.c
>>> @@ -282,7 +282,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;
>>> @@ -293,7 +293,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);
>>
>> After checking that the file starts with the letters 'MZ' we can find at
>> file offset 0x3c the position of the 'PE\0\0' indicating the start of
>> the COFF header. This avoids relying upon any fixed offset and conforms
>> to the PE Format specification.
>>
>
> I think you might be missing the point here. This is not about
> arbitrary PE/COFF images that GRUB loads, this is about the PE/COFF
> image that is constructed by the GRUB build system to carry GRUB
> itself: what would be the point of extracting the offset from the
> image if we always put the PE header at the same offset?

We should not assume that a file /boot/grub/x86_64-efi/core.efi contains
a GRUB EFI binary. We should check the header. If we can't find 'MZ' at
offset 0 and 'PE\0\0' at the position indicated by the offset at 0x3c,
it is definitively not a valid EFI binary.

I would prefer one common function for checking EFI headers instead of
grub_arch_efi_linux_check_image() per architecture.

Best regards

Heinrich

>
> In any case, I don't care deeply one way or the other, so feel free to
> propose an alternative to this patch.
>
>
>> See
>> https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#ms-dos-stub-image-only
>>
>> 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];
>>>
>>>
>>