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
