[PATCH v2 0/2] Fix fallout from 4k section alignment patch

[PATCH v2 0/2] Fix fallout from 4k section alignment patch

[Alexander Graf]

Commit a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
broke ARM builds. There were 2 reasons for that:

  1) An NX bug that was lingering forever in the code base and only got
triggered because of the change

  2) A missing adjustment in the original patch

This patch set fixes both issues, making 32bit ARM targets for UEFI
fully functional again for me.

Alex

Alexander Graf (2):
  arm: Move trampolines into code section
  arm: Align section alignment with manual relocation offset code

 util/grub-mkimagexx.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

-- 
2.16.4

[PATCH v2 1/2] arm: Move trampolines into code section

[Alexander Graf]

When creating T32->A32 transition jumps, the relocation code in grub
will generate trampolines. These trampolines live in the .data section
of our PE binary which means they are not marked as executable.

This misbehavior was unmasked by commit a51f953f4ee87 ("mkimage: Align
efi sections on 4k boundary") which made the X/NX boundary more obvious
because everything became page aligned.

To put things into proper order, let's move the arm trampolines into the
.text section instead. That way everyone knows they are executable.

Fixes: a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
Reported-by: Julien ROBIN <julien.robin28@free.fr>
Reported-by: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Alexander Graf <agraf@csgraf.de>
Tested-by: Julien ROBIN <julien.robin28@free.fr>

---

v1 -> v2:

  - Take trampoline code size into account when calculating offsets
---
 util/grub-mkimagexx.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index a79034e7b..470fbf4dd 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -2197,25 +2197,10 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
 	  }
       }
 
-  layout->kernel_size = ALIGN_UP (layout->kernel_size + image_target->vaddr_offset,
-			      image_target->section_align)
-    - image_target->vaddr_offset;
-  layout->exec_size = layout->kernel_size;
-
-  /* .data */
-  for (i = 0, s = smd->sections;
-       i < smd->num_sections;
-       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
-    if (SUFFIX (is_data_section) (s, image_target))
-      layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, smd,
-						  image_target);
-
 #ifdef MKIMAGE_ELF32
   if (image_target->elf_target == EM_ARM)
     {
       grub_size_t tramp;
-      layout->kernel_size = ALIGN_UP (layout->kernel_size + image_target->vaddr_offset,
-				      image_target->section_align) - image_target->vaddr_offset;
 
       layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
 
@@ -2227,6 +2212,19 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
     }
 #endif
 
+  layout->kernel_size = ALIGN_UP (layout->kernel_size + image_target->vaddr_offset,
+			      image_target->section_align)
+    - image_target->vaddr_offset;
+  layout->exec_size = layout->kernel_size;
+
+  /* .data */
+  for (i = 0, s = smd->sections;
+       i < smd->num_sections;
+       i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
+    if (SUFFIX (is_data_section) (s, image_target))
+      layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, smd,
+						  image_target);
+
   layout->bss_start = layout->kernel_size;
   layout->end = layout->kernel_size;
   
-- 
2.16.4

[PATCH v2 2/2] arm: Align section alignment with manual relocation offset code

[Alexander Graf]

The arm relocation code has a manual special case for EFI binaries to
add the natural alignment to its own relocation awareness.

Since commit a51f953f4ee87 ("mkimage: Align efi sections on 4k
boundary") we changed that alignment from 0x400 to 0x1000 bytes. Reflect
the change in that branch that we forgot as well.

This fixes running 32bit arm grub efi binaries for me again.

Fixes: a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reported-by: Steve McIntyre <steve@einval.com>
Signed-off-by: Alexander Graf <agraf@csgraf.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Julien ROBIN <julien.robin28@free.fr>
---
 util/grub-mkimagexx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 470fbf4dd..bc087c2b5 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -1137,7 +1137,7 @@ SUFFIX (relocate_addrs) (Elf_Ehdr *e, struct section_metadata *smd,
 				       (int) sym_addr, (int) sym_addr);
 		       /* Data will be naturally aligned */
 		       if (image_target->id == IMAGE_EFI)
-			 sym_addr += 0x400;
+			 sym_addr += GRUB_PE32_SECTION_ALIGNMENT;
 		       *target = grub_host_to_target32 (grub_target_to_host32 (*target) + sym_addr);
 		     }
 		     break;
-- 
2.16.4

Re: [PATCH v2 0/2] Fix fallout from 4k section alignment patch

[Leif Lindholm]

On Tue, Apr 30, 2019 at 10:43:55PM +0200, Alexander Graf wrote:
> Commit a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
> broke ARM builds. There were 2 reasons for that:
> 
>   1) An NX bug that was lingering forever in the code base and only got
> triggered because of the change
> 
>   2) A missing adjustment in the original patch
> 
> This patch set fixes both issues, making 32bit ARM targets for UEFI
> fully functional again for me.

This looks good to me.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Tested-by: Leif Lindholm <leif.lindholm@linaro.org>

Thanks!

> Alex
> 
> Alexander Graf (2):
>   arm: Move trampolines into code section
>   arm: Align section alignment with manual relocation offset code
> 
>  util/grub-mkimagexx.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> -- 
> 2.16.4
> 

Re: [PATCH v2 0/2] Fix fallout from 4k section alignment patch

[Daniel Kiper]

On Wed, May 01, 2019 at 05:24:00PM +0100, Leif Lindholm wrote:
> On Tue, Apr 30, 2019 at 10:43:55PM +0200, Alexander Graf wrote:
> > Commit a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
> > broke ARM builds. There were 2 reasons for that:
> >
> >   1) An NX bug that was lingering forever in the code base and only got
> > triggered because of the change
> >
> >   2) A missing adjustment in the original patch
> >
> > This patch set fixes both issues, making 32bit ARM targets for UEFI
> > fully functional again for me.
>
> This looks good to me.
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> Tested-by: Leif Lindholm <leif.lindholm@linaro.org>

Pushed! Thanks guys for fixing that.

Daniel

PS I am back from vacation. I will try to reply for all lingering
   emails by the end of this week.