Re: [PATCH v3 5/8] linux/arm: account for COFF headers appearing at unexpected offsets

[Heinrich Schuchardt]

On 8/18/22 16:51, Ard Biesheuvel wrote:
> The way we load the Linux and PE/COFF image headers depends on a fixed
> placement of the COFF header at offset 0x40 into the file. This is a
> reasonable default, given that this is where Linux emits it today.
> However, in order to comply with the PE/COFF spec, which allows this
> header to appear anywhere in the file, let's ensure that we read the
> header from where it actually appears in the file if it is not located
> at offset 0x40.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   grub-core/loader/arm64/linux.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 7c0f17cf933d..56ba8d0a6ea3 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -63,6 +63,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
>     grub_dprintf ("linux", "UEFI stub kernel:\n");
>     grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
>   
> +  /*
> +   * The PE/COFF spec permits the COFF header to appear anywhere in the file, 
> so
> +   * we need to double check whether it was where we expected it, and if not, 
> we
> +   * must load it from the correct offset into the coff_image_header field of
> +   * struct linux_arch_kernel_header.
> +   */
> +  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) 
> &lh->coff_image_header)
> +    {
> +      grub_file_seek (file, lh->hdr_offset);
> +
> +      if (grub_file_read (file, &lh->coff_image_header, sizeof(struct 
> grub_coff_image_header))
> +         != sizeof(struct grub_coff_image_header))
> +       return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image 
> header");
> +    }

Why do we use multiple reads? We need the whole binary for booting. 
Reading the complete file into memory once should be good enough. But 
that seems to be more a question of overall design.

In grub_efi_modules_addr() the same logic is needed. It is not ARM 
specific. Please, move it to a common code module.

Best regards

Heinrich

> +
>     return GRUB_ERR_NONE;
>   }
>