On Mon, Mar 4, 2019 at 7:59 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > On 03/04/2019 07:03 AM, Andrii Nakryiko wrote: > > On Thu, Feb 28, 2019 at 3:31 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > [...] > >> @@ -6664,8 +6669,10 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env) > >> } > >> > >> if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) { > >> + struct bpf_insn_aux_data *aux; > >> struct bpf_map *map; > >> struct fd f; > >> + u64 addr; > >> > >> if (i == insn_cnt - 1 || insn[1].code != 0 || > >> insn[1].dst_reg != 0 || insn[1].src_reg != 0 || > > > > Next line after this one rejects ldimm64 instructions with off != 0. > > This check needs to be changed, depending on whether src_reg == > > BPF_PSEUDO_MAP_VALUE, right? > > Yes, that's correct, I already have that changed in my local branch for > supporting non-zero off. > > > This is also to the previously discussed question of not enforcing > > offset (imm=0 in 2nd part of insn) for BPF_PSEUDO_MAP_FD. Seems like > > verifier *does* enforce that (not that I'm advocating for re-using > > BPF_PSEUDO_MAP_FD, just stumbled on this bit when going through > > verifier code). > > Not really, lets test: Ah, sorry, my bad. That code tests .off, not .imm, so yeah, any imm would be accepted. > > [...] > .insns = { > BPF_MOV64_IMM(BPF_REG_0, 0), > BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, BPF_REG_1, > BPF_PSEUDO_MAP_FD, 0, 0), > BPF_RAW_INSN(0, 0, 0, 0, 0xfefefe), > BPF_EXIT_INSN(), > }, > [...] > > #545/p test14 ld_imm64: reject 2nd imm != 0 FAIL > Unexpected success to load! > 0: (b7) r0 = 0 > 1: (18) r1 = 0xffff97e612486400 > 3: (95) exit > processed 3 insns (limit 131072), stack depth 0 > Summary: 0 PASSED, 0 SKIPPED, 2 FAILED > > So I still think it would be worth doing something like the below > for bpf: Yep, lgtm. > > From 290f739ae6bab7b0709d327855a1812f9022beed Mon Sep 17 00:00:00 2001 > From: Daniel Borkmann <daniel@xxxxxxxxxxxxx> > Date: Mon, 4 Mar 2019 14:22:41 +0000 > Subject: [PATCH bpf] bpf: fix replace_map_fd_with_map_ptr wrt ldimm64 wrt second imm field > > Non-zero imm value in the second part of the ldimm64 instruction for > BPF_PSEUDO_MAP_FD is invalid, and thus must be rejected. The map fd > only ever sits in the first instructions' imm field. None of the BPF > loaders known to us are using it, so risk of regression is minimal. > For clarity and consistency, the few insn->{src_reg,imm} occurences > are rewritten into insn[0].{src_reg,imm}. Add a test case to the BPF > selftest suite as well. > > Fixes: 0246e64d9a5f ("bpf: handle pseudo BPF_LD_IMM64 insn") > Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx> > --- > kernel/bpf/verifier.c | 10 +++++----- > tools/testing/selftests/bpf/verifier/ld_imm64.c | 17 +++++++++++++++-- > 2 files changed, 20 insertions(+), 7 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 0e4edd7e3c5f..c8d2a948db37 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6678,17 +6678,17 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env) > /* valid generic load 64-bit imm */ > goto next_insn; > > - if (insn->src_reg != BPF_PSEUDO_MAP_FD) { > - verbose(env, > - "unrecognized bpf_ld_imm64 insn\n"); > + if (insn[0].src_reg != BPF_PSEUDO_MAP_FD || > + insn[1].imm != 0) { > + verbose(env, "unrecognized bpf_ld_imm64 insn\n"); > return -EINVAL; > } > > - f = fdget(insn->imm); > + f = fdget(insn[0].imm); > map = __bpf_map_get(f); > if (IS_ERR(map)) { > verbose(env, "fd %d is not pointing to valid bpf_map\n", > - insn->imm); > + insn[0].imm); > return PTR_ERR(map); > } > > diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c > index 28b8c805a293..4a1ff4560a8a 100644 > --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c > +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c > @@ -121,8 +121,8 @@ > "test12 ld_imm64", > .insns = { > BPF_MOV64_IMM(BPF_REG_1, 0), > - BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1), > - BPF_RAW_INSN(0, 0, 0, 0, 1), > + BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 999), > + BPF_RAW_INSN(0, 0, 0, 0, 0), > BPF_EXIT_INSN(), > }, > .errstr = "not pointing to valid bpf_map", > @@ -139,3 +139,16 @@ > .errstr = "invalid bpf_ld_imm64 insn", > .result = REJECT, > }, > +{ > + "test14 ld_imm64: reject 2nd imm != 0", > + .insns = { > + BPF_MOV64_IMM(BPF_REG_0, 0), > + BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, BPF_REG_1, > + BPF_PSEUDO_MAP_FD, 0, 0), > + BPF_RAW_INSN(0, 0, 0, 0, 0xfefefe), > + BPF_EXIT_INSN(), > + }, > + .fixup_map_hash_48b = { 1 }, > + .errstr = "unrecognized bpf_ld_imm64 insn", > + .result = REJECT, > +}, > -- > 2.17.1 >