On Thu, 2025-10-23 at 10:38 -0700, Eduard Zingerman wrote:
On Thu, 2025-10-23 at 19:26 +0800, KaFai Wan wrote:
[...]
@@ -16173,6 +16173,25 @@ static int is_pkt_ptr_branch_taken(struct bpf_reg_state *dst_reg, static int is_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_state *reg2, u8 opcode, bool is_jmp32) { + if (reg1 == reg2) { + switch (opcode) { + case BPF_JGE: + case BPF_JLE: + case BPF_JSGE: + case BPF_JSLE: + case BPF_JEQ: + case BPF_JSET:
Others are fine, but BPF_JSET on the same register could be 0 (if value is 0). And it's unknown to take the branch if 0 within the range.
Right, missed that one.
+ return 1; + case BPF_JGT: + case BPF_JLT: + case BPF_JSGT: + case BPF_JSLT: + case BPF_JNE: + return 0; + default: + return -1; + } + }
But that's too much code for an artificial case. Idk, either way is fine with me.
There is is_scalar_branch_taken() in is_branch_taken(), I missed it. I'll a) check the opcode one by one in is_scalar_branch_taken(), and b) keep this patch for unknown BPF_JSET branch.
Sounds good to me. Note that the logic is correct for both scalar and non-scalar cases, so I don't think we have to constrain it to is_scalar_branch_taken() (don't think there is a need to check if pointer comparisons are allowed, as no new information is inferred from comparisons with self).
For non-scalar cases we only allow pointer comparison on pkt_ptr, this check is before is_branch_taken()
src_reg = ®s[insn->src_reg]; if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) && is_pointer_value(env, insn->src_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg); return -EACCES; }
and in the end of check_cond_jmp_op() (after is_branch_taken()), we checked again
} else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg], this_branch, other_branch) && is_pointer_value(env, insn->dst_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->dst_reg); return -EACCES; }
this time we check if it is valid comparison on pkt_ptr in try_match_pkt_pointers().
Currently we just allow 4 opcode (BPF_JGT, BPF_JLT, BPF_JGE, BPF_JLE) on pkt_ptr, and with conditions. But we bypass these prohibits in privileged mode (is_pointer_value() always return false in privileged mode).
So the logic skip these prohibits for pkt_ptr in unprivileged mode.