On Tue, Oct 31, 2023 at 10:50 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Oct 31, 2023 at 9:24 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 30, 2023 at 11:03 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 30, 2023 at 7:02 PM Alexei Starovoitov
> > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Oct 27, 2023 at 11:13:40AM -0700, Andrii Nakryiko wrote:
> > > > > static void reg_set_min_max(struct bpf_reg_state *true_reg1,
> > > > > + struct bpf_reg_state *true_reg2,
> > > > > struct bpf_reg_state *false_reg1,
> > > > > - u64 val, u32 val32,
> > > > > + struct bpf_reg_state *false_reg2,
> > > > > u8 opcode, bool is_jmp32)
> > > > > {
> > > > > - struct tnum false_32off = tnum_subreg(false_reg1->var_off);
> > > > > - struct tnum false_64off = false_reg1->var_off;
> > > > > - struct tnum true_32off = tnum_subreg(true_reg1->var_off);
> > > > > - struct tnum true_64off = true_reg1->var_off;
> > > > > - s64 sval = (s64)val;
> > > > > - s32 sval32 = (s32)val32;
> > > > > -
> > > > > - /* If the dst_reg is a pointer, we can't learn anything about its
> > > > > - * variable offset from the compare (unless src_reg were a pointer into
> > > > > - * the same object, but we don't bother with that.
> > > > > - * Since false_reg1 and true_reg1 have the same type by construction, we
> > > > > - * only need to check one of them for pointerness.
> > > > > + struct tnum false_32off, false_64off;
> > > > > + struct tnum true_32off, true_64off;
> > > > > + u64 val;
> > > > > + u32 val32;
> > > > > + s64 sval;
> > > > > + s32 sval32;
> > > > > +
> > > > > + /* If either register is a pointer, we can't learn anything about its
> > > > > + * variable offset from the compare (unless they were a pointer into
> > > > > + * the same object, but we don't bother with that).
> > > > > */
> > > > > - if (__is_pointer_value(false, false_reg1))
> > > >
> > > > The removal of the above check, but not the comment was surprising and concerning,
> > > > so I did a bit of git-archaeology.
> > > > It was added in commit f1174f77b50c ("bpf/verifier: rework value tracking")
> > > > back in 2017 !
> > > > and in that commit reg_set_min_max() was always called with reg == scalar.
> > > > It looked like premature check. Then I spotted a comment in that commit:
> > > > * this is only legit if both are scalars (or pointers to the same
> > > > * object, I suppose, but we don't support that right now), because
> > > > * otherwise the different base pointers mean the offsets aren't
> > > > * comparable.
> > > > so the intent back then was to generalize reg_set_min_max() to be used with pointers too,
> > > > but we never got around to do that and the comment now reads:
> > >
> > > Yeah, it shouldn't be too hard to "generalize" to pointer vs pointer,
> > > if we ensure they point to exactly the same thing (I haven't thought
> > > much about how), because beyond that it's still basically SCALAR
> > > offsets. But I figured it's out of scope for these changes :)
> > >
> > > > * this is only legit if both are scalars (or pointers to the same
> > > > * object, I suppose, see the PTR_MAYBE_NULL related if block below),
> > > > * because otherwise the different base pointers mean the offsets aren't
> > > > * comparable.
> > > >
> > > > So please remove is_pointer check and remove the comment,
> > >
> > > So I'm a bit confused. I did remove __is_pointer_value() check, but I
> > > still need to guard against having pointers, which is why I have:
> > >
> > > if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE).
> > > return;
> > >
> > > I think I need this check, because reg_set_min_max() can be called
> > > from check_cond_jmp_op() with pointer regs, and we shouldn't try to
> > > adjust them. Or am I missing something? And the comment I have here
> > > now:
> >
> > I don't see a code path where reg_set_min_max() is called
> > with pointers. At least not in the current code base.
> > Are you saying somewhere in your later patch it happens?
> >
>
> Hm.. no, it's all in this patch. Check check_cond_jmp_op(). We at
> least allow `(reg_is_pkt_pointer_any(dst_reg) &&
> reg_is_pkt_pointer_any(src_reg)` case to get into is_branch_taken(),
> which, btw, does handle pointer x pointer, pointer x scalar, and
> scalar x scalar cases. Then, we go straight to reg_set_min_max(), both
> for BPF_X and BPF_K cases. So reg_set_min_max() has to guard itself
> against pointers.
Correction, BPF_K branch does check for dst_reg->type == SCALAR_VALUE.
But BPF_X doesn't. I stared at this code for so long that I don't even
notice those checks anymore :(
I'd rather drop this SCALAR check for the BPF_K case and keep
reg_set_min_max() as generic as is_branch_taken(), if that's ok. I
think it's less error-prone and a more consistent approach.
>
>
> > Then the question is whether to do this is_scalar check inside
> > reg_set_min_max() or outside. Both options are probably fine.
>
> Given we have two separate calls to reg_set_min_max(), BPF_X and
> BPF_K, it seems cleaner to do it once at the beginning of
> reg_set_min_max(). And if in the future we do support pointer
> variants, I'd handle them inside reg_set_min_max(), just like
> is_branch_taken() handles different situations in one place
> transparently to the caller.
>
> >
> > >
> > > + /* If either register is a pointer, we can't learn anything about its
> > > + * variable offset from the compare (unless they were a pointer into
> > > + * the same object, but we don't bother with that).
> > > */
> > >
> > > is trying to explain that we don't really adjust two pointers.
> > >
> > > > and fixup the comment in check_cond_jmp_op() where reg_set_min_max().
> > >
> > > I have this locally for now, please let me know if this is fine or you
> > > had something else in mind:
> > >
> > > -/* Adjusts the register min/max values in the case that the dst_reg is the
> > > - * variable register that we are working on, and src_reg is a constant or we're
> > > - * simply doing a BPF_K check.
> > > - * In JEQ/JNE cases we also adjust the var_off values.
> > > +/* Adjusts the register min/max values in the case that the dst_reg and
> > > + * src_reg are both SCALAR_VALUE registers (or we are simply doing a BPF_K
> > > + * check, in which case we havea fake SCALAR_VALUE representing insn->imm).
> > > + * Technically we can do similar adjustments for pointers to the same object,
> > > + * but we don't support that right now.
> >
> > Looks fine. I'm trying to say that we had such comment forever and
> > it never lead to actually doing the work.
> > So I'd just remove the last sentence about pointers ...
>
> Ah, ok, yep, sure.
>
> >
> > > */
> > > static void reg_set_min_max(struct bpf_reg_state *true_reg1,
> > > struct bpf_reg_state *true_reg2,
> > > @@ -14884,13 +14885,6 @@ static int check_cond_jmp_op(struct
> > > bpf_verifier_env *env,
> > > return -EFAULT;
> > > other_branch_regs = other_branch->frame[other_branch->curframe]->regs;
> > >
> > > - /* detect if we are comparing against a constant value so we can adjust
> > > - * our min/max values for our dst register.
> > > - * this is only legit if both are scalars (or pointers to the same
> > > - * object, I suppose, see the PTR_MAYBE_NULL related if block below),
> > > - * because otherwise the different base pointers mean the offsets aren't
> > > - * comparable.
> > > - */
> >
> > ... and removing this comment is good thing too.
> > In general the comments should be in front of the function body
> > (as you're doing) instead of the callsite.
>
> yep, sounds good
