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.robi...@free.fr>
> Reported-by: Leif Lindholm <leif.lindh...@linaro.org>
>
> Signed-off-by: Alexander Graf <ag...@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
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
