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

[Daniel Kiper]

On Mon, Jan 28, 2019 at 01:33:33PM +0100, Alexander Graf wrote:
> On 28.01.19 13:22, Daniel Kiper wrote:
> > On Fri, Jan 25, 2019 at 12:45:15PM +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>
> >>
> >> ---
> >>
> >> v4 -> v5:
> >>
> >>   - Use GRUB_EFI_PAGE_SIZE
> >>   - Add include to have above const defined
> >> ---
> >>  include/grub/efi/pe32.h | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> >> index 7d44732d2..fe8a85ce6 100644
> >> --- a/include/grub/efi/pe32.h
> >> +++ b/include/grub/efi/pe32.h
> >> @@ -20,6 +20,7 @@
> >>  #define GRUB_EFI_PE32_HEADER      1
> >>
> >>  #include <grub/types.h>
> >> +#include <grub/efi/memory.h>
> >>
> >>  /* The MSDOS compatibility stub. This was copied from the output of
> >>     objcopy, and it is not necessary to care about what this means.  */
> >> @@ -50,8 +51,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/ By chance GRUB_EFI_PAGE_SIZE is equal to 4k but
> > there is no requirement for that...
>
> There is, it's defined in the spec.

It is for currently existing platforms. There is no guarantee that it will
be the same for new ones. So, I tend to change it to GRUB_EFI_PAGE_SIZE.
Even if today GRUB_EFI_PAGE_SIZE always equals 4k.

> >> +#define GRUB_PE32_SECTION_ALIGNMENT       GRUB_EFI_PAGE_SIZE
> >
> > I am still missing an explanation here why GRUB_PE32_FILE_ALIGNMENT has
> > to be equal GRUB_PE32_SECTION_ALIGNMENT. And IIRC I have asked about
> > that in my earlier emails...
>
> It is in the comment above exactly those 2 lines. What more do you want?

Ahhh... Sorry, I have missed that. Though I would change
s/Because we/Because currently existing GRUB code/

Daniel