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

[Jan Kiszka]

On 16.01.24 14:47, Ard Biesheuvel wrote:
> On Tue, 16 Jan 2024 at 14:44, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
>>
>> On 16.01.24 09:36, Ard Biesheuvel wrote:
>>> On Tue, 16 Jan 2024 at 06:21, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
>>>>
>>>> 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?
>>>>
>>>
>>> We'll need to keep the HI20, in fact - I got confused between HI20 and
>>> PCREL_HI20, and the former is actually used for 32-bit absolute
>>> addresses in 32-bit code.
>>>
>>> This seems to do the trick: it disallows relaxation relocations and
>>> native word sizes absolute references. AFAICT, those are the only ones
>>> we should care about.
>>>
>>> STUBCOPY_RELOC-$(CONFIG_RISCV) := -E
>>> R_RISCV_HI20\|R_RISCV_$(BITS)\|R_RISCV_RELAX
>>
>> I would suggest to do that on top of this patch. Want me to write such a
>> patch, or will you? You can probably more fluently explain why
>> R_RISCV_32/64 is important, I would first have to understand what that
>> is exactly. :)
>>
> 
> Sure, I can take care of that.
> 

Perfect.

> For your patch,
> 
> Reviewed-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> 
> I'll queue this up as a EFI fix.

Thanks. Will you take care of stable, or should I once the commit was
merged?

Jan

-- 
Siemens AG, Technology
Linux Expert Center