[PATCH] efi: arm,arm64: Fix relocations from not being loaded

[Heinrich Schuchardt]

Am 6. November 2022 19:36:15 MEZ schrieb Patrick Zacharias <littlefighter19 at web.de>:
>I created a GitHub repository demonstrating the issue, together with instructions on how to build it.
>
>https://github.com/Fighter19/hello-world-arm-efi

Please, check the instructions in

https://doc.rust-lang.org/rustc/platform-support/unknown-uefi.html

Best regards

Heinrich




>
>Currently, when using the old linker file, it will crash.
>Personally, to test it, I built U-Boot for QEMU and loaded the EFI file via TFTP.
>
>Am 06.11.22 um 18:03 schrieb Patrick Zacharias:
>> Hi Heinrich,
>> 
>> Thanks for the information, I'll make sure to use it for future contributions.
>> 
>> I encountered this issue, while building a custom EFI application using Rust by linking
>> the following files:
>> 
>> ./arch/arm/lib/reloc_arm_efi.c
>> ./arch/arm/lib/crt0_arm_efi.S
>> ./arch/arm/lib/elf_arm_efi.lds
>> 
>> As well as the custom application built as a library, that exports "efi_main".
>> 
>> Thus, the relocations were caused by the "write_fmt" functions that Rust provides.
>> I encountered it, when concatenating two strings.
>> format!("Test {}", "Test2");
>> 
>> The relocation was not applied and therefor lead to a crash.
>> 
>> From my understanding these relocations are ELF relocations and are therefor applied by the _relocate function (inside the built application)
>> and not by the EFI loader. This is why I assumed, that it's fine for these relocations to reside in the data section.
>> 
>> I noticed, that in the _relocate function in ./arch/arm/lib/reloc_arm_efi.c, that the case R_ARM_RELATIVE was never entered,
>> even though the generated shared object included them.
>> I then looked at the EFI header file (crt0_arm_efi.S) and noticed, that it specifies only up to _edata to be loaded.
>> 
>> I concluded that _edata needs to be extended to include the ELF relocations as well.
>> 
>> After I decided to place it after the relocations, I noticed, that _edata is already specified at the same (new position) in gnu-efi.
>> This reaffirmed me to contribute the patch as it currently is.
>> 
>> I assumed, that on x86, that the linker treats the ELF relocations (.rela.*) as if it was part of the data section.
>> From my understanding the EFI relocations are in .reloc while the ELF relocations are in .rela.*.
>> I might be mistaken, however.
>> 
>> Greetings,
>> Patrick
>> 
>> Am 06.11.22 um 10:20 schrieb Heinrich Schuchardt:
>>> On 10/31/22 21:01, Patrick Zacharias wrote:
>>>> Prior to this commit, the relocations would not get loaded by the efi
>>>> loader.
>>>> 
>>>> This lead to none of the relocations being applied.
>>>> 
>>>> Signed-off-by: Fighter19 <1475802+Fighter19 at users.noreply.github.com>
>>> 
>>> Thanks Patrick for your contribution.
>>> 
>>> You can use scripts/get_maintainer.pl to determine to whom a patch
>>> should be sent.
>>> 
>>> Where did you actually see relocations?
>>> Which code is not position independent?
>>> 
>>>> ---
>>>>   arch/arm/lib/elf_aarch64_efi.lds | 2 +-
>>>>   arch/arm/lib/elf_arm_efi.lds     | 2 +-
>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/arch/arm/lib/elf_aarch64_efi.lds
>>>> b/arch/arm/lib/elf_aarch64_efi.lds
>>>> index c0604dad46..1982864d17 100644
>>>> --- a/arch/arm/lib/elf_aarch64_efi.lds
>>>> +++ b/arch/arm/lib/elf_aarch64_efi.lds
>>>> @@ -46,12 +46,12 @@ SECTIONS
>>>>           *(COMMON)
>>>>           . = ALIGN(512);
>>>>           _bss_end = .;
>>>> -        _edata = .;
>>>>       }
>>>>       .rela.dyn : { *(.rela.dyn) }
>>>>       .rela.plt : { *(.rela.plt) }
>>>>       .rela.got : { *(.rela.got) }
>>>>       .rela.data : { *(.rela.data) *(.rela.data*) }
>>>> +    _edata = .;
>>>>       _data_size = . - _etext;
>>>> 
>>>>       . = ALIGN(4096);
>>>> diff --git a/arch/arm/lib/elf_arm_efi.lds b/arch/arm/lib/elf_arm_efi.lds
>>>> index 767ebda635..c1b58a8033 100644
>>>> --- a/arch/arm/lib/elf_arm_efi.lds
>>>> +++ b/arch/arm/lib/elf_arm_efi.lds
>>>> @@ -46,12 +46,12 @@ SECTIONS
>>>>           *(COMMON)
>>>>           . = ALIGN(512);
>>>>           _bss_end = .;
>>>> -        _edata = .;
>>>>       }
>>>>       .rel.dyn : { *(.rel.dyn) }
>>>>       .rel.plt : { *(.rel.plt) }
>>>>       .rel.got : { *(.rel.got) }
>>>>       .rel.data : { *(.rel.data) *(.rel.data*) }
>>>> +    _edata = .;
>>> 
>>> Relocations (if they exist) should be in the .reloc section, not in the
>>> .data section.
>>> 
>>> If we want to create a .reloc section, we have to change
>>> arch/arm/lib/crt0_*_efi.S too. Furthermore the relocation section must
>>> be pointed to by field BaseRelocationTable of the Optional Header Data
>>> Directories (see PE-COFF specification).
>>> 
>>> Please, consider the other UEFI architectures (x86 and RISC-V) too.
>>> 
>>> Best regards
>>> 
>>> Heinrich
>>> 
>>>>       _data_size = . - _etext;
>>>> 
>>>>       /DISCARD/ : {
>>> 
>