[PATCH v2 0/2] arm64: Support HP Envy X2

[PATCH v2 0/2] arm64: Support HP Envy X2

[Alexander Graf]

I got a new toy this week: An HP Envy X2 system. This is one of those shiny
new Qualcomm Snapdragon based Windows tablet/notebook hybrid things.

While running Windows on those is actually not a terribly bad experience now
that WSL is out, I would like to see Linux run on those as well in the future.

Unfortunately as far as I'm aware so far nobody was able to run self built
binaries on the built-in UEFI version.

Turns out, it's a problem with aligning the start of the header to 4k. Once
we do that, binaries can be loaded just fine and run.

The reason behind that is simple: Its firmware tries to ensure NX protection
flags and can do so only when the code is 4K aligned.

So to maintain compatibility with that device, this patch set just bumps the
header alignment to 4K always on arm64-efi. This way we improve overall
compatibility - there surely will be more devices coming with similar
constraints.

v1 -> v2:

  - Remove explicit device wording from patch
  - Use GRUB_EFI_PAGE_SIZE

Alexander Graf (2):
  mkimage: Simplify header size logic
  mkimage: arm64-efi: Align first section to page

 util/mkimage.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

-- 
2.12.3

[PATCH v2 1/2] mkimage: Simplify header size logic

[Alexander Graf]

For EFI images, we always have the following layout:

  [PE header]
  [padding]
  [first section (which also is the entry point)]

Currently there are 2 places where we define how big header+padding are:
in the .vaddr_offset member of our target image definition struct as well
as in code in grub_install_generate_image().

Remove the latter, so that we only have a single place to modify if we
need to change the padding.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 util/mkimage.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/util/mkimage.c b/util/mkimage.c
index 353bb1098..88b991764 100644
--- a/util/mkimage.c
+++ b/util/mkimage.c
@@ -1226,10 +1226,7 @@ grub_install_generate_image (const char *dir, const char *prefix,
 	int header_size;
 	int reloc_addr;
 
-	if (image_target->voidp_sizeof == 4)
-	  header_size = EFI32_HEADER_SIZE;
-	else
-	  header_size = EFI64_HEADER_SIZE;
+	header_size = image_target->vaddr_offset;
 
 	reloc_addr = ALIGN_UP (header_size + core_size,
 			       image_target->section_align);
-- 
2.12.3

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

[Alexander Graf]

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,
       .pe_target = GRUB_PE32_MACHINE_ARM64,
       .elf_target = EM_AARCH64,
     },
-- 
2.12.3

Re: [PATCH v2 0/2] arm64: Support HP Envy X2

[Leif Lindholm]

On Sun, Dec 23, 2018 at 03:52:05AM +0100, Alexander Graf wrote:
> I got a new toy this week: An HP Envy X2 system. This is one of those shiny
> new Qualcomm Snapdragon based Windows tablet/notebook hybrid things.
> 
> While running Windows on those is actually not a terribly bad experience now
> that WSL is out, I would like to see Linux run on those as well in the future.
> 
> Unfortunately as far as I'm aware so far nobody was able to run self built
> binaries on the built-in UEFI version.
> 
> Turns out, it's a problem with aligning the start of the header to 4k. Once
> we do that, binaries can be loaded just fine and run.
> 
> The reason behind that is simple: Its firmware tries to ensure NX protection
> flags and can do so only when the code is 4K aligned.
> 
> So to maintain compatibility with that device, this patch set just bumps the
> header alignment to 4K always on arm64-efi. This way we improve overall
> compatibility - there surely will be more devices coming with similar
> constraints.
> 
> v1 -> v2:
> 
>   - Remove explicit device wording from patch
>   - Use GRUB_EFI_PAGE_SIZE

Looks good to me, thanks!
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Tested-by: Leif Lindholm <leif.lindholm@linaro.org>

> Alexander Graf (2):
>   mkimage: Simplify header size logic
>   mkimage: arm64-efi: Align first section to page
> 
>  util/mkimage.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> -- 
> 2.12.3
> 

Re: [PATCH v2 1/2] mkimage: Simplify header size logic

[Daniel Kiper]

On Sun, Dec 23, 2018 at 03:52:06AM +0100, Alexander Graf wrote:
> For EFI images, we always have the following layout:
>
>   [PE header]
>   [padding]
>   [first section (which also is the entry point)]
>
> Currently there are 2 places where we define how big header+padding are:
> in the .vaddr_offset member of our target image definition struct as well
> as in code in grub_install_generate_image().
>
> Remove the latter, so that we only have a single place to modify if we
> need to change the padding.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

[Daniel Kiper]

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

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

[Alexander Graf]

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

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

[Leif Lindholm]

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