On Tue, Jan 2, 2024 at 11:24 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Tue, 2024-01-02 at 10:30 -0800, Maciej Żenczykowski wrote:
> [...]
> >
> > The check is:
> > if (data + 98 != data_end) return;
> > so now (after this check) you *know* that 'data + 98 == data_end' and
> > thus you know there are *exactly* 98 valid bytes.
>
> Apologies, you are correct.
> So you want to have something along the following lines:
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a376eb609c41..6ddb34d5b9aa 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14712,6 +14712,28 @@ static bool try_match_pkt_pointers(const struct bpf_insn *insn,
> return false;
> }
> break;
> + case BPF_JEQ:
> + case BPF_JNE:
> + if ((dst_reg->type == PTR_TO_PACKET &&
> + src_reg->type == PTR_TO_PACKET_END) ||
> + (dst_reg->type == PTR_TO_PACKET_META &&
> + reg_is_init_pkt_pointer(src_reg, PTR_TO_PACKET)) ||
> + (dst_reg->type == PTR_TO_PACKET_END &&
> + src_reg->type == PTR_TO_PACKET) ||
> + (reg_is_init_pkt_pointer(dst_reg, PTR_TO_PACKET) &&
> + src_reg->type == PTR_TO_PACKET_META)) {
> + /* pkt_data' != pkt_end, pkt_meta' != pkt_data,
> + * pkt_end != pkt_data', pkt_data != pkt_meta'
> + */
> + struct bpf_verifier_state *eq_branch;
> +
> + eq_branch = BPF_OP(insn->code) == BPF_JEQ ? other_branch : this_branch;
> + find_good_pkt_pointers(eq_branch, dst_reg, dst_reg->type, true);
> + mark_pkt_end(eq_branch, insn->dst_reg, false);
> + } else {
> + return false;
> + }
> + break;
> case BPF_JGE:
> if ((dst_reg->type == PTR_TO_PACKET &&
> src_reg->type == PTR_TO_PACKET_END) ||
>
> Right?
I think so? I don't fully follow the logic.
I wonder if JEQ/JNE couldn't simply be folded into the existing cases though...
Or perhaps some other refactoring to tri-state the jmps...
switch (opcode) {
case BPF_JEQ: eq_branch = this_branch; lt_branch = gt_branch =
other_branch; break;
case BPF_JNE: lt_branch = gt_branch = this_branch; eq_branch =
other_branch; break;
case BPF_LT: lt_branch = this_branch; eq_branch = gt_branch =
other_branch; break;
...
}
and then you can ignore opcode...
> This passes the following test case:
>
> SEC("tc")
> __success
> __naked void data_plus_const_eq_pkt_end(void)
> {
> asm volatile (" \n\
> r9 = r1; \n\
> r1 = *(u32*)(r9 + %[__sk_buff_data]); \n\
> r2 = *(u32*)(r9 + %[__sk_buff_data_end]); \n\
> r3 = r1; \n\
> r3 += 8; \n\
> if r3 != r2 goto 1f; \n\
> r1 = *(u64 *)(r1 + 0); \n\
> 1: \n\
> r0 = 0; \n\
> exit; \n\
> " :
> : __imm_const(__sk_buff_data, offsetof(struct __sk_buff, data)),
> __imm_const(__sk_buff_data_end, offsetof(struct __sk_buff, data_end))
> : __clobber_all);
> }
>
> And it's variations for EQ/NEQ, positive/negative.
