On Tue, May 24, 2022 at 12:11 AM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
>
> 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;
> + }
> + }
> +
This makes the code worse.
There is no need for these patches.
