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

[Alexander Graf]

> Am 30.04.2019 um 14:14 schrieb Leif Lindholm <address@hidden>:
> 
>> 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 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
>>