Re: [PATCH v2 2/2] mkimage: arm64-efi: Align first section to page

[Alexander Graf]

On 01/14/2019 02:37 PM, Daniel Kiper wrote:
> On Sun, Dec 23, 2018 at 03:52:07AM +0100, Alexander Graf wrote:
> > In order to enforce NX semantics on non-code pages, UEFI firmware
> > may require that all code is EFI_PAGE_SIZE (4k) aligned. A similar
> > change has recently been applied to edk2 to accomodate for the same
> > fact:
> >
> >    https://lists.01.org/pipermail/edk2-devel/2018-December/033708.html
> >
> > This patch adapts grub to also implement the same alignment guarantees
> > and thus ensures we can boot even when strict permission checks are in
> > place.
> >
> > Signed-off-by: Alexander Graf <address@hidden>
> >
> > ---
> >
> > v1 -> v2:
> >
> >    - Mention only NX requirement in patch description
> >    - Use GRUB_EFI_PAGE_SIZE
> > ---
> >   util/mkimage.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/mkimage.c b/util/mkimage.c
> > index 88b991764..de93c5a13 100644
> > --- a/util/mkimage.c
> > +++ b/util/mkimage.c
> > @@ -39,6 +39,7 @@
> >   #include <string.h>
> >   #include <stdlib.h>
> >   #include <assert.h>
> > +#include <grub/efi/memory.h>
> >   #include <grub/efi/pe32.h>
> >   #include <grub/uboot/image.h>
> >   #include <grub/arm/reloc.h>
> > @@ -623,7 +624,7 @@ static const struct grub_install_image_target_desc 
> > image_targets[] =
> >         .decompressor_uncompressed_size = TARGET_NO_FIELD,
> >         .decompressor_uncompressed_addr = TARGET_NO_FIELD,
> >         .section_align = GRUB_PE32_SECTION_ALIGNMENT,
> > -      .vaddr_offset = EFI64_HEADER_SIZE,
> > +      .vaddr_offset = GRUB_EFI_PAGE_SIZE,
> Nack.
>
> I think that we will soon have the same problem on other targtes.
> So, I would try this:
>
> #define EFI32_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE           \
>                                      + GRUB_PE32_SIGNATURE_SIZE          \
>                                      + sizeof (struct grub_pe32_coff_header) \
>                                      + sizeof (struct 
> grub_pe32_optional_header) \
>                                      + 4 * sizeof (struct 
> grub_pe32_section_table), \
>                                      ALIGN_UP (GRUB_PE32_SECTION_ALIGNMENT, 
> GRUB_EFI_PAGE_SIZE))
>
> #define EFI64_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE           \
>                                      + GRUB_PE32_SIGNATURE_SIZE          \
>                                      + sizeof (struct grub_pe32_coff_header) \
>                                      + sizeof (struct 
> grub_pe64_optional_header) \
>                                      + 4 * sizeof (struct 
> grub_pe32_section_table), \
>                                      ALIGN_UP (GRUB_PE32_SECTION_ALIGNMENT, 
> GRUB_EFI_PAGE_SIZE))
>
> And there is another problem with your proposal. What will happen
> if EFI64_HEADER_SIZE > GRUB_EFI_PAGE_SIZE?

I don't think it ever can be, can it? The header size is pretty constant.

> Additionally, why arm-efi is different?

Mostly because in the wild I have not seen anyone on non-aarch64 pull in 
page alignment requirements :).

But yes, I'll be happy to redo the patch and make EFI binaries always 4k 
aligned. I also learned in an off-list mailing list thread, that newer 
versions of that Qcom firmware require 4k section alignments, so I will 
push for that across all targets too. That way we should be aligned with 
the MS compiler suite.


Alex