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

[Daniel Kiper]

On Tue, Jan 22, 2019 at 04:36:39PM +0100, Alexander Graf wrote:
> On 15.01.19 14:40, Daniel Kiper wrote:
> > 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...
>
> I can't see a good way to make this work. All layers in mkimagexx are
> somewhat assuming that they own physical and virtual address space.
>
> I think I managed to hack things up to a point where I can have them
> disjoint, but it's not pretty. I also still have problems where
> relocations just won't work, because they are constructed in mkimagexx
> which has no visibility on virtual placement eventually.
>
> At this point, I would rather sacrifice 14kb rather than have an
> unmaintainable mess that is even worse than today's state of affairs.

Thank you for trying to do that. If it is difficult then make
GRUB_PE32_SECTION_ALIGNMENT equal to GRUB_PE32_FILE_ALIGNMENT. So, just
change GRUB_PE32_SECTION_ALIGNMENT value and the comment before it why
GRUB_PE32_SECTION_ALIGNMENT have to be equal GRUB_PE32_FILE_ALIGNMENT.
However, I think that the rest of my comments still stands.

Daniel