Re: [PATCH v2 bpf-next 03/10] bpf: enforce exact retval range on subprog/callback exit

[Shung-Hsi Yu <shung-hsi.yu@suse.com>]

On Wed, Nov 29, 2023 at 08:23:38AM -0800, Andrii Nakryiko wrote:
> On Wed, Nov 29, 2023 at 2:56 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote:
> > On Tue, Nov 28, 2023 at 04:36:13PM -0800, Andrii Nakryiko wrote:
> > > Instead of relying on potentially imprecise tnum representation of
> > > expected return value range for callbacks and subprogs, validate that
> > > 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
> > > checking umin/umax range, we can make sure that subprog/callback indeed
> > > returns only valid [0, 2] range.
> > >
> > > Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  include/linux/bpf_verifier.h |  7 ++++++-
> > >  kernel/bpf/verifier.c        | 40 ++++++++++++++++++++++++++----------
> > >  2 files changed, 35 insertions(+), 12 deletions(-)
> >
> > ...
> >
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -9560,6 +9565,19 @@ 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)
> > > +{
> > > +     return range.minval <= reg->umin_value && reg->umax_value <= range.maxval;
> > > +}
> > > +
> > > +static struct tnum retval_range_as_tnum(struct bpf_retval_range range)
> > > +{
> > > +     if (range.minval == range.maxval)
> > > +             return tnum_const(range.minval);
> > > +     else
> > > +             return tnum_range(range.minval, range.maxval);
> > > +}
> >
> > Nit: find it slightly strange to have retval_range_as_tnum() added here
> > (patch 3), only to be removed again in the patch 5. As far as I can see
> > patch 4 doesn't require this, and it is only used once.
> >
> > Perhaps just replace its use below with tnum_range() instead? (Not
> > pretty, but will be removed anyway).
> 
> I do this to delay the refactoring of verbose_invalid_scalar() which
> is used by another piece of logic which I refactor in a separate
> patch. If I don't do this temporary retval_range_as_tnum() helper, I
> might need to update some more tests that expect exact var_off value
> in logs, and I didn't want to do it. Given it's a trivial helper, it
> feels like it's not a big deal to keep it for a patch or two before
> completing the refactoring.

Replace retval_range_as_tnum(callee->callback_ret_range) with 

  tnum_range(callee->callback_ret_range.minval,
             callee->callback_ret_range.maxval)

and the verbose_invalid_scalar() signature stays the same; also no var_off
changes because it is just manual inline of retval_range_as_tnum(), as
tnum_range(n, n) == tnum_const(n).

Agree it really is not a big deal, so I won't insist on it.

> > > @@ -9597,7 +9612,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
> > >               if (err)
> > >                       return err;
> > >
> > > -             if (!tnum_in(range, r0->var_off)) {
> > > +             /* enforce R0 return value range */
> > > +             if (!retval_range_within(callee->callback_ret_range, r0)) {
> > > +                     struct tnum range = retval_range_as_tnum(callee->callback_ret_range);