Messages in this thread | | | Date | Fri, 20 May 2022 16:50:21 -0700 | Subject | Re: [PATCH bpf-next 2/4] bpf: verifier: explain opcode check in check_ld_imm() | From | Yonghong Song <> |
| |
On 5/20/22 4:37 AM, Shung-Hsi Yu wrote: > The BPF_SIZE check in the beginning of check_ld_imm() actually guard > against program with JMP instructions that goes to the second > instruction of BPF_LD_IMM64, but may be easily dismissed as an simple > opcode check that's duplicating the effort of bpf_opcode_in_insntable(). > > Add comment to better reflect the importance of the check. > > Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com> > --- > kernel/bpf/verifier.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 79a2695ee2e2..133929751f80 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -9921,6 +9921,10 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn) > struct bpf_map *map; > int err; > > + /* checks that this is not the second part of BPF_LD_IMM64, which is > + * skipped over during opcode check, but a JMP with invalid offset may > + * cause check_ld_imm() to be called upon it. > + */
The check_ld_imm() call context is:
} else if (class == BPF_LD) { u8 mode = BPF_MODE(insn->code);
if (mode == BPF_ABS || mode == BPF_IND) { err = check_ld_abs(env, insn); if (err) return err;
} else if (mode == BPF_IMM) { err = check_ld_imm(env, insn); if (err) return err;
env->insn_idx++; sanitize_mark_insn_seen(env); } else { verbose(env, "invalid BPF_LD mode\n"); return -EINVAL; } }
which is a normal checking of LD_imm64 insn.
I think the to-be-added comment is incorrect and unnecessary.
> if (BPF_SIZE(insn->code) != BPF_DW) { > verbose(env, "invalid BPF_LD_IMM insn\n"); > return -EINVAL;
| |