On Wed, Nov 29, 2023 at 2:56 AM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > 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.
> > @@ -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);
