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?
[...]
