Hi Yonghong,
Thanks for the reviews. I will prepare a patch series with your
recomendations soon.
> On 4/11/24 10:37 AM, Cupertino Miranda wrote:
>> MUL instruction required that src_reg would be a known value (i.e.
>> src_reg would be evaluate as a const value). The condition in this case
>> can be relaxed, since multiplication is a commutative operator and the
>> range computation is still valid if at least one of its registers is
>> known.
>>
>> BPF self-tests were added to check the new functionality.
>>
>> Signed-off-by: Cupertino Miranda <cupertino.miranda@xxxxxxxxxx>
>> ---
>> kernel/bpf/verifier.c | 10 +-
>> .../selftests/bpf/progs/verifier_bounds.c | 99 +++++++++++++++++++
>> 2 files changed, 105 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 7894af2e1bdb..a326ec024d82 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -13741,15 +13741,17 @@ static bool is_const_reg_and_valid(struct bpf_reg_state reg, bool alu32,
>> }
>> static bool is_safe_to_compute_dst_reg_ranges(struct bpf_insn *insn,
>> + struct bpf_reg_state dst_reg,
>> struct bpf_reg_state src_reg)
>> {
>> - bool src_known;
>> + bool src_known, dst_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);
>> + dst_known = is_const_reg_and_valid(dst_reg, alu32, &valid_known);
>
> Is it a possible the above could falsely reject some operation since
> in the original code, dst_reg is not checked here?
I guess, I believe in the case where min/max information is not matching
tnum value/mask, when the value is known.
My thoguht was that there would be no arm, since it could not
rely on that known value to compute MUL ranges anyway.
Still, I just realized this applies for all the other expressions, so
indeed it is rejecting and need some more patching.
>> /* Taint dst register if offset had invalid bounds
>> * derived from e.g. dead branches.
>> @@ -13765,10 +13767,10 @@ static bool is_safe_to_compute_dst_reg_ranges(struct bpf_insn *insn,
>> case BPF_OR:
>> return true;
>> - /* Compute range for MUL if the src_reg is known.
>> + /* Compute range for MUL if at least one of its registers is know.
>
> know => known
>
>> */
>> case BPF_MUL:
>> - return src_known;
>> + return src_known || dst_known;
>> /* Shift operators range is only computable if shift dimension operand
>> * is known. Also, shifts greater than 31 or 63 are undefined. This
>> @@ -13799,7 +13801,7 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
>> bool alu32 = (BPF_CLASS(insn->code) != BPF_ALU64);
>> int ret;
>> - if (!is_safe_to_compute_dst_reg_ranges(insn, src_reg)) {
>> + if (!is_safe_to_compute_dst_reg_ranges(insn, *dst_reg, src_reg)) {
>> __mark_reg_unknown(env, dst_reg);
>> return 0;
>> }
>> diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds.c b/tools/testing/selftests/bpf/progs/verifier_bounds.c
>
> Let us break this commit into two patches: verifier change and selftest change. This will make possible backport easier.
>
>> index 2fcf46341b30..09bb1b270ca7 100644
>> --- a/tools/testing/selftests/bpf/progs/verifier_bounds.c
>> +++ b/tools/testing/selftests/bpf/progs/verifier_bounds.c
>> @@ -949,6 +949,105 @@ l1_%=: r0 = 0; \
>> : __clobber_all);
>> }
>>
> [...]
