On Wed, Nov 29, 2023 at 9:23 PM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
>
> 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@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.
>
> 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).
I tried it locally, and I don't have to adjust any new tests, so I'll
inline tnum_range() as you suggested, thanks.
>
> 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);
