[PATCH v5 0/3] arm64: Support HP Envy X2

[PATCH v5 0/3] arm64: Support HP Envy X2

[Alexander Graf]

I got a new toy recently: 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 all
PE alignments to 4K always on all efi targets. This way we improve overall
compatibility - there surely will be more devices coming with similar
constraints.

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

v3 -> v4:

  - Reduce everything down to 1 patch which just adapts *all* alignment
    to GRUB_EFI_PAGE_SIZE (4k).

v4 -> v5:

  - Use GRUB_EFI_PAGE_SIZE
  - Add include to have above const defined
  - new patch: mkimage: Clarify file alignment in efi case

Alexander Graf (3):
  mkimage: Use EFI32_HEADER_SIZE define in arm-efi case
  mkimage: Align efi sections on 4k boundary
  mkimage: Clarify file alignment in efi case

 include/grub/efi/pe32.h | 10 ++++++++--
 util/mkimage.c          | 15 +++++----------
 2 files changed, 13 insertions(+), 12 deletions(-)

-- 
2.12.3

[PATCH v5 1/3] 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 353bb1098..16af12e0c 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

Re: [PATCH v5 1/3] mkimage: Use EFI32_HEADER_SIZE define in arm-efi case

[Daniel Kiper]

On Fri, Jan 25, 2019 at 12:45:14PM +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>

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

Daniel

[PATCH v5 2/3] 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.

Unfortunately our internals can't deal very well with a mismatch of alignment
between the virtual and file offsets, so we have to also pad our target
binary a bit.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v4 -> v5:

  - Use GRUB_EFI_PAGE_SIZE
  - Add include to have above const defined
---
 include/grub/efi/pe32.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
index 7d44732d2..fe8a85ce6 100644
--- a/include/grub/efi/pe32.h
+++ b/include/grub/efi/pe32.h
@@ -20,6 +20,7 @@
 #define GRUB_EFI_PE32_HEADER	1
 
 #include <grub/types.h>
+#include <grub/efi/memory.h>
 
 /* The MSDOS compatibility stub. This was copied from the output of
    objcopy, and it is not necessary to care about what this means.  */
@@ -50,8 +51,13 @@
 /* According to the spec, the minimal alignment is 512 bytes...
    But some examples (such as EFI drivers in the Intel
    Sample Implementation) use 32 bytes (0x20) instead, and it seems
-   to be working. For now, GRUB uses 512 bytes for safety.  */
-#define GRUB_PE32_SECTION_ALIGNMENT	0x200
+   to be working.
+
+   However, there is firmware showing up in the field now with
+   page alignment constraints to guarantee that page protection
+   bits take effect. Because we can not easily distinguish between
+   in-memory and in-file layout, let's bump all alignment to 4k. */
+#define GRUB_PE32_SECTION_ALIGNMENT	GRUB_EFI_PAGE_SIZE
 #define GRUB_PE32_FILE_ALIGNMENT	GRUB_PE32_SECTION_ALIGNMENT
 
 struct grub_pe32_coff_header
-- 
2.12.3

Re: [PATCH v5 2/3] mkimage: Align efi sections on 4k boundary

[Daniel Kiper]

On Fri, Jan 25, 2019 at 12:45:15PM +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.
>
> Unfortunately our internals can't deal very well with a mismatch of alignment
> between the virtual and file offsets, so we have to also pad our target
> binary a bit.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> v4 -> v5:
>
>   - Use GRUB_EFI_PAGE_SIZE
>   - Add include to have above const defined
> ---
>  include/grub/efi/pe32.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> index 7d44732d2..fe8a85ce6 100644
> --- a/include/grub/efi/pe32.h
> +++ b/include/grub/efi/pe32.h
> @@ -20,6 +20,7 @@
>  #define GRUB_EFI_PE32_HEADER	1
>
>  #include <grub/types.h>
> +#include <grub/efi/memory.h>
>
>  /* The MSDOS compatibility stub. This was copied from the output of
>     objcopy, and it is not necessary to care about what this means.  */
> @@ -50,8 +51,13 @@
>  /* According to the spec, the minimal alignment is 512 bytes...
>     But some examples (such as EFI drivers in the Intel
>     Sample Implementation) use 32 bytes (0x20) instead, and it seems
> -   to be working. For now, GRUB uses 512 bytes for safety.  */
> -#define GRUB_PE32_SECTION_ALIGNMENT	0x200
> +   to be working.
> +
> +   However, there is firmware showing up in the field now with
> +   page alignment constraints to guarantee that page protection
> +   bits take effect. Because we can not easily distinguish between
> +   in-memory and in-file layout, let's bump all alignment to 4k. */

s/4k/GRUB_EFI_PAGE_SIZE/ By chance GRUB_EFI_PAGE_SIZE is equal to 4k but
there is no requirement for that...

> +#define GRUB_PE32_SECTION_ALIGNMENT	GRUB_EFI_PAGE_SIZE

I am still missing an explanation here why GRUB_PE32_FILE_ALIGNMENT has
to be equal GRUB_PE32_SECTION_ALIGNMENT. And IIRC I have asked about
that in my earlier emails...

>  #define GRUB_PE32_FILE_ALIGNMENT	GRUB_PE32_SECTION_ALIGNMENT

Daniel

Re: [PATCH v5 2/3] mkimage: Align efi sections on 4k boundary

[Alexander Graf]

On 28.01.19 13:22, Daniel Kiper wrote:
> On Fri, Jan 25, 2019 at 12:45:15PM +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.
>>
>> Unfortunately our internals can't deal very well with a mismatch of alignment
>> between the virtual and file offsets, so we have to also pad our target
>> binary a bit.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> ---
>>
>> v4 -> v5:
>>
>>   - Use GRUB_EFI_PAGE_SIZE
>>   - Add include to have above const defined
>> ---
>>  include/grub/efi/pe32.h | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
>> index 7d44732d2..fe8a85ce6 100644
>> --- a/include/grub/efi/pe32.h
>> +++ b/include/grub/efi/pe32.h
>> @@ -20,6 +20,7 @@
>>  #define GRUB_EFI_PE32_HEADER	1
>>
>>  #include <grub/types.h>
>> +#include <grub/efi/memory.h>
>>
>>  /* The MSDOS compatibility stub. This was copied from the output of
>>     objcopy, and it is not necessary to care about what this means.  */
>> @@ -50,8 +51,13 @@
>>  /* According to the spec, the minimal alignment is 512 bytes...
>>     But some examples (such as EFI drivers in the Intel
>>     Sample Implementation) use 32 bytes (0x20) instead, and it seems
>> -   to be working. For now, GRUB uses 512 bytes for safety.  */
>> -#define GRUB_PE32_SECTION_ALIGNMENT	0x200
>> +   to be working.
>> +
>> +   However, there is firmware showing up in the field now with
>> +   page alignment constraints to guarantee that page protection
>> +   bits take effect. Because we can not easily distinguish between
>> +   in-memory and in-file layout, let's bump all alignment to 4k. */
> 
> s/4k/GRUB_EFI_PAGE_SIZE/ By chance GRUB_EFI_PAGE_SIZE is equal to 4k but
> there is no requirement for that...

There is, it's defined in the spec.

> 
>> +#define GRUB_PE32_SECTION_ALIGNMENT	GRUB_EFI_PAGE_SIZE
> 
> I am still missing an explanation here why GRUB_PE32_FILE_ALIGNMENT has
> to be equal GRUB_PE32_SECTION_ALIGNMENT. And IIRC I have asked about
> that in my earlier emails...

It is in the comment above exactly those 2 lines. What more do you want?


Alex

Re: [PATCH v5 2/3] mkimage: Align efi sections on 4k boundary

[Daniel Kiper]

On Mon, Jan 28, 2019 at 01:33:33PM +0100, Alexander Graf wrote:
> On 28.01.19 13:22, Daniel Kiper wrote:
> > On Fri, Jan 25, 2019 at 12:45:15PM +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.
> >>
> >> Unfortunately our internals can't deal very well with a mismatch of alignment
> >> between the virtual and file offsets, so we have to also pad our target
> >> binary a bit.
> >>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>
> >> ---
> >>
> >> v4 -> v5:
> >>
> >>   - Use GRUB_EFI_PAGE_SIZE
> >>   - Add include to have above const defined
> >> ---
> >>  include/grub/efi/pe32.h | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> >> index 7d44732d2..fe8a85ce6 100644
> >> --- a/include/grub/efi/pe32.h
> >> +++ b/include/grub/efi/pe32.h
> >> @@ -20,6 +20,7 @@
> >>  #define GRUB_EFI_PE32_HEADER	1
> >>
> >>  #include <grub/types.h>
> >> +#include <grub/efi/memory.h>
> >>
> >>  /* The MSDOS compatibility stub. This was copied from the output of
> >>     objcopy, and it is not necessary to care about what this means.  */
> >> @@ -50,8 +51,13 @@
> >>  /* According to the spec, the minimal alignment is 512 bytes...
> >>     But some examples (such as EFI drivers in the Intel
> >>     Sample Implementation) use 32 bytes (0x20) instead, and it seems
> >> -   to be working. For now, GRUB uses 512 bytes for safety.  */
> >> -#define GRUB_PE32_SECTION_ALIGNMENT	0x200
> >> +   to be working.
> >> +
> >> +   However, there is firmware showing up in the field now with
> >> +   page alignment constraints to guarantee that page protection
> >> +   bits take effect. Because we can not easily distinguish between
> >> +   in-memory and in-file layout, let's bump all alignment to 4k. */
> >
> > s/4k/GRUB_EFI_PAGE_SIZE/ By chance GRUB_EFI_PAGE_SIZE is equal to 4k but
> > there is no requirement for that...
>
> There is, it's defined in the spec.

It is for currently existing platforms. There is no guarantee that it will
be the same for new ones. So, I tend to change it to GRUB_EFI_PAGE_SIZE.
Even if today GRUB_EFI_PAGE_SIZE always equals 4k.

> >> +#define GRUB_PE32_SECTION_ALIGNMENT	GRUB_EFI_PAGE_SIZE
> >
> > I am still missing an explanation here why GRUB_PE32_FILE_ALIGNMENT has
> > to be equal GRUB_PE32_SECTION_ALIGNMENT. And IIRC I have asked about
> > that in my earlier emails...
>
> It is in the comment above exactly those 2 lines. What more do you want?

Ahhh... Sorry, I have missed that. Though I would change
s/Because we/Because currently existing GRUB code/

Daniel

[PATCH v5 3/3] mkimage: Clarify file alignment in efi case

[Alexander Graf]

There are a few spots in the PE generation code for EFI binaries that uses
the section alignment rather than file alignment, even though the alignment
is really only file bound.

Replace those cases with the file alignment constant instead.

Reported-by: Daniel Kiper <dkiper@net-space.pl>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 util/mkimage.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/mkimage.c b/util/mkimage.c
index 16af12e0c..6631bfe45 100644
--- a/util/mkimage.c
+++ b/util/mkimage.c
@@ -1227,10 +1227,10 @@ grub_install_generate_image (const char *dir, const char *prefix,
 	  header_size = EFI64_HEADER_SIZE;
 
 	reloc_addr = ALIGN_UP (header_size + core_size,
-			       image_target->section_align);
+			       GRUB_PE32_FILE_ALIGNMENT);
 
 	pe_size = ALIGN_UP (reloc_addr + layout.reloc_size,
-			    image_target->section_align);
+			    GRUB_PE32_FILE_ALIGNMENT);
 	pe_img = xmalloc (reloc_addr + layout.reloc_size);
 	memset (pe_img, 0, header_size);
 	memcpy ((char *) pe_img + header_size, core_img, core_size);
@@ -1280,7 +1280,7 @@ grub_install_generate_image (const char *dir, const char *prefix,
 
 	    o->image_base = 0;
 	    o->section_alignment = grub_host_to_target32 (image_target->section_align);
-	    o->file_alignment = grub_host_to_target32 (image_target->section_align);
+	    o->file_alignment = grub_host_to_target32 (GRUB_PE32_FILE_ALIGNMENT);
 	    o->image_size = grub_host_to_target32 (pe_size);
 	    o->header_size = grub_host_to_target32 (header_size);
 	    o->subsystem = grub_host_to_target16 (GRUB_PE32_SUBSYSTEM_EFI_APPLICATION);
@@ -1315,7 +1315,7 @@ grub_install_generate_image (const char *dir, const char *prefix,
 	    o->code_base = grub_cpu_to_le32 (header_size);
 	    o->image_base = 0;
 	    o->section_alignment = grub_host_to_target32 (image_target->section_align);
-	    o->file_alignment = grub_host_to_target32 (image_target->section_align);
+	    o->file_alignment = grub_host_to_target32 (GRUB_PE32_FILE_ALIGNMENT);
 	    o->image_size = grub_host_to_target32 (pe_size);
 	    o->header_size = grub_host_to_target32 (header_size);
 	    o->subsystem = grub_host_to_target16 (GRUB_PE32_SUBSYSTEM_EFI_APPLICATION);
-- 
2.12.3

Re: [PATCH v5 3/3] mkimage: Clarify file alignment in efi case

[Daniel Kiper]

On Fri, Jan 25, 2019 at 12:45:16PM +0100, Alexander Graf wrote:
> There are a few spots in the PE generation code for EFI binaries that uses
> the section alignment rather than file alignment, even though the alignment
> is really only file bound.
>
> Replace those cases with the file alignment constant instead.
>
> Reported-by: Daniel Kiper <dkiper@net-space.pl>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Great! However, this patch misses changes for EFI32_HEADER_SIZE
and EFI64_HEADER_SIZE macros. In general I think about
s/GRUB_PE32_SECTION_ALIGNMENT/GRUB_PE32_FILE_ALIGNMENT/
I have asked about that in my earlier emails too...

Daniel

Re: [PATCH v5 3/3] mkimage: Clarify file alignment in efi case

[Alexander Graf]

On 28.01.19 13:27, Daniel Kiper wrote:
> On Fri, Jan 25, 2019 at 12:45:16PM +0100, Alexander Graf wrote:
>> There are a few spots in the PE generation code for EFI binaries that uses
>> the section alignment rather than file alignment, even though the alignment
>> is really only file bound.
>>
>> Replace those cases with the file alignment constant instead.
>>
>> Reported-by: Daniel Kiper <dkiper@net-space.pl>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> Great! However, this patch misses changes for EFI32_HEADER_SIZE
> and EFI64_HEADER_SIZE macros. In general I think about
> s/GRUB_PE32_SECTION_ALIGNMENT/GRUB_PE32_FILE_ALIGNMENT/
> I have asked about that in my earlier emails too...

If you have such a strong opinion, why don't you just simply do the
patch and I review it? The way we're bouncing this back and forth is
unproductive for both of us.


Alex

Re: [PATCH v5 3/3] mkimage: Clarify file alignment in efi case

[Daniel Kiper]

On Mon, Jan 28, 2019 at 01:34:20PM +0100, Alexander Graf wrote:
> On 28.01.19 13:27, Daniel Kiper wrote:
> > On Fri, Jan 25, 2019 at 12:45:16PM +0100, Alexander Graf wrote:
> >> There are a few spots in the PE generation code for EFI binaries that uses
> >> the section alignment rather than file alignment, even though the alignment
> >> is really only file bound.
> >>
> >> Replace those cases with the file alignment constant instead.
> >>
> >> Reported-by: Daniel Kiper <dkiper@net-space.pl>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >
> > Great! However, this patch misses changes for EFI32_HEADER_SIZE
> > and EFI64_HEADER_SIZE macros. In general I think about
> > s/GRUB_PE32_SECTION_ALIGNMENT/GRUB_PE32_FILE_ALIGNMENT/
> > I have asked about that in my earlier emails too...
>
> If you have such a strong opinion, why don't you just simply do the
> patch and I review it? The way we're bouncing this back and forth is
> unproductive for both of us.

Yes, I agree. However, I am asking you about that for quite long time.
You do not object and you do not take into account this comment. So,
I assume that you are missing it. If you would do one of above things
earlier I would not chase you up until now. Just please read my comments
more carefully.

Daniel