[PATCH] arm: Move trampolines into code section

[PATCH] 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>
---
 util/grub-mkimagexx.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 2059890c3..af23fae52 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);
 
@@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
 				       smd->num_sections, image_target);
 
       layout->tramp_off = layout->kernel_size;
-      layout->kernel_size += ALIGN_UP (tramp, 16);
     }
 #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

Re: [PATCH] arm: Move trampolines into code section

[Leif Lindholm]

On Tue, Apr 30, 2019 at 02:12:17AM +0200, Alexander Graf wrote:
> 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>

I can confirm that in combination with
"arm: Align section alignment with manual relocation offset code",
this patch resolves all of my test cases for armhf.

I'm going to grab some sleep and run some tests on !armhf tomorrow.

Many thanks for untangling this!

/
    Leif

> ---
>  util/grub-mkimagexx.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 2059890c3..af23fae52 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);
>  
> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
>  				       smd->num_sections, image_target);
>  
>        layout->tramp_off = layout->kernel_size;
> -      layout->kernel_size += ALIGN_UP (tramp, 16);
>      }
>  #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
> 

Re: [PATCH] arm: Move trampolines into code section

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3385 bytes --]

LGTM

On Tue, 30 Apr 2019, 10:12 Alexander Graf, <agraf@csgraf.de> wrote:

> 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>
> ---
>  util/grub-mkimagexx.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 2059890c3..af23fae52 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);
>
> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char
> *kernel_path,
>                                        smd->num_sections, image_target);
>
>        layout->tramp_off = layout->kernel_size;
> -      layout->kernel_size += ALIGN_UP (tramp, 16);
>      }
>  #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
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4662 bytes --]

Re: [PATCH] arm: Move trampolines into code section

[Leif Lindholm]

On Tue, Apr 30, 2019 at 02:12:17AM +0200, Alexander Graf wrote:
> 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.

Which was always a bug.

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

Right, so with a brain that's actually awake:

> ---
>  util/grub-mkimagexx.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 2059890c3..af23fae52 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);
> -

This patch only moves the below ifdef/conditional before the above
stanza, which remains unchanged. So this does not affect !armhf at
all. The generated diff is less than helpful here.

>  #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;

*boggle*, so we were double adjusting these on arm? That explains why
things were confused/confusing.

>  
>        layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
>  
> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,

However, the line just left out of context here
# tramp = arm_get_trampoline_size (e, smd->sections, smd->section_entsize,

>  				       smd->num_sections, image_target);

now looks a bit weird. We set "tramp" but never use it.

>  
>        layout->tramp_off = layout->kernel_size;
> -      layout->kernel_size += ALIGN_UP (tramp, 16);

Because we delete this adjustment.
Why is that no longer needed?

/
    Leif

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

Re: [PATCH] arm: Move trampolines into code section

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 4314 bytes --]

On Tue, 30 Apr 2019, 22:15 Leif Lindholm, <leif.lindholm@linaro.org> wrote:

> On Tue, Apr 30, 2019 at 02:12:17AM +0200, Alexander Graf wrote:
> > 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.
>
> Which was always a bug.
>
> > 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>
>
> Right, so with a brain that's actually awake:
>
> > ---
> >  util/grub-mkimagexx.c | 29 +++++++++++++----------------
> >  1 file changed, 13 insertions(+), 16 deletions(-)
> >
> > diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> > index 2059890c3..af23fae52 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);
> > -
>
> This patch only moves the below ifdef/conditional before the above
> stanza, which remains unchanged. So this does not affect !armhf at
> all. The generated diff is less than helpful here.
>
> >  #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;
>
> *boggle*, so we were double adjusting these on arm? That explains why
> things were confused/confusing.
>
> >
> >        layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
> >
> > @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const
> char *kernel_path,
>
> However, the line just left out of context here
> # tramp = arm_get_trampoline_size (e, smd->sections, smd->section_entsize,
>
> >                                      smd->num_sections, image_target);
>
> now looks a bit weird. We set "tramp" but never use it.
>
> >
> >        layout->tramp_off = layout->kernel_size;
> > -      layout->kernel_size += ALIGN_UP (tramp, 16);
>
> Because we delete this adjustment.
> Why is that no longer needed?
>
Because this point is no longer a section boundary.

>
> /
>     Leif
>
> >      }
> >  #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
> >
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 6163 bytes --]

Re: [PATCH] arm: Move trampolines into code section

[Alexander Graf]

> Am 30.04.2019 um 14:14 schrieb Leif Lindholm <leif.lindholm@linaro.org>:
> 
>> On Tue, Apr 30, 2019 at 02:12:17AM +0200, Alexander Graf wrote:
>> 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.
> 
> Which was always a bug.
> 
>> 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>
> 
> Right, so with a brain that's actually awake:
> 
>> ---
>> util/grub-mkimagexx.c | 29 +++++++++++++----------------
>> 1 file changed, 13 insertions(+), 16 deletions(-)
>> 
>> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
>> index 2059890c3..af23fae52 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);
>> -
> 
> This patch only moves the below ifdef/conditional before the above
> stanza, which remains unchanged. So this does not affect !armhf at
> all. The generated diff is less than helpful here.
> 
>> #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;
> 
> *boggle*, so we were double adjusting these on arm? That explains why
> things were confused/confusing.
> 
>> 
>>       layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
>> 
>> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
> 
> However, the line just left out of context here
> # tramp = arm_get_trampoline_size (e, smd->sections, smd->section_entsize,
> 
>>                       smd->num_sections, image_target);
> 
> now looks a bit weird. We set "tramp" but never use it.
> 
>> 
>>       layout->tramp_off = layout->kernel_size;
>> -      layout->kernel_size += ALIGN_UP (tramp, 16);
> 
> Because we delete this adjustment.
> Why is that no longer needed?

Because it was 2am for me too :). It obviously is needed - otherwise we blindly rely on the section padding to fit our trampoline.

Alex

> 
> /
>    Leif
> 
>>     }
>> #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
>> 

Re: [PATCH] arm: Move trampolines into code section

[Leif Lindholm]

On Tue, Apr 30, 2019 at 03:06:35PM +0200, Alexander Graf wrote:
> > Right, so with a brain that's actually awake:
> > 
> >> ---
> >> util/grub-mkimagexx.c | 29 +++++++++++++----------------
> >> 1 file changed, 13 insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> >> index 2059890c3..af23fae52 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);
> >> -
> > 
> > This patch only moves the below ifdef/conditional before the above
> > stanza, which remains unchanged. So this does not affect !armhf at
> > all. The generated diff is less than helpful here.
> > 
> >> #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;
> > 
> > *boggle*, so we were double adjusting these on arm? That explains why
> > things were confused/confusing.
> > 
> >> 
> >>       layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
> >> 
> >> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
> > 
> > However, the line just left out of context here
> > # tramp = arm_get_trampoline_size (e, smd->sections, smd->section_entsize,
> > 
> >>                       smd->num_sections, image_target);
> > 
> > now looks a bit weird. We set "tramp" but never use it.
> > 
> >> 
> >>       layout->tramp_off = layout->kernel_size;
> >> -      layout->kernel_size += ALIGN_UP (tramp, 16);
> > 
> > Because we delete this adjustment.
> > Why is that no longer needed?
> 
> Because it was 2am for me too :). It obviously is needed - otherwise
> we blindly rely on the section padding to fit our trampoline.

Ah, that makes more sense then :)

Well, if you put that adjustment in, and turn this into a 2-patch set
with the offset adjustment one, I think we're good to go.

I think this ought to be 1/2 with the offset adjustment 2/2, to
emphasise this fixes a problem alrady present.

/
    Leif

Re: [PATCH] arm: Move trampolines into code section

[Julien ROBIN]

[-- Attachment #1: Type: text/plain, Size: 5370 bytes --]

Hi,
This patch applied with "[PATCH] arm: Align section alignment with 
manual relocation offset code" seems to work perfect!

I tested as much EFI things as possible, with success in every case 
(using grub-mkstandalone EFI images).

     --target=arm-linux-gnueabihf --with-platform=efi
         fine with u-boot
         fine with https://efi.akeo.ie/QEMU_EFI/
         fine with "AAVMF32_code.fd" available into Debian 
"qemu-efi-arm" package (buster and experimental)
         fine with recent versions of : 
http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/latest/QEMU-ARM/

     --target=aarch64-linux-gnu --with-platform=efi
         fine with https://efi.akeo.ie/QEMU_EFI/
         fine with "AAVMF_code.fd" available into Debian 
"qemu-efi-aarch64" package (buster and experimental)
         fine with "QEMU_EFI.fd" available into Debian 
"qemu-efi-aarch64" package (buster and experimental)
         fine with recent versions of : 
http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/latest/QEMU-ARM/
         fine with Asus NovaGo TP370QL (Qualcomm Snapdragon 835)

     --target=i386  --with-platform=efi
         fine with OVMF-pure-efi.fd from 
https://www.kraxel.org/repos/jenkins/edk2/edk2.git-ovmf-ia32-0-20190308.1033.ge2d3a25f1a.noarch.rpm

     --target=x86_64  --with-platform=efi
         fine with OVMF.fd (from debian packages Debian)
         fine with an Asus R510JK-DM086H (Intel Core-i5 4200H)

For arm64, i386 and x86_64, none of the 2 patches changed the size of 
those 3 efi files resulting from grub-mkstandalone (the size only 
changed for arm-efi bootarm.efi). Which tends to confirm that the 
modification only implies arm 32 bits without impacting others efi 
architectures.

It's a chance we have efficient guys working with success on those 
complicated things! Thanks and congrats for having it repaired. Even if 
I'm not the user of arm-efi (32 bits) grub versions, I missed it when 
tested the patch about aarch64-laptops support so I'm glad those more 
advanced tests helped everyone.

Of course you can add "Tested-by: Julien ROBIN <julien.robin28@free.fr>" 
if relevant. If the patch is retouched I can still do some tests, but if 
you're hurried (or in case someone need it some day) the command lines I 
used are attached.

Bye !
Julien

On 4/30/19 2:12 AM, Alexander Graf wrote:
> 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>
> ---
>   util/grub-mkimagexx.c | 29 +++++++++++++----------------
>   1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 2059890c3..af23fae52 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);
>   
> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char *kernel_path,
>   				       smd->num_sections, image_target);
>   
>         layout->tramp_off = layout->kernel_size;
> -      layout->kernel_size += ALIGN_UP (tramp, 16);
>       }
>   #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;
>     
> 

[-- Attachment #2: firsts-complete-tests.txt --]
[-- Type: text/plain, Size: 3826 bytes --]

#MASTER / ARM 32 :

	#After having installed
	apt install pkg-config automake autopoint crossbuild-essential-armhf

	git clone git://git.savannah.gnu.org/grub.git && cd grub && ./bootstrap && ./configure --target=arm-linux-gnueabihf --with-platform=efi --prefix=/home/user/Desktop/grub/ && make -j 8 && make install
	/home/user/Desktop/grub/bin/grub-mkstandalone -o bootarm.efi -O arm-efi --directory /home/user/Desktop/grub/lib/grub/arm-efi

	#Then I created a GPT USB Stick with ESP/FAT32 partition (marked as EFI System Partition) in which /EFI/boot/bootarm.efi is placed, unmounted it, and made an image of it at /home/user/Desktop/esp/esp-master.img

	cd /home/user/Desktop/esp
	qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L ./ -bios QEMU_EFI_ARM.fd -drive format=raw,file=esp-master.img

------------------------------------------------------------------------------------------------------
#MASTER / ARM 64 :

	git clone git://git.savannah.gnu.org/grub.git && cd grub && ./bootstrap && ./configure --target=aarch64-linux-gnu --with-platform=efi --prefix=/home/user/Desktop/grub-arm64-efi && make -j 8 && make install
	cd ..
	/home/user/Desktop/grub-arm64-efi/bin/grub-mkstandalone -o bootaa64.efi -O arm64-efi --directory /home/user/Desktop/grub-arm64-efi/lib/grub/arm64-efi/
	qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./ -bios QEMU_EFI_AA64.fd -drive format=raw,file=esp-master.img

------------------------------------------------------------------------------------------------------
QEMU_EFI_ARM firmwares :
	https://efi.akeo.ie/QEMU_EFI/
	https://wiki.linaro.org/LEG/UEFIforQEMU
	wget http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/latest/QEMU-ARM/DEBUG_GCC5/QEMU_EFI.fd
	wget http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/latest/QEMU-ARM/RELEASE_GCC5/QEMU_EFI.fd
	
Another one, split in 2 files (the first one can be used as bios) :
	https://packages.debian.org/buster/qemu-efi-arm
	seems that you can use "-flash firstFile -flash secondFile", instead of "-bios firstFile"

------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------

Crossbuilding u-boot.bin for ARM, using x86_64 computer (having crossbuild-essential-armhf installed)

	git clone git://git.denx.de/u-boot-efi.git
	cd u-boot-efi/
	make ARCH=arm qemu_arm_defconfig
	make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- -j 8


#Running the /EFI/boot/bootarm.efi (placed into esp-master.img) from u-boot.bin bios :
qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L U-BOOT -bios u-boot.bin -drive format=raw,file=esp-master.img
	virtio scan
	virtio info
	virtio part
	load virtio 0:1 ${kernel_addr_r} EFI/boot/bootarm.efi
	bootefi ${kernel_addr_r}


#Building the RFC modified GRUB
	mkdir /home/user/Desktop/grub-rfc
	cd /home/user/Desktop/grub-rfc
	git clone git://git.savannah.gnu.org/grub.git
	#modifying grub/include/grub/efi/pe32.h 
	cd grub && ./bootstrap && ./configure --target=arm-linux-gnueabihf --with-platform=efi --prefix=/home/user/Desktop/grub-rfc/ && make -j 8 && make install
	cd ..
	/home/user/Desktop/grub-rfc/bin/grub-mkstandalone -o bootarm-rfc.efi -O arm-efi --directory /home/user/Desktop/grub-rfc/lib/grub/arm-efi
	#placing the file into an ESP and doing and image of it, named esp-rfc-arm.img

#Running it on QEMU :
	qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L U-BOOT -bios u-boot.bin -drive format=raw,file=esp-rfc-arm.img

#Once inside QEMU :
	virtio scan
	load virtio 0:1 ${kernel_addr_r} EFI/boot/bootarm-rfc.efi
	bootefi ${kernel_addr_r}

=> Works with modified grub/include/grub/efi/pe32.h.

[-- Attachment #3: grub-efi-esp-arm-intel-32-64-test-V2.txt --]
[-- Type: text/plain, Size: 4324 bytes --]

#arm-efi build (after having the patches applied)
cd grub && ./bootstrap && ./configure --target=arm-linux-gnueabihf --with-platform=efi --prefix=/home/user/Desktop/grub-arm-32/ && make -j 8 && make install && /home/user/Desktop/grub-arm-32/bin/grub-mkstandalone -o bootarm.efi -O arm-efi --directory /home/user/Desktop/grub-arm-32/lib/grub/arm-efi/

#arm64-efi build (after having the patches applied)
cd grub && ./bootstrap && ./configure --target=aarch64-linux-gnu --with-platform=efi --prefix=/home/user/Desktop/grub-arm-64/ && make -j 8 && make install && /home/user/Desktop/grub-arm-64/bin/grub-mkstandalone -o bootaa64.efi -O arm64-efi --directory /home/user/Desktop/grub-arm-64/lib/grub/arm64-efi/

#i386-efi build (after having the patches applied)
cd grub && ./bootstrap && ./configure --target=i386 --with-platform=efi --prefix=/home/user/Desktop/grub-x86_32/ && make -j 8 && make install && /home/user/Desktop/grub-x86_32/bin/grub-mkstandalone -o bootia32.efi -O i386-efi --directory /home/user/Desktop/grub-x86_32/lib/grub/i386-efi/

#x86_64-efi build (after having the patches applied)
cd grub && ./bootstrap && ./configure --target=x86_64 --with-platform=efi --prefix=/home/user/Desktop/grub-x86_64/ && make -j 8 && make install && /home/user/Desktop/grub-x86_64/bin/grub-mkstandalone -o bootx64.efi -O x86_64-efi --directory /home/user/Desktop/grub-x86_64/lib/grub/x86_64-efi/

#Placed the 4 bootXXX.efi files into /EFI/boot/bootXXX.efi of an USB stick (GPT, fat32, marked EFI System Paritition)
#Used it for physical hardware, and made an image of it (grub-efi-esp-arm-intel-32-64.img) for QEMU tests

#arm-efi test :
qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L ./arm-efi -bios AAVMF32_CODE_buster.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L ./arm-efi -bios AAVMF32_CODE_experimental.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L ./arm-efi -bios QEMU_EFI_ARM_AKEO.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L ./arm-efi -bios QEMU_EFI_ARM_DEBUG.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L ./arm-efi -bios QEMU_EFI_ARM_RELEASE.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-arm -M virt -m 512 -net none -monitor none -parallel none -L ./arm-efi -bios u-boot.bin -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img


#arm64-efi test :
qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./arm64-efi -bios AAVMF_CODE_buster.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./arm64-efi -bios AAVMF_CODE_experimental.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./arm64-efi -bios QEMU_EFI_AA64_AKEO.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./arm64-efi -bios QEMU_EFI_AA64_buster.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./arm64-efi -bios QEMU_EFI_AA64_experimental.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./arm64-efi -bios QEMU_EFI_AA64_DEBUG.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img
qemu-system-aarch64 -M virt -cpu cortex-a57 -m 2048 -monitor none -parallel none -L ./arm64-efi -bios QEMU_EFI_AA64_RELEASE.fd -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img

#i386-efi test :
qemu-system-i386 -m 2048 -smp 4 -L ./i386-efi -bios OVMF-pure-efi.fd -vga virtio -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img

#x86_64-efi test :
qemu-system-x86_64 -m 2048 -smp 4 -L /usr/share/ovmf -bios OVMF.fd -vga virtio -drive format=raw,file=../grub-efi-esp-arm-intel-32-64.img