[PATCH v3 0/4] arm64: Support HP Envy X2

[PATCH v3 0/4] 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 and other efi targets. This way we
improve overall compatibility - there surely will be more devices coming with
similar constraints.

We also extend that alignemnt to any section that follows afterwards, to
ensure that other protection mechanisms will have a chance to protect
individual sections with page table bits.

This gets us into alignment with how the MS tools build UEFI applications,
so we should not run into compatibility problems about alignment going forward.

v1 -> v2:

  - Remove explicit device wording from patch
  - Use GRUB_EFI_PAGE_SIZE

v2 -> v3:

  - Apply alignment to all architectures
  - new patch: mkimage: Align efi sections on 4k boundary

Alexander Graf (4):
  mkimage: Simplify header size logic
  mkimage: Use EFI32_HEADER_SIZE define in arm-efi case
  mkimage: arm64-efi: Align header to page granularity
  mkimage: Align efi sections on 4k boundary

 util/mkimage.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

-- 
2.12.3

[PATCH v3 1/4] 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>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 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 v3 2/4] mkimage: Use EFI32_HEADER_SIZE define in arm-efi case

[Alexander Graf]

The efi-arm case was defining its own header size calculation, even though it's
100% identical to the common EFI32_HEADER_SIZE definition.

So let's clean it up to use the common define.

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

diff --git a/util/mkimage.c b/util/mkimage.c
index 88b991764..a670db456 100644
--- a/util/mkimage.c
+++ b/util/mkimage.c
@@ -602,12 +602,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 = 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),
-                                GRUB_PE32_SECTION_ALIGNMENT),
+      .vaddr_offset = EFI32_HEADER_SIZE,
       .pe_target = GRUB_PE32_MACHINE_ARMTHUMB_MIXED,
       .elf_target = EM_ARM,
     },
-- 
2.12.3

[PATCH v3 3/4] mkimage: arm64-efi: Align header to page granularity

[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

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)
 
 static const struct grub_install_image_target_desc image_targets[] =
   {
-- 
2.12.3

[PATCH v3 4/4] mkimage: Align efi sections on 4k boundary

[Alexander Graf]

There is UEFI firmware popping up in the wild now that implements stricter
permission checks using NX and write protect page table entry bits.

This means that firmware now may fail to load binaries if its individual
sections are not page aligned, as otherwise it can not ensure permission
boundaries.

So let's bump all efi section alignments up to 4k (EFI page size). That way
we will stay compatible going forward. This brings us into sync with what
the MS C compiler toolchain does.

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

diff --git a/util/mkimage.c b/util/mkimage.c
index 6b372cba5..a57f277ec 100644
--- a/util/mkimage.c
+++ b/util/mkimage.c
@@ -194,7 +194,7 @@ static const struct grub_install_image_target_desc image_targets[] =
       .decompressor_compressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_addr = TARGET_NO_FIELD,
-      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
+      .section_align = GRUB_EFI_PAGE_SIZE,
       .vaddr_offset = EFI32_HEADER_SIZE,
       .pe_target = GRUB_PE32_MACHINE_I386,
       .elf_target = EM_386,
@@ -244,7 +244,7 @@ static const struct grub_install_image_target_desc image_targets[] =
       .decompressor_compressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_addr = TARGET_NO_FIELD,
-      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
+      .section_align = GRUB_EFI_PAGE_SIZE,
       .vaddr_offset = EFI64_HEADER_SIZE,
       .pe_target = GRUB_PE32_MACHINE_X86_64,
       .elf_target = EM_X86_64,
@@ -421,7 +421,7 @@ static const struct grub_install_image_target_desc image_targets[] =
       .decompressor_compressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_addr = TARGET_NO_FIELD,
-      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
+      .section_align = GRUB_EFI_PAGE_SIZE,
       .vaddr_offset = EFI64_HEADER_SIZE,
       .pe_target = GRUB_PE32_MACHINE_IA64,
       .elf_target = EM_IA_64,
@@ -602,7 +602,7 @@ static const struct grub_install_image_target_desc image_targets[] =
       .decompressor_compressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_addr = TARGET_NO_FIELD,
-      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
+      .section_align = GRUB_EFI_PAGE_SIZE,
       .vaddr_offset = EFI32_HEADER_SIZE,
       .pe_target = GRUB_PE32_MACHINE_ARMTHUMB_MIXED,
       .elf_target = EM_ARM,
@@ -618,7 +618,7 @@ static const struct grub_install_image_target_desc image_targets[] =
       .decompressor_compressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_size = TARGET_NO_FIELD,
       .decompressor_uncompressed_addr = TARGET_NO_FIELD,
-      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
+      .section_align = GRUB_EFI_PAGE_SIZE,
       .vaddr_offset = EFI64_HEADER_SIZE,
       .pe_target = GRUB_PE32_MACHINE_ARM64,
       .elf_target = EM_AARCH64,
-- 
2.12.3

Re: [PATCH v3 1/4] mkimage: Simplify header size logic

[Daniel Kiper]

On Mon, Jan 14, 2019 at 04:27:15PM +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>
> ---
>  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;

After some thinking it seems to me that this patch is wrong and should
be dropped. It, at least, increases needlessly header size. More in the
comments for the other patches.

Daniel

Re: [PATCH v3 2/4] mkimage: Use EFI32_HEADER_SIZE define in arm-efi case

[Daniel Kiper]

On Mon, Jan 14, 2019 at 04:27:16PM +0100, Alexander Graf wrote:
> The efi-arm case was defining its own header size calculation, even though it's
> 100% identical to the common EFI32_HEADER_SIZE definition.
>
> So let's clean it up to use the common define.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  util/mkimage.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/util/mkimage.c b/util/mkimage.c
> index 88b991764..a670db456 100644
> --- a/util/mkimage.c
> +++ b/util/mkimage.c
> @@ -602,12 +602,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 = 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),
> -                                GRUB_PE32_SECTION_ALIGNMENT),
> +      .vaddr_offset = EFI32_HEADER_SIZE,
>        .pe_target = GRUB_PE32_MACHINE_ARMTHUMB_MIXED,
>        .elf_target = EM_ARM,
>      },

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

Daniel

Re: [PATCH v3 3/4] mkimage: arm64-efi: Align header to page granularity

[Daniel Kiper]

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

Re: [PATCH v3 4/4] mkimage: Align efi sections on 4k boundary

[Daniel Kiper]

On Mon, Jan 14, 2019 at 04:27:18PM +0100, Alexander Graf wrote:
> There is UEFI firmware popping up in the wild now that implements stricter
> permission checks using NX and write protect page table entry bits.
>
> This means that firmware now may fail to load binaries if its individual
> sections are not page aligned, as otherwise it can not ensure permission
> boundaries.
>
> So let's bump all efi section alignments up to 4k (EFI page size). That way
> we will stay compatible going forward. This brings us into sync with what
> the MS C compiler toolchain does.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  util/mkimage.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/util/mkimage.c b/util/mkimage.c
> index 6b372cba5..a57f277ec 100644
> --- a/util/mkimage.c
> +++ b/util/mkimage.c
> @@ -194,7 +194,7 @@ static const struct grub_install_image_target_desc image_targets[] =
>        .decompressor_compressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_addr = TARGET_NO_FIELD,
> -      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
> +      .section_align = GRUB_EFI_PAGE_SIZE,
>        .vaddr_offset = EFI32_HEADER_SIZE,
>        .pe_target = GRUB_PE32_MACHINE_I386,
>        .elf_target = EM_386,
> @@ -244,7 +244,7 @@ static const struct grub_install_image_target_desc image_targets[] =
>        .decompressor_compressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_addr = TARGET_NO_FIELD,
> -      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
> +      .section_align = GRUB_EFI_PAGE_SIZE,
>        .vaddr_offset = EFI64_HEADER_SIZE,
>        .pe_target = GRUB_PE32_MACHINE_X86_64,
>        .elf_target = EM_X86_64,
> @@ -421,7 +421,7 @@ static const struct grub_install_image_target_desc image_targets[] =
>        .decompressor_compressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_addr = TARGET_NO_FIELD,
> -      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
> +      .section_align = GRUB_EFI_PAGE_SIZE,
>        .vaddr_offset = EFI64_HEADER_SIZE,
>        .pe_target = GRUB_PE32_MACHINE_IA64,
>        .elf_target = EM_IA_64,
> @@ -602,7 +602,7 @@ static const struct grub_install_image_target_desc image_targets[] =
>        .decompressor_compressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_addr = TARGET_NO_FIELD,
> -      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
> +      .section_align = GRUB_EFI_PAGE_SIZE,
>        .vaddr_offset = EFI32_HEADER_SIZE,
>        .pe_target = GRUB_PE32_MACHINE_ARMTHUMB_MIXED,
>        .elf_target = EM_ARM,
> @@ -618,7 +618,7 @@ static const struct grub_install_image_target_desc image_targets[] =
>        .decompressor_compressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_size = TARGET_NO_FIELD,
>        .decompressor_uncompressed_addr = TARGET_NO_FIELD,
> -      .section_align = GRUB_PE32_SECTION_ALIGNMENT,
> +      .section_align = GRUB_EFI_PAGE_SIZE,
>        .vaddr_offset = EFI64_HEADER_SIZE,
>        .pe_target = GRUB_PE32_MACHINE_ARM64,
>        .elf_target = EM_AARCH64,

GRUB_PE32_SECTION_ALIGNMENT should be defined as GRUB_EFI_PAGE_SIZE.
Please explain in the comment before its definition why.

Daniel

Re: [PATCH v3 3/4] mkimage: arm64-efi: Align header to page granularity

[Alexander Graf]

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

Re: [PATCH v3 3/4] mkimage: arm64-efi: Align header to page granularity

[Daniel Kiper]

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

Re: [PATCH v3 3/4] mkimage: arm64-efi: Align header to page granularity

[Alexander Graf]

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

Re: [PATCH v3 3/4] mkimage: arm64-efi: Align header to page granularity

[Daniel Kiper]

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