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

[Vladimir 'phcoder' Serbinenko]

On Tue, 30 Apr 2019, 22:15 Leif Lindholm, <address@hidden> 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 <address@hidden>
> Reported-by: Leif Lindholm <address@hidden>
>
> Signed-off-by: Alexander Graf <address@hidden>

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
address@hidden
https://lists.gnu.org/mailman/listinfo/grub-devel