| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 8 Apr 2020 07:39:06 +0200
From: Björn Töpel <bjorn.topel@...il.com>
To: Luke Nelson <lukenels@...washington.edu>
Cc: bpf <bpf@...r.kernel.org>, Xi Wang <xi.wang@...il.com>,
Luke Nelson <luke.r.nels@...il.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Andrii Nakryiko <andriin@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...omium.org>,
Netdev <netdev@...r.kernel.org>, linux-riscv@...ts.infradead.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf] riscv, bpf: Fix offset range checking for auipc+jalr
on RV64
On Tue, 7 Apr 2020 at 00:16, Luke Nelson <lukenels@...washington.edu> 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@...il.com>
> Signed-off-by: Luke Nelson <luke.r.nels@...il.com>
> ---
> 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
>
Powered by blists - more mailing lists