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

[Alexander Graf]

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.


Alex