Received-SPF: pass (google.com: domain of linux-kernel+bounces-27131-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <mhng-8a4a5f85-faf9-405c-95a0-78cd04ec6509@palmer-ri-x1c9>
 <56b52d4e-9dc0-400c-a141-7e70f5c72afa@siemens.com> <CAMj1kXGR1aQdvej+0drfim-ZP27ZhO9UR_i_PT6F+hsV0UvbJw@mail.gmail.com>
 <578aae7c-4069-4071-ba4b-cc86d3b516c1@siemens.com>
In-Reply-To: <578aae7c-4069-4071-ba4b-cc86d3b516c1@siemens.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Tue, 16 Jan 2024 09:36:45 +0100
Message-ID: <CAMj1kXEC4=j5zcSyo91jnUkYMoqBG5M2raMXm4DNdD5ZbS3U7g@mail.gmail.com>
Subject: Re: [PATCH] riscv/efistub: Ensure GP-relative addressing is not used
To: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Palmer Dabbelt <palmer@rivosinc.com>, linux-efi@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Tue, 16 Jan 2024 at 06:21, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 15.01.24 18:34, Ard Biesheuvel wrote:
> > On Sat, 13 Jan 2024 at 11:35, Jan Kiszka <jan.kiszka@siemens.com> 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@siemens.com> wrote:
> >>>>>
> >>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>
> >>>>> 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@siemens.com>
> >>>>> ---
> >>>>>
> >>>>> 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