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

[Patrick Zacharias <littlefighter19@web.de>]

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@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/ : {
>