Re: [PATCH bpf-next v4 2/7] bpf/verifier: refactor checks for range computation

[Eduard Zingerman <eddyz87@gmail.com>]

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6fe641c8ae33..1777ab00068b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13695,6 +13695,77 @@ static void scalar_min_max_arsh(struct bpf_reg_state *dst_reg,
>  	__update_reg_bounds(dst_reg);
>  }
>  
> +static bool is_const_reg_and_valid(const struct bpf_reg_state *reg, bool alu32,
> +				   bool *valid)
> +{
> +	s64 smin_val = reg->smin_value;
> +	s64 smax_val = reg->smax_value;
> +	u64 umin_val = reg->umin_value;
> +	u64 umax_val = reg->umax_value;
> +	s32 s32_min_val = reg->s32_min_value;
> +	s32 s32_max_val = reg->s32_max_value;
> +	u32 u32_min_val = reg->u32_min_value;
> +	u32 u32_max_val = reg->u32_max_value;
> +	bool is_const = alu32 ? tnum_subreg_is_const(reg->var_off) :
> +				tnum_is_const(reg->var_off);
> +

Nit:
Sorry for missing this earlier, should we initialize 'valid' here? e.g.:

	*valid = true;

I understand that it is initialized upper in the stack,
but setting it here seems better.

> +	if (alu32) {
> +		if ((is_const &&
> +		     (s32_min_val != s32_max_val || u32_min_val != u32_max_val)) ||
> +		      s32_min_val > s32_max_val || u32_min_val > u32_max_val)
> +			*valid = false;
> +	} else {
> +		if ((is_const &&
> +		     (smin_val != smax_val || umin_val != umax_val)) ||
> +		    smin_val > smax_val || umin_val > umax_val)
> +			*valid = false;
> +	}
> +
> +	return is_const;
> +}

[...]