On Wed, Nov 22, 2023 at 7:13 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Tue, 2023-11-21 at 17:16 -0800, Andrii Nakryiko wrote:
> > Instead of relying on potentially imprecise tnum representation of
> > expected return value range for callbacks and subprogs, validate that
> > both tnum and umin/umax range satisfy exact expected range of return
> > values.
> >
> > E.g., if callback would need to return [0, 2] range, tnum can't
> > represent this precisely and instead will allow [0, 3] range. By
> > additionally checking umin/umax range, we can make sure that
> > subprog/callback indeed returns only valid [0, 2] range.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> (but please see a question below)
>
> [...]
>
> > @@ -9464,6 +9477,16 @@ static bool in_rbtree_lock_required_cb(struct bpf_verifier_env *env)
> > return is_rbtree_lock_required_kfunc(kfunc_btf_id);
> > }
> >
> > +static bool retval_range_within(struct bpf_retval_range range, const struct bpf_reg_state *reg)
> > +{
> > + struct tnum trange = retval_range_as_tnum(range);
> > +
> > + if (!tnum_in(trange, reg->var_off))
> > + return false;
>
> Q: When is it necessary to do this check?
> I tried commenting it and test_{verifier,progs} still pass.
> Are there situations when umin/umax change is not sufficient?
I believe not. But we still check tnum in check_cond_jmp_op, for
example, so I decided to keep it to not have to argue and prove why
it's ok to ditch tnum.
Generally speaking, I think tnum is useful in only one use case:
checking (un)aligned memory accesses. This is the only representation
that can make sure we have lower 2-3 bits as zero to prove that memory
access is 4- or 8-byte aligned.
Other than this, I think ranges are more precise and easier to work with.
But I'm not ready to go on the quest to eliminate tnum usage everywhere :)
>
> > +
> > + return range.minval <= reg->umin_value && reg->umax_value <= range.maxval;
> > +}
> > +
>
> [...]
>
>
