Yonghong Song writes:
> On 4/11/24 10:37 AM, Cupertino Miranda wrote:
>> Split range computation checks in its own function, isolating pessimitic
>> range set for dst_reg and failing return to a single point.
>>
>> Signed-off-by: Cupertino Miranda <cupertino.miranda@xxxxxxxxxx>
>> ---
>> kernel/bpf/verifier.c | 141 +++++++++++++++++++++++-------------------
>> 1 file changed, 77 insertions(+), 64 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index a219f601569a..7894af2e1bdb 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -13709,6 +13709,82 @@ static void scalar_min_max_arsh(struct bpf_reg_state *dst_reg,
>> __update_reg_bounds(dst_reg);
>> }
>> +static bool is_const_reg_and_valid(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 known = alu32 ? tnum_subreg_is_const(reg.var_off) :
>> + tnum_is_const(reg.var_off);
>> +
>> + if (alu32) {
>> + if ((known &&
>> + (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;
>
> *valid = false;
>
>> + } else {
>> + if ((known &&
>> + (smin_val != smax_val || umin_val != umax_val)) ||
>> + smin_val > smax_val || umin_val > umax_val)
>> + *valid &= false;
>
> *valid = false;
>
>> + }
>> +
>> + return known;
>> +}
>> +
>> +static bool is_safe_to_compute_dst_reg_ranges(struct bpf_insn *insn,
>> + struct bpf_reg_state src_reg)
>> +{
>> + bool src_known;
>> + u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32;
>> + bool alu32 = (BPF_CLASS(insn->code) != BPF_ALU64);
>> + u8 opcode = BPF_OP(insn->code);
>> +
>> + bool valid_known = true;
>> + src_known = is_const_reg_and_valid(src_reg, alu32, &valid_known);
>> +
>> + /* Taint dst register if offset had invalid bounds
>> + * derived from e.g. dead branches.
>> + */
>> + if (valid_known == false)
>> + return false;
>> +
>> + switch (opcode) {
>> + case BPF_ADD:
>> + case BPF_SUB:
>> + case BPF_AND:
>> + case BPF_XOR:
>> + case BPF_OR:
>> + return true;
>> +
>> + /* Compute range for MUL if the src_reg is known.
>> + */
>> + case BPF_MUL:
>> + return src_known;
>> +
>> + /* Shift operators range is only computable if shift dimension operand
>> + * is known. Also, shifts greater than 31 or 63 are undefined. This
>> + * includes shifts by a negative number.
>> + */
>> + case BPF_LSH:
>> + case BPF_RSH:
>> + case BPF_ARSH:
>> + return src_known && (src_reg.umax_value < insn_bitness);
>> + default:
>> + break;
>> + }
>> +
>> + return false;
>> +}
>> +
>> /* WARNING: This function does calculations on 64-bit values, but the actual
>> * execution may occur on 32-bit values. Therefore, things like bitshifts
>> * need extra checks in the 32-bit case.
>> @@ -13720,52 +13796,10 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
>> {
>> struct bpf_reg_state *regs = cur_regs(env);
>> u8 opcode = BPF_OP(insn->code);
>> - bool src_known;
>> - s64 smin_val, smax_val;
>> - u64 umin_val, umax_val;
>> - s32 s32_min_val, s32_max_val;
>> - u32 u32_min_val, u32_max_val;
>> - u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32;
>> bool alu32 = (BPF_CLASS(insn->code) != BPF_ALU64);
>> int ret;
>> - smin_val = src_reg.smin_value;
>> - smax_val = src_reg.smax_value;
>> - umin_val = src_reg.umin_value;
>> - umax_val = src_reg.umax_value;
>> -
>> - s32_min_val = src_reg.s32_min_value;
>> - s32_max_val = src_reg.s32_max_value;
>> - u32_min_val = src_reg.u32_min_value;
>> - u32_max_val = src_reg.u32_max_value;
>> -
>> - if (alu32) {
>> - src_known = tnum_subreg_is_const(src_reg.var_off);
>> - if ((src_known &&
>> - (s32_min_val != s32_max_val || u32_min_val != u32_max_val)) ||
>> - s32_min_val > s32_max_val || u32_min_val > u32_max_val) {
>> - /* Taint dst register if offset had invalid bounds
>> - * derived from e.g. dead branches.
>> - */
>> - __mark_reg_unknown(env, dst_reg);
>> - return 0;
>> - }
>> - } else {
>> - src_known = tnum_is_const(src_reg.var_off);
>> - if ((src_known &&
>> - (smin_val != smax_val || umin_val != umax_val)) ||
>> - smin_val > smax_val || umin_val > umax_val) {
>> - /* Taint dst register if offset had invalid bounds
>> - * derived from e.g. dead branches.
>> - */
>> - __mark_reg_unknown(env, dst_reg);
>> - return 0;
>> - }
>> - }
>> -
>> - if (!src_known &&
>> - opcode != BPF_ADD && opcode != BPF_SUB && opcode != BPF_AND &&
>> - opcode != BPF_XOR && opcode != BPF_OR) {
>> + if (!is_safe_to_compute_dst_reg_ranges(insn, src_reg)) {
>> __mark_reg_unknown(env, dst_reg);
>
> This is not a precise refactoring. there are some cases like below
> which uses mark_reg_unknow().
Oh, right I miss those underscores above. :(
Will make sure to cover that.
>
> Let us put the refactoring patch as the first patch in the serious and all
> additional changes after that and this will make it easy to review.
>
>> return 0;
>> }
>> @@ -13822,39 +13856,18 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
>> scalar_min_max_xor(dst_reg, &src_reg);
>> break;
>> case BPF_LSH:
>> - if (umax_val >= insn_bitness) {
>> - /* Shifts greater than 31 or 63 are undefined.
>> - * This includes shifts by a negative number.
>> - */
>> - mark_reg_unknown(env, regs, insn->dst_reg);
>> - break;
>> - }
>> if (alu32)
>> scalar32_min_max_lsh(dst_reg, &src_reg);
>> else
>> scalar_min_max_lsh(dst_reg, &src_reg);
>> break;
>> case BPF_RSH:
>> - if (umax_val >= insn_bitness) {
>> - /* Shifts greater than 31 or 63 are undefined.
>> - * This includes shifts by a negative number.
>> - */
>> - mark_reg_unknown(env, regs, insn->dst_reg);
>> - break;
>> - }
>> if (alu32)
>> scalar32_min_max_rsh(dst_reg, &src_reg);
>> else
>> scalar_min_max_rsh(dst_reg, &src_reg);
>> break;
>> case BPF_ARSH:
>> - if (umax_val >= insn_bitness) {
>> - /* Shifts greater than 31 or 63 are undefined.
>> - * This includes shifts by a negative number.
>> - */
>> - mark_reg_unknown(env, regs, insn->dst_reg);
>> - break;
>> - }
>> if (alu32)
>> scalar32_min_max_arsh(dst_reg, &src_reg);
>> else
