[PATCH] arm64/efi: set PE/COFF section alignment to 4 KB

[Ard Biesheuvel]

On 10 October 2014 16:09, Mark Rutland <mark.rutland at arm.com> wrote:
> On Fri, Oct 10, 2014 at 11:37:03AM +0100, Ard Biesheuvel wrote:
>> On 10 October 2014 12:33, Mark Rutland <mark.rutland at arm.com> wrote:
>> > Hi Ard,
>> >
>> > On Fri, Oct 10, 2014 at 10:25:24AM +0100, Ard Biesheuvel wrote:
>> >> Position independent AArch64 code needs to be linked and loaded at the same
>> >> relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will
>> >> not work correctly. (This is how PC relative symbol references with a 4 GB
>> >> reach are emitted)
>> >>
>> >> We need to declare this in the PE/COFF header, otherwise the PE/COFF loader
>> >> may load the Image and invoke the stub at an offset which violates this rule.
>> >
>> > Has this been observed happening, or was this just found by inspection?
>> >
>>
>> This is also something found by inspection, or rather, by the
>> discussion going on in the other thread. I am not aware of any PE/COFF
>> loaders that may choose an offset that is not 4 KB aligned, even if
>> the header we give it appears to allow it.
>>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> >> ---
>> >>  arch/arm64/kernel/head.S | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> >> index 0a6e4f924df8..5e83e5b8a9de 100644
>> >> --- a/arch/arm64/kernel/head.S
>> >> +++ b/arch/arm64/kernel/head.S
>> >> @@ -159,7 +159,7 @@ optional_header:
>> >>
>> >>  extra_header_fields:
>> >>       .quad   0                               // ImageBase
>> >> -     .long   0x20                            // SectionAlignment
>> >> +     .long   0x1000                          // SectionAlignment
>
> Looking at this again, I'm more confused than I was to begin with.
>

:-)

> Surely we know exactly where the .text section will be loaded because of
> its VirtualAddress? If that were the case, we can drop the .align 12 as
> we already load it at the offset any arp or :lo12: immediate will have
> been built for.
>

No, not quite. It only tells us what the /offset/ should be of the
section from the ImageBase chosen by the loader, not what the
alignment of ImageBase itself should be. For instance, with a section
VirtualAddress of 0x1000 and a SectionAlignment of 0x400, the section
could legally be loaded @ 0x1400 or 0x1800 (for ImageBase == 0x400 or
0x800, respectively)

-- 
Ard.


> I don't follow how SectionAlignment interacts with a given section's
> VirtualAddress.
>
> Thanks,
> Mark.
>
>> >>       .long   0x8                             // FileAlignment
>> >>       .short  0                               // MajorOperatingSystemVersion
>> >>       .short  0                               // MinorOperatingSystemVersion
>> >> @@ -226,7 +226,7 @@ section_table:
>> >>       .short  0               // NumberOfRelocations  (0 for executables)
>> >>       .short  0               // NumberOfLineNumbers  (0 for executables)
>> >>       .long   0xe0500020      // Characteristics (section flags)
>> >> -     .align 5
>> >> +     .align 12
>> >
>> > Can we get a comment explaining why stext needs the additional
>> > alignment? Something like:
>> >
>> >         /*
>> >          * EFI will load stext onwards at the 4k section alignment
>> >          * described in the PE/COFF header. To ensure that instruction
>> >          * sequences using an adrp and a :lo12: immediate will function
>> >          * correctly at this alignment, we must ensure that stext is
>> >          * placed at a 4k boundary in the Image to begin with.
>> >          */
>> >         .align 12
>> >
>>
>> OK
>>
>> --
>> Ard.
>>