On Mon, Nov 27, 2023 at 2:55 AM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
>
> 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.
Ok, I think it's as trivial as removing two lines of code. So I'll add
it as the last patch to the series and will let BPF maintainers decide
if they want this change or not.
>
> 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;
> > > > +}
> > > > +
> > >
> > > [...]
