Re: [RFC PATCH v2] util/mkimage: Fix wrong PE32+ section sizes for some arches

[Daniel Kiper]

On Mon, Apr 26, 2021 at 09:21:02AM +0200, Javier Martinez Canillas wrote:
> Commit f60ba9e5945 (util/mkimage: Refactor section setup to use a helper)
> added a helper function to setup PE sections. But it also changed how the
> raw data offsets were calculated since all the section sizes are aligned.
>
> But for some platforms (i.e: ia64 and aa64) the kernel image size wasn't

s/But/However, /
s/(i.e: ia64 and aa64)/, i.e ia64-efi and arm64-efi,/
s/wasn't/is not/

> aligned using the section alignment, which causes the PE section headers

s/, which causes the PE section headers/./

> to not match the actual section sizes in the PE32+ binary file.

s/to not match the actual section sizes in the PE32+ binary file./
 /This leads to the situation in which the mods section offset in its
  PE section header does not match its real placement in the PE file.
  So, finally the GRUB is not able to locate and load built-in modules./

The problem surfaces on ia64-efi and arm64-efi because both platforms
require additional relocation data which is added behind .bss section.
So, we have to add some padding behind this extra data to make the
beginning of mods section properly aligned in the PE file.

> This caused problems on ia64 EFI machines, since the .data section size
> is bigger than the actual section in the PE32+ binary, overlapping with
> part of the mods section. That leads to GRUB not being able to load any
> built-in module.

Please drop this paragraph...

> Fix it by aligning the kernel_size to the section alignment, that makes

s/, t/. T/

> the sizes and offsets in the PE section headers to match the sections

s/the sections/relevant sections/

> in the PE32+ binary file.
>
> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> Changes in v2:
> - Align up for any arch in the is_relocatable (image_target) patch and
>   not only for MKIMAGE_ELF64 or EM_AARCH64 (suggested by Daniel Kiper).
>
>  util/grub-mkimagexx.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 00f49ccaaaf..73646f12f14 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -2388,6 +2388,11 @@ SUFFIX (grub_mkimage_load_image) (const char 
> *kernel_path,
>         layout->kernel_size += ALIGN_UP (layout->got_size, 16);
>       }
>  #endif
> +
> +      if (image_target->id == IMAGE_EFI)
> +        layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> image_target->vaddr_offset,
> +                                        image_target->section_align)
> +          - image_target->vaddr_offset;

I think this should be:
  layout->kernel_size = ALIGN_UP (layout->kernel_size, 
GRUB_PE32_FILE_ALIGNMENT);

Here we care about alignment in the PE file not in the memory. So,
I think we have to use GRUB_PE32_FILE_ALIGNMENT instead. Though
the result is the same...

Daniel