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 <ag...@suse.de> > >> --- > >> 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
