Re: [PATCH v3 3/4] mkimage: arm64-efi: Align header to page granularity

[Daniel Kiper]

On Tue, Jan 15, 2019 at 01:52:41PM +0100, Alexander Graf wrote:
> On 01/15/2019 01:45 PM, Daniel Kiper wrote:
> > On Mon, Jan 14, 2019 at 04:27:17PM +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
> > >
> > > v2 -> v3:
> > >
> > >    - Apply alignment to all architectures
> > > ---
> > >   util/mkimage.c | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/util/mkimage.c b/util/mkimage.c
> > > index a670db456..6b372cba5 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>
> > > @@ -66,14 +67,14 @@
> > >                                       + sizeof (struct 
> > > grub_pe32_coff_header) \
> > >                                       + sizeof (struct 
> > > grub_pe32_optional_header) \
> > >                                       + 4 * sizeof (struct 
> > > grub_pe32_section_table), \
> > > -                             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), \
> > > -                             GRUB_PE32_SECTION_ALIGNMENT)
> > > +                             GRUB_EFI_PAGE_SIZE)
> > GRUB_PE32_SECTION_ALIGNMENT should be changed to GRUB_PE32_FILE_ALIGNMENT.
> > However, earlier GRUB_PE32_FILE_ALIGNMENT should be defined as 0x20. Yeah,
> > I know that this is contrary to the PE/COFF spec. Though everybody uses
> > that value. Even MS...
>
> I'm not sure I follow. When compiling code with MSVC, every section is
> definitely page aligned?
>
> > Then below in the util/mkimage.c some code should look more or less like 
> > that:
> >
> >          reloc_addr = ALIGN_UP (header_size + core_size,
> >                                 GRUB_PE32_FILE_ALIGNMENT);
> >
> >          pe_size = ALIGN_UP (reloc_addr + layout.reloc_size,
> >                              GRUB_PE32_FILE_ALIGNMENT);
> >
> > ...and...
> >
> >          o->file_alignment = grub_host_to_target32 
> > (GRUB_PE32_FILE_ALIGNMENT);
> >
> > Last line should be changed at least in two places.
>
> I again don't think I fully follow your thought process here yet. Can you
> please enlighten me?

There are two alignments in PE/COFF file: FileAlignment and
SectionAlignment. You can see both of them using "objdump -x"
or "readpe". FileAlignment enforces PE/COFF data/sections/etc.
alignment in a file. This should be quite small. SectionAlignment
enforces PE/COFF data/sections/etc. in memory. This should be
GRUB_EFI_PAGE_SIZE. Both are not related and can be different.
And I think that we should try to use both FileAlignment and
SectionAlignment properly. If it is not possible then we can
make them equal but then in the worst case we will have extra
~14 KiB of zeros in PE GRUB image. Not much for today but why
carry this garbage...

Daniel