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. :) Jan -- Siemens AG, Technology Linux Expert Center
