| Message ID | 20181223025207.40755-3-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Support HP Envy X2 | expand |
On Sun, Dec 23, 2018 at 03:52:07AM +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 > --- > util/mkimage.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/util/mkimage.c b/util/mkimage.c > index 88b991764..de93c5a13 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> > @@ -623,7 +624,7 @@ static const struct grub_install_image_target_desc image_targets[] = > .decompressor_uncompressed_size = TARGET_NO_FIELD, > .decompressor_uncompressed_addr = TARGET_NO_FIELD, > .section_align = GRUB_PE32_SECTION_ALIGNMENT, > - .vaddr_offset = EFI64_HEADER_SIZE, > + .vaddr_offset = GRUB_EFI_PAGE_SIZE, Nack. I think that we will soon have the same problem on other targtes. So, I would try this: #define EFI32_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE \ + GRUB_PE32_SIGNATURE_SIZE \ + sizeof (struct grub_pe32_coff_header) \ + sizeof (struct grub_pe32_optional_header) \ + 4 * sizeof (struct grub_pe32_section_table), \ ALIGN_UP (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), \ ALIGN_UP (GRUB_PE32_SECTION_ALIGNMENT, GRUB_EFI_PAGE_SIZE)) And there is another problem with your proposal. What will happen if EFI64_HEADER_SIZE > GRUB_EFI_PAGE_SIZE? Additionally, why arm-efi is different? Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On 01/14/2019 02:37 PM, Daniel Kiper wrote: > On Sun, Dec 23, 2018 at 03:52:07AM +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 >> --- >> util/mkimage.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/util/mkimage.c b/util/mkimage.c >> index 88b991764..de93c5a13 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> >> @@ -623,7 +624,7 @@ static const struct grub_install_image_target_desc image_targets[] = >> .decompressor_uncompressed_size = TARGET_NO_FIELD, >> .decompressor_uncompressed_addr = TARGET_NO_FIELD, >> .section_align = GRUB_PE32_SECTION_ALIGNMENT, >> - .vaddr_offset = EFI64_HEADER_SIZE, >> + .vaddr_offset = GRUB_EFI_PAGE_SIZE, > Nack. > > I think that we will soon have the same problem on other targtes. > So, I would try this: > > #define EFI32_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE \ > + GRUB_PE32_SIGNATURE_SIZE \ > + sizeof (struct grub_pe32_coff_header) \ > + sizeof (struct grub_pe32_optional_header) \ > + 4 * sizeof (struct grub_pe32_section_table), \ > ALIGN_UP (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), \ > ALIGN_UP (GRUB_PE32_SECTION_ALIGNMENT, GRUB_EFI_PAGE_SIZE)) > > And there is another problem with your proposal. What will happen > if EFI64_HEADER_SIZE > GRUB_EFI_PAGE_SIZE? I don't think it ever can be, can it? The header size is pretty constant. > Additionally, why arm-efi is different? Mostly because in the wild I have not seen anyone on non-aarch64 pull in page alignment requirements :). But yes, I'll be happy to redo the patch and make EFI binaries always 4k aligned. I also learned in an off-list mailing list thread, that newer versions of that Qcom firmware require 4k section alignments, so I will push for that across all targets too. That way we should be aligned with the MS compiler suite. Alex _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Mon, Jan 14, 2019 at 02:41:30PM +0100, Alexander Graf wrote: > On 01/14/2019 02:37 PM, Daniel Kiper wrote: > > On Sun, Dec 23, 2018 at 03:52:07AM +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 > > > --- > > > util/mkimage.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/util/mkimage.c b/util/mkimage.c > > > index 88b991764..de93c5a13 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> > > > @@ -623,7 +624,7 @@ static const struct grub_install_image_target_desc image_targets[] = > > > .decompressor_uncompressed_size = TARGET_NO_FIELD, > > > .decompressor_uncompressed_addr = TARGET_NO_FIELD, > > > .section_align = GRUB_PE32_SECTION_ALIGNMENT, > > > - .vaddr_offset = EFI64_HEADER_SIZE, > > > + .vaddr_offset = GRUB_EFI_PAGE_SIZE, > > Nack. > > > > I think that we will soon have the same problem on other targtes. > > So, I would try this: > > > > #define EFI32_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE \ > > + GRUB_PE32_SIGNATURE_SIZE \ > > + sizeof (struct grub_pe32_coff_header) \ > > + sizeof (struct grub_pe32_optional_header) \ > > + 4 * sizeof (struct grub_pe32_section_table), \ > > ALIGN_UP (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), \ > > ALIGN_UP (GRUB_PE32_SECTION_ALIGNMENT, GRUB_EFI_PAGE_SIZE)) > > > > And there is another problem with your proposal. What will happen > > if EFI64_HEADER_SIZE > GRUB_EFI_PAGE_SIZE? > > I don't think it ever can be, can it? The header size is pretty constant. > > > Additionally, why arm-efi is different? > > Mostly because in the wild I have not seen anyone on non-aarch64 pull in > page alignment requirements :). arm-efi is mainly different in that I don't expect non-embedded 32-bit devices to show up. There would be nothing wrong in applying the same change. There is an argument for i386/x86_64 to do the same as arm64, but ... > But yes, I'll be happy to redo the patch and make EFI binaries always 4k > aligned. I also learned in an off-list mailing list thread, that newer > versions of that Qcom firmware require 4k section alignments, so I will push > for that across all targets too. That way we should be aligned with the MS > compiler suite. Ouch. But, yes, that would also make sense. / Leif _______________________________________________ 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 88b991764..de93c5a13 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> @@ -623,7 +624,7 @@ static const struct grub_install_image_target_desc image_targets[] = .decompressor_uncompressed_size = TARGET_NO_FIELD, .decompressor_uncompressed_addr = TARGET_NO_FIELD, .section_align = GRUB_PE32_SECTION_ALIGNMENT, - .vaddr_offset = EFI64_HEADER_SIZE, + .vaddr_offset = GRUB_EFI_PAGE_SIZE, .pe_target = GRUB_PE32_MACHINE_ARM64, .elf_target = EM_AARCH64, },
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 --- util/mkimage.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- 2.12.3 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel