Re: [PATCH v4 2/2] mkimage: Align efi sections on 4k boundary

[Daniel Kiper]

On Thu, Jan 24, 2019 at 03:10:58PM +0100, Alexander Graf wrote:
> On 24.01.19 15:08, Daniel Kiper wrote:
> > On Wed, Jan 23, 2019 at 04:34:58PM +0100, Alexander Graf wrote:
> >> There is UEFI firmware popping up in the wild now that implements stricter
> >> permission checks using NX and write protect page table entry bits.
> >>
> >> This means that firmware now may fail to load binaries if its individual
> >> sections are not page aligned, as otherwise it can not ensure permission
> >> boundaries.
> >>
> >> So let's bump all efi section alignments up to 4k (EFI page size). That way
> >> we will stay compatible going forward.
> >>
> >> Unfortunately our internals can't deal very well with a mismatch of 
> >> alignment
> >> between the virtual and file offsets, so we have to also pad our target
> >> binary a bit.
> >>
> >> Signed-off-by: Alexander Graf <address@hidden>
> >> ---
> >>  include/grub/efi/pe32.h | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> >> index 7d44732d2..52ff208c0 100644
> >> --- a/include/grub/efi/pe32.h
> >> +++ b/include/grub/efi/pe32.h
> >> @@ -50,8 +50,13 @@
> >>  /* According to the spec, the minimal alignment is 512 bytes...
> >>     But some examples (such as EFI drivers in the Intel
> >>     Sample Implementation) use 32 bytes (0x20) instead, and it seems
> >> -   to be working. For now, GRUB uses 512 bytes for safety.  */
> >> -#define GRUB_PE32_SECTION_ALIGNMENT       0x200
> >> +   to be working.
> >> +
> >> +   However, there is firmware showing up in the field now with
> >> +   page alignment constraints to guarantee that page protection
> >> +   bits take effect. Because we can not easily distinguish between
> >> +   in-memory and in-file layout, let's bump all alignment to 4k. */
> >
> > s/4k/GRUB_EFI_PAGE_SIZE/
>
> Are you sure you want an
>
>   #include <efi/memory.h>
>
> in this file? I omitted it on purpose ;).

OK, probably this is not perfect but if nothing falls a part I would
include it in this file. If something breaks I would explain in the
comment why we hardcode this value here.

> >> +#define GRUB_PE32_SECTION_ALIGNMENT       0x1000
> >
> > I think that this should be:
> >
> > #define GRUB_PE32_SECTION_ALIGNMENT GRUB_EFI_PAGE_SIZE
> >
> > And I still miss two patches. One replacing GRUB_PE32_SECTION_ALIGNMENT
> > with GRUB_PE32_FILE_ALIGNMENT in EFI32_HEADER_SIZE and EFI64_HEADER_SIZE
> > macros.
> >
> > And another for some code in the util/mkimage.c...
> >
> >         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);
> >
> > ...plus...
> >
> >         o->file_alignment = grub_host_to_target32 
> > (GRUB_PE32_FILE_ALIGNMENT);
>
> As mentioned above, these *have* to be identical today, because
> otherwise all other assumptions in the code just fall apart. I see
> little point in playing that they may be unrelated.

GRUB_PE32_FILE_ALIGNMENT is equal to GRUB_PE32_SECTION_ALIGNMENT (well,
I think that it also makes sense to explain before GRUB_PE32_FILE_ALIGNMENT
definition why we should keep it equal to GRUB_PE32_SECTION_ALIGNMENT)
hence nothing breaks. However, IMO after these changes the code looks
more logical and is less confusing.

Daniel