Messages in this thread | | | Date | Fri, 20 May 2022 17:25:36 -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:50 PM, Yonghong Song wrote: > > > 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.
Okay, double check again and now I understand what happens when hitting the second insn of ldimm64 with a branch target. Here we have BPF_LD = 0 and BPF_IMM = 0, so for a branch target to the 2nd part of ldimm64, it will come to check_ld_imm() and have error "invalid BPF_LD_IMM insn"
So check_ld_imm() is to check whether the insn is a *legal* insn for the first part of ldimm64.
So the comment may be rewritten as below.
This is to verify whether an insn is a BPF_LD_IMM64 or not. But since BPF_LD = 0 and BPF_IMM = 0, if the branch target comes to the second part of BPF_LD_IMM64, the control may come here as well.
> >> if (BPF_SIZE(insn->code) != BPF_DW) { >> verbose(env, "invalid BPF_LD_IMM insn\n"); >> return -EINVAL;
| |