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

[Ard Biesheuvel]

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)