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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Jan 14, 2019 at 02:41:30PM +0100, Alexander Graf wrote:
> 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 <agraf@suse.de>
> > > 
> > > ---
> > > 
> > > 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 :).

arm-efi is mainly different in that I don't expect non-embedded 32-bit
devices to show up. There would be nothing wrong in applying the same
change.

There is an argument for i386/x86_64 to do the same as arm64, but ...

> 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.

Ouch. But, yes, that would also make sense.

/
    Leif