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).
> @@ -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);
