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