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 <address@hidden>
> Reported-by: Leif Lindholm <address@hidden>
> 
> Signed-off-by: Alexander Graf <address@hidden>

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
>