On Wed, Nov 22, 2023 at 09:45:27AM -0800, Andrii Nakryiko wrote:
> 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)
> >
> > [...]
> >
> > > +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.
Semi-related proof[1] from awhile back :)
> 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.
Agree with the above.
I'd vote for ditching tnum for the retval check here. With umin/umax
check in place there really isn't a need for an additional tnum check at
all. Keeping it probably does more harm (in the form of confusion) than
good.
1: https://lore.kernel.org/bpf/20220831031907.16133-3-shung-hsi.yu@xxxxxxxx/
> 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;
> > > +}
> > > +
> >
> > [...]
