On Tue, Jan 2, 2024 at 2:45 PM Maciej Żenczykowski
<zenczykowski@xxxxxxxxx> wrote:
>
> On Tue, Jan 2, 2024 at 1:46 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Fri, Dec 29, 2023 at 5:31 PM Maciej Żenczykowski
> > <zenczykowski@xxxxxxxxx> wrote:
> > >
> > > I have a relatively complex program that fails to load on 6.5.6 with a
> > >
> > > if (data + 98 != data_end) return TC_ACT_SHOT;
> > >
> >
> > How realistic is such code in practice? Is there a situation in which
> > it's critical to ensure that the packet has exactly X bytes in [data,
> > data_end) range? Even in that case we can have data in frags, though,
> > right? So I'm just wondering if we are discussing some rather
> > theoretical situation?
>
> So, as I mentioned I hit this while debugging some other complex code,
> so the example 98 isn't a particularly good value
> (when I actually hit this I think I was trying to match some ping packets).
>
> However, elsewhere in the same program I need to match and reply to
> IPv6 NS packets
> (for an IPv6 address the kernel doesn't own, to workaround a pair of
> kernel bugs / missing features in ipv6 neigh proxy).
>
> In practice the NS I receive and need to handle are always:
> 14 ethernet + 40 ipv6 + 8 icmp6 + 16 target + 8 byte link address
> option = 86 bytes long
> (and if they're not, then my current code can't parse them anyway)
> so it's natural to do something like:
>
> handle_ns_with_laddr(struct __sk_buff *skb) {
> if (skb->data + 86 != skb->data_end) return;
> struct ethernet_ipv6_ns *pkt = data;
> if (pkt->ether.h_proto != htons(ETH_P_IPV6)) return;
> if (pkt->ip6.nexthdr != IPPROTO_ICMPV6) return;
> ...etc...
> }
>
> Yeah, there's lots of caveats in the above (lack of pull, etc), but it
> is enough to handle the case I need handled...
Yeah, that's what I was getting at, just using data_end as a marker
for packet length seems incorrect. But I do see the point that it's
just an unnecessary complication for users to work around.
For the fix that Eduard proposed (and checking
try_match_pkt_pointers), should we do a similar simplification as we
do for scalar register comparisons? Make sure that data_end is always
on the right by swapping, if that's not the case. And also use
corresponding rev_opcode() and flip_opcode() operations to minimize
the amount of logic and duplicated code?
And I mean not just for new JEQ/JNE cases, but let's also refactor and
simplify existing logic as well?
>
> Obviously I could rewrite the above as:
> if (skb->data + 86 > skb->data_end) return;
> if (skb->data + 86 < skb->data_end) return;
>
> though I guess a too smart compiler could optimize that back down to == ...
>
> > > check, that loads fine if I change the above != to (a you would think
> > > weaker) > check.
> > >
> > > It's not important, hit this while debugging, and I don't know if the
> > > cause is the verifier treating != differently than > or the compiler
> > > optimizing != somehow... but my gut feeling is on the former: some
> > > verifier logic special cases > without doing something similar for the
> > > stronger != comparison.
> > >
> > > ...
> > > 453: (85) call bpf_trace_printk#6 ; R0_w=scalar()
> > > ; if (data + 98 != data_end) return TC_ACT_SHOT;
> > > 454: (bf) r1 = r6 ; R1_w=pkt(off=0,r=42,imm=0)
> > > R6=pkt(off=0,r=42,imm=0)
> > > 455: (07) r1 += 98 ; R1_w=pkt(off=98,r=42,imm=0)
> > > ; if (data + 98 != data_end) return TC_ACT_SHOT;
> > > 456: (5d) if r1 != r9 goto pc-23 ; R1_w=pkt(off=98,r=42,imm=0)
> > > R9=pkt_end(off=0,imm=0)
> > > *** IMHO here r=42 should be bumped to 98 ***
> > > 457: (bf) r3 = r6 ; R3_w=pkt(off=0,r=42,imm=0)
> > > R6=pkt(off=0,r=42,imm=0)
> > > 458: (07) r3 += 34 ; R3_w=pkt(off=34,r=42,imm=0)
> > > ; uint64_t cs = bpf_csum_diff(NULL, 0, data + 14 + 20, 98 - 14 - 20, 0xFFFF);
> > > 459: (b7) r1 = 0 ; R1_w=0
> > > 460: (b7) r2 = 0 ; R2_w=0
> > > 461: (b7) r4 = 64 ; R4_w=64
> > > 462: (b7) r5 = 65535 ; R5_w=65535
> > > 463: (85) call bpf_csum_diff#28
> > > invalid access to packet, off=34 size=64, R3(id=0,off=34,r=42)
> > > R3 offset is outside of the packet
> > >
> > > Side note: bpf_csum_diff() is super non user-friendly, but that's for
> > > another thread...
> > >
> > > Happy New Year,
> > > Maciej
> > >
