| Message ID | 20190114152718.60629-4-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Support HP Envy X2 | expand |
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 <agraf@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... 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. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 <agraf@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? Alex _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 <agraf@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... Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 <agraf@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. Alex _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
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 <agraf@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
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) static const struct grub_install_image_target_desc image_targets[] = {
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 <agraf@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(-) -- 2.12.3 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel