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

[Julien ROBIN]

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

firsts-complete-tests.txt
Description: Text document

grub-efi-esp-arm-intel-32-64-test-V2.txt
Description: Text document