Re: [PATCH bpf-next 2/4] bpf: verifier: explain opcode check in check_ld_imm()

[Shung-Hsi Yu]

On Fri, May 20, 2022 at 05:25:36PM -0700, Yonghong Song wrote:
> 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@xxxxxxxx>
> > > ---
> > >   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"

Yes, the 2nd instruction uses the reserved opcode 0, which could be
interpreted as BPF_LD | BPF_W | BPF_IMM.

> 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;

After giving it a bit more though, maybe it'd be clearer if we simply detect
such case in the JMP branch of do_check().

Something like this instead. Though I haven't tested yet, and it still check
the jump destination even it's a dead branch.

---
 kernel/bpf/verifier.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index aedac2ac02b9..59228806884e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12191,6 +12191,25 @@ static int do_check(struct bpf_verifier_env *env)
 			u8 opcode = BPF_OP(insn->code);
 
 			env->jmps_processed++;
+
+			/* check jump offset */
+			if (opcode != BPF_CALL && opcode != BPF_EXIT) {
+				u32 dst_insn_idx = env->insn_idx + insn->off + 1;
+				struct bpf_insn *dst_insn = &insns[dst_insn_idx];
+
+				if (dst_insn_idx > insn_cnt) {
+					verbose(env, "invalid JMP idx %d off %d beyond end of program insn_cnt %d\n", env->insn_idx, insn->off, insn_cnt);
+					return -EFAULT;
+				}
+				if (!bpf_opcode_in_insntable(dst_insn->code)) {
+					/* Should we simply tell the user that it's a
+					 * jump to the 2nd LD_IMM64 instruction
+					 * here? */
+					verbose(env, "idx %d JMP to idx %d with unknown opcode %02x\n", env->insn_idx, dst_insn_idx, insn->code);
+					return -EINVAL;
+				}
+			}
+
 			if (opcode == BPF_CALL) {
 				if (BPF_SRC(insn->code) != BPF_K ||
 				    (insn->src_reg != BPF_PSEUDO_KFUNC_CALL
-- 
2.36.1