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?
Then the question is whether to do this is_scalar check inside
reg_set_min_max() or outside. Both options are probably fine.
>
> + /* 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 ...
> */
> 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.
