Re: [PATCH bpf-next v2 1/5] bpf/verifier: refactor checks for range computation

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-04-17 at 13:23 +0100, Cupertino Miranda wrote:
[...]

> @@ -13406,53 +13490,19 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,

[...]

> -	if (!src_known &&
> -	    opcode != BPF_ADD && opcode != BPF_SUB && opcode != BPF_AND) {
> +	int is_safe = is_safe_to_compute_dst_reg_range(insn, src_reg);
> +	switch (is_safe) {
> +	case UNCOMPUTABLE_RANGE:
>  		__mark_reg_unknown(env, dst_reg);
>  		return 0;
> +	case UNDEFINED_BEHAVIOUR:
> +		mark_reg_unknown(env, regs, insn->dst_reg);
> +		return 0;
> +	default:
> +		break;
>  	}

Nit: I know that the division between __mark_reg_unknown() and
mark_reg_unknown() was asked for directly, but tbh I don't think that
it adds any value here, here is how mark_reg_unknown() is implemented:

static void mark_reg_unknown(struct bpf_verifier_env *env,
			     struct bpf_reg_state *regs, u32 regno)
{
	if (WARN_ON(regno >= MAX_BPF_REG)) {
		... mark all regs not init ...
		return;
    }
	__mark_reg_unknown(env, regs + regno);
}

The 'regno >= MAX_BPF_REG' does not apply here, because
adjust_scalar_min_max_vals() is only called from the following stack:
- check_alu_op
  - adjust_reg_min_max_vals
    - adjust_scalar_min_max_vals

The check_alu_op() does check_reg_arg() which verifies that both src
and dst register numbers are within bounds.

I suggest to replace the enum with as boolean value.
Miranda, Yonhong, what do you think?

[...]