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:
* 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,
and fixup the comment in check_cond_jmp_op() where reg_set_min_max().
