On Tue, May 24, 2022 at 08:12:24AM -0700, Alexei Starovoitov wrote:
> 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.
Could you elaborate a bit more on the reason? I'd like to try avoid
submitting patch like this in the future.
In hindsight I'd guess it's because it adds more branching into do_check()
and more lines of code, making it harder to understand, but at the same time
the added checks is mostly repeating existing
1. insn_cnt check in the beginning of do_check() loop
2. BPF_SIZE check in check_ld_imm()
While it has the benefit of adding more specific error message, such error
message is too specific to be useful in general, thus does not outweigh the
cost of added complexity?
> There is no need for these patches.
Just to be clear, "these" refers to the added checks do_check()
<Yoxjvvm9poTC3Atv@syu-laptop> only, or does it also includes the to-be-added
comment inside check_ld_imm() of patch 2?
