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

[Cupertino Miranda <cupertino.miranda@oracle.com>]

Eduard Zingerman writes:

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

With the last patch and the suggestions of Andrii this code gets
removed.
Should we we keep having this small changes? :-)

Also the function was left like this on purpose since the original idea
was that it could be used multiple times for different registers and only
verified once, after calling for both src and dst.
It was in the context to verify that either the src or dst in MUL was a
const. That was further relaxed and aagain with the last patch it
removes the argument completelly.

Hope that it is Ok.


>> +	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;
>> +}
>
> [...]