Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 11 Mar 2022 09:00:25 -0800 (PST)
Subject:     Re: [PATCH] riscv: Fix auipc+jalr relocation range checks
In-Reply-To: <20220223191257.143694-1-kernel@esmil.dk>
CC:     linux-riscv@lists.infradead.org, kernel@esmil.dk,
        Paul Walmsley <paul.walmsley@sifive.com>,
        aou@eecs.berkeley.edu, alex@ghiti.fr, jszhang@kernel.org,
        linux-kernel@vger.kernel.org
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     kernel@esmil.dk
Message-ID: <mhng-5e2f04e5-f721-4ef6-a51d-ddd4611bfa5f@palmer-ri-x1c9>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, 23 Feb 2022 11:12:57 PST (-0800), kernel@esmil.dk wrote:
> RISC-V can do PC-relative jumps with a 32bit range using the following
> two instructions:
>
> 	auipc	t0, imm20	; t0 = PC + imm20 * 2^12
> 	jalr	ra, t0, imm12	; ra = PC + 4, PC = t0 + imm12
>
> Crucially both the 20bit immediate imm20 and the 12bit immediate imm12
> are treated as two's-complement signed values. For this reason the
> immediates are usually calculated like this:
>
> 	imm20 = (offset + 0x800) >> 12
> 	imm12 = offset & 0xfff
>
> ..where offset is the signed offset from the auipc instruction. When
> the 11th bit of offset is 0 the addition of 0x800 doesn't change the top
> 20 bits and imm12 considered positive. When the 11th bit is 1 the carry
> of the addition by 0x800 means imm20 is one higher, but since imm12 is
> then considered negative the two's complement representation means it
> all cancels out nicely.
>
> However, this addition by 0x800 (2^11) means an offset greater than or
> equal to 2^31 - 2^11 would overflow so imm20 is considered negative and
> result in a backwards jump. Similarly the lower range of offset is also
> moved down by 2^11 and hence the true 32bit range is
>
> 	[-2^31 - 2^11, 2^31 - 2^11)
>
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> ---
>  arch/riscv/kernel/module.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 68a9e3d1fe16..4a48287513c3 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -13,6 +13,19 @@
>  #include <linux/pgtable.h>
>  #include <asm/sections.h>
>
> +/*
> + * The auipc+jalr instruction pair can reach any PC-relative offset
> + * in the range [-2^31 - 2^11, 2^31 - 2^11)
> + */
> +static bool riscv_insn_valid_32bit_offset(ptrdiff_t val)
> +{
> +#ifdef CONFIG_32BIT
> +	return true;
> +#else
> +	return (-(1L << 31) - (1L << 11)) <= val && val < ((1L << 31) - (1L << 11));
> +#endif
> +}
> +
>  static int apply_r_riscv_32_rela(struct module *me, u32 *location, Elf_Addr v)
>  {
>  	if (v != (u32)v) {
> @@ -95,7 +108,7 @@ static int apply_r_riscv_pcrel_hi20_rela(struct module *me, u32 *location,
>  	ptrdiff_t offset = (void *)v - (void *)location;
>  	s32 hi20;
>
> -	if (offset != (s32)offset) {
> +	if (!riscv_insn_valid_32bit_offset(offset)) {
>  		pr_err(
>  		  "%s: target %016llx can not be addressed by the 32-bit offset from PC = %p\n",
>  		  me->name, (long long)v, location);
> @@ -197,10 +210,9 @@ static int apply_r_riscv_call_plt_rela(struct module *me, u32 *location,
>  				       Elf_Addr v)
>  {
>  	ptrdiff_t offset = (void *)v - (void *)location;
> -	s32 fill_v = offset;
>  	u32 hi20, lo12;
>
> -	if (offset != fill_v) {
> +	if (!riscv_insn_valid_32bit_offset(offset)) {
>  		/* Only emit the plt entry if offset over 32-bit range */
>  		if (IS_ENABLED(CONFIG_MODULE_SECTIONS)) {
>  			offset = module_emit_plt_entry(me, v);
> @@ -224,10 +236,9 @@ static int apply_r_riscv_call_rela(struct module *me, u32 *location,
>  				   Elf_Addr v)
>  {
>  	ptrdiff_t offset = (void *)v - (void *)location;
> -	s32 fill_v = offset;
>  	u32 hi20, lo12;
>
> -	if (offset != fill_v) {
> +	if (!riscv_insn_valid_32bit_offset(offset)) {
>  		pr_err(
>  		  "%s: target %016llx can not be addressed by the 32-bit offset from PC = %p\n",
>  		  me->name, (long long)v, location);

Thanks, this is on fixes.