Re: [PATCH bpf] riscv, bpf: Fix offset range checking for auipc+jalr on RV64

[BjÃrn TÃpel]

On Tue, 7 Apr 2020 at 00:16, Luke Nelson <lukenels@xxxxxxxxxxxxxxxxx> wrote:
>
> The existing code in emit_call on RV64 checks that the PC-relative offset
> to the function fits in 32 bits before calling emit_jump_and_link to emit
> an auipc+jalr pair. However, this check is incorrect because offsets in
> the range [2^31 - 2^11, 2^31 - 1] cannot be encoded using auipc+jalr on
> RV64 (see discussion [1]). The RISC-V spec has recently been updated
> to reflect this fact [2, 3].
>
> This patch fixes the problem by moving the check on the offset into
> emit_jump_and_link and modifying it to the correct range of encodable
> offsets, which is [-2^31 - 2^11, 2^31 - 2^11). This also enforces the
> check on the offset to other uses of emit_jump_and_link (e.g., BPF_JA)
> as well.
>
> Currently, this bug is unlikely to be triggered, because the memory
> region from which JITed images are allocated is close enough to kernel
> text for the offsets to not become too large; and because the bounds on
> BPF program size are small enough. This patch prevents this problem from
> becoming an issue if either of these change.
>
> [1]: https://groups.google.com/a/groups.riscv.org/forum/#!topic/isa-dev/bwWFhBnnZFQ
> [2]: https://github.com/riscv/riscv-isa-manual/commit/b1e42e09ac55116dbf9de5e4fb326a5a90e4a993
> [3]: https://github.com/riscv/riscv-isa-manual/commit/4c1b2066ebd2965a422e41eb262d0a208a7fea07
>

Wow! Interesting! Thanks for fixing this!

Too late to Ack, but still:

Acked-by: BjÃrn TÃpel <bjorn.topel@xxxxxxxxx>

> Signed-off-by: Luke Nelson <luke.r.nels@xxxxxxxxx>
> ---
>  arch/riscv/net/bpf_jit_comp64.c | 49 +++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index cc1985d8750a..d208a9fd6c52 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -110,6 +110,16 @@ static bool is_32b_int(s64 val)
>         return -(1L << 31) <= val && val < (1L << 31);
>  }
>
> +static bool in_auipc_jalr_range(s64 val)
> +{
> +       /*
> +        * auipc+jalr can reach any signed PC-relative offset in the range
> +        * [-2^31 - 2^11, 2^31 - 2^11).
> +        */
> +       return (-(1L << 31) - (1L << 11)) <= val &&
> +               val < ((1L << 31) - (1L << 11));
> +}
> +
>  static void emit_imm(u8 rd, s64 val, struct rv_jit_context *ctx)
>  {
>         /* Note that the immediate from the add is sign-extended,
> @@ -380,20 +390,24 @@ static void emit_sext_32_rd(u8 *rd, struct rv_jit_context *ctx)
>         *rd = RV_REG_T2;
>  }
>
> -static void emit_jump_and_link(u8 rd, s64 rvoff, bool force_jalr,
> -                              struct rv_jit_context *ctx)
> +static int emit_jump_and_link(u8 rd, s64 rvoff, bool force_jalr,
> +                             struct rv_jit_context *ctx)
>  {
>         s64 upper, lower;
>
>         if (rvoff && is_21b_int(rvoff) && !force_jalr) {
>                 emit(rv_jal(rd, rvoff >> 1), ctx);
> -               return;
> +               return 0;
> +       } else if (in_auipc_jalr_range(rvoff)) {
> +               upper = (rvoff + (1 << 11)) >> 12;
> +               lower = rvoff & 0xfff;
> +               emit(rv_auipc(RV_REG_T1, upper), ctx);
> +               emit(rv_jalr(rd, RV_REG_T1, lower), ctx);
> +               return 0;
>         }
>
> -       upper = (rvoff + (1 << 11)) >> 12;
> -       lower = rvoff & 0xfff;
> -       emit(rv_auipc(RV_REG_T1, upper), ctx);
> -       emit(rv_jalr(rd, RV_REG_T1, lower), ctx);
> +       pr_err("bpf-jit: target offset 0x%llx is out of range\n", rvoff);
> +       return -ERANGE;
>  }
>
>  static bool is_signed_bpf_cond(u8 cond)
> @@ -407,18 +421,16 @@ static int emit_call(bool fixed, u64 addr, struct rv_jit_context *ctx)
>         s64 off = 0;
>         u64 ip;
>         u8 rd;
> +       int ret;
>
>         if (addr && ctx->insns) {
>                 ip = (u64)(long)(ctx->insns + ctx->ninsns);
>                 off = addr - ip;
> -               if (!is_32b_int(off)) {
> -                       pr_err("bpf-jit: target call addr %pK is out of range\n",
> -                              (void *)addr);
> -                       return -ERANGE;
> -               }
>         }
>
> -       emit_jump_and_link(RV_REG_RA, off, !fixed, ctx);
> +       ret = emit_jump_and_link(RV_REG_RA, off, !fixed, ctx);
> +       if (ret)
> +               return ret;
>         rd = bpf_to_rv_reg(BPF_REG_0, ctx);
>         emit(rv_addi(rd, RV_REG_A0, 0), ctx);
>         return 0;
> @@ -429,7 +441,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>  {
>         bool is64 = BPF_CLASS(insn->code) == BPF_ALU64 ||
>                     BPF_CLASS(insn->code) == BPF_JMP;
> -       int s, e, rvoff, i = insn - ctx->prog->insnsi;
> +       int s, e, rvoff, ret, i = insn - ctx->prog->insnsi;
>         struct bpf_prog_aux *aux = ctx->prog->aux;
>         u8 rd = -1, rs = -1, code = insn->code;
>         s16 off = insn->off;
> @@ -699,7 +711,9 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>         /* JUMP off */
>         case BPF_JMP | BPF_JA:
>                 rvoff = rv_offset(i, off, ctx);
> -               emit_jump_and_link(RV_REG_ZERO, rvoff, false, ctx);
> +               ret = emit_jump_and_link(RV_REG_ZERO, rvoff, false, ctx);
> +               if (ret)
> +                       return ret;
>                 break;
>
>         /* IF (dst COND src) JUMP off */
> @@ -801,7 +815,6 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>         case BPF_JMP | BPF_CALL:
>         {
>                 bool fixed;
> -               int ret;
>                 u64 addr;
>
>                 mark_call(ctx);
> @@ -826,7 +839,9 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>                         break;
>
>                 rvoff = epilogue_offset(ctx);
> -               emit_jump_and_link(RV_REG_ZERO, rvoff, false, ctx);
> +               ret = emit_jump_and_link(RV_REG_ZERO, rvoff, false, ctx);
> +               if (ret)
> +                       return ret;
>                 break;
>
>         /* dst = imm64 */
> --
> 2.17.1
>