diff mbox series

[v2,2/2] mkimage: arm64-efi: Align first section to page

Message ID 20181223025207.40755-3-agraf@suse.de
State New
Headers show
Series arm64: Support HP Envy X2 | expand

Commit Message

Alexander Graf Dec. 23, 2018, 2:52 a.m. UTC
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

Comments

Daniel Kiper Jan. 14, 2019, 1:37 p.m. UTC | #1
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
Alexander Graf Jan. 14, 2019, 1:41 p.m. UTC | #2
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
Leif Lindholm Jan. 14, 2019, 2:21 p.m. UTC | #3
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 mbox series

Patch

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,
     },