Re: [PATCH] riscv/efistub: Ensure GP-relative addressing is not used

[Jan Kiszka]

On 15.01.24 18:34, Ard Biesheuvel wrote:
> On Sat, 13 Jan 2024 at 11:35, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
>>
>> On 12.01.24 19:56, Palmer Dabbelt wrote:
>>> On Fri, 12 Jan 2024 10:51:16 PST (-0800), Ard Biesheuvel wrote:
>>>> Hi Jan,
>>>>
>>>> On Fri, 12 Jan 2024 at 19:37, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
>>>>>
>>>>> From: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>>>>>
>>>>> The cflags for the RISC-V efistub were missing -mno-relax, thus were
>>>>> under the risk that the compiler could use GP-relative addressing. That
>>>>> happened for _edata with binutils-2.41 and kernel 6.1, causing the
>>>>> relocation to fail due to an invalid kernel_size in handle_kernel_image.
>>>>> It was not yet observed with newer versions, but that may just be luck.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>> Something like this should go to stable as well, but we will need
>>>>> rebased patches.
>>>>>
>>>>>  drivers/firmware/efi/libstub/Makefile | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/firmware/efi/libstub/Makefile
>>>>> b/drivers/firmware/efi/libstub/Makefile
>>>>> index 06964a3c130f..d561d7de46a9 100644
>>>>> --- a/drivers/firmware/efi/libstub/Makefile
>>>>> +++ b/drivers/firmware/efi/libstub/Makefile
>>>>> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM)          += -DEFI_HAVE_STRLEN
>>>>> -DEFI_HAVE_STRNLEN \
>>>>>                                    -DEFI_HAVE_MEMCHR
>>>>> -DEFI_HAVE_STRRCHR \
>>>>>                                    -DEFI_HAVE_STRCMP -fno-builtin
>>>>> -fpic \
>>>>>                                    $(call
>>>>> cc-option,-mno-single-pic-base)
>>>>> -cflags-$(CONFIG_RISCV)         += -fpic -DNO_ALTERNATIVE
>>>>> +cflags-$(CONFIG_RISCV)         += -fpic -DNO_ALTERNATIVE -mno-relax
>>>>
>>>> Can we detect the presence of these references (via the relocation
>>>> type)? We already do something similar for ordinary absolute
>>>> references too.
>>>
>>> If there's no `__global_pointer$` symbol then the linker won't make
>>> GP-relative relaxations (because it doesn't know where GP is).  We
>>> usually define that symbol in the linker script, but I'm not entierly
>>> sure how libstub gets its linker script...
>>>
>>
>> The stub seems to be linked together with the rest of the kernel, thus
>> the regular arch/riscv/kernel/vmlinux.lds.S is used.
>>
> 
> Indeed - the EFI stub is part of the same executable as vmlinux, we
> just mangle the symbol names to ensure that only code that can be
> safely called from the EFI stub can be linked to it.
> 
> If the effect of -mno-relax is to stop emitting R_RISCV_RELAX
> relocations, we should perhaps add those to the STUBCOPY_RELOC-y
> Makefile variable? (in the same file). BTW R_RISCV_HI20 doesn't seem
> like the right value there to begin with: the idea of that is to
> disallow ELF relocations that evaluate to expressions that can only be
> known at runtime (like absolute addresses for global pointer
> variables)

How to do that best? Simply replace R_RISCV_HI20 with R_RISCV_RELAX?

Jan

-- 
Siemens AG, Technology
Linux Expert Center