netdev - Re: [PATCH] net: fix wrong network header length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 08 Feb 2022 17:01:07 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Maciej Żenczykowski <maze@...gle.com>,
        Steffen Klassert <steffen.klassert@...unet.com>
Cc:     Lina Wang <lina.wang@...iatek.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Linux NetDev <netdev@...r.kernel.org>,
        Kernel hackers <linux-kernel@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Willem Bruijn <willemb@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH] net: fix wrong network header length

+ Steffen
On Tue, 2022-02-08 at 04:57 -0800, Maciej Żenczykowski wrote:
> On Tue, Feb 8, 2022 at 12:25 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > 
> > Hello,
> > 
> > On Tue, 2022-02-08 at 10:55 +0800, Lina Wang wrote:
> > > When clatd starts with ebpf offloaing, and NETIF_F_GRO_FRAGLIST is enable,
> > > several skbs are gathered in skb_shinfo(skb)->frag_list. The first skb's
> > > ipv6 header will be changed to ipv4 after bpf_skb_proto_6_to_4,
> > > network_header\transport_header\mac_header have been updated as ipv4 acts,
> > > but other skbs in frag_list didnot update anything, just ipv6 packets.
> > > 
> > > udp_queue_rcv_skb will call skb_segment_list to traverse other skbs in
> > > frag_list and make sure right udp payload is delivered to user space.
> > > Unfortunately, other skbs in frag_list who are still ipv6 packets are
> > > updated like the first skb and will have wrong transport header length.
> > > 
> > > e.g.before bpf_skb_proto_6_to_4,the first skb and other skbs in frag_list
> > > has the same network_header(24)& transport_header(64), after
> > > bpf_skb_proto_6_to_4, ipv6 protocol has been changed to ipv4, the first
> > > skb's network_header is 44,transport_header is 64, other skbs in frag_list
> > > didnot change.After skb_segment_list, the other skbs in frag_list has
> > > different network_header(24) and transport_header(44), so there will be 20
> > > bytes difference,that is difference between ipv6 header and ipv4 header.
> > 
> > > Actually, there are two solutions to fix it, one is traversing all skbs
> > > and changing every skb header in bpf_skb_proto_6_to_4, the other is
> > > modifying frag_list skb's header in skb_segment_list.
> > 
> > I don't think the above should be addressed into the GSO layer. The
> > ebpf program is changing the GRO packet in arbitrary way violating the
> > GSO packet constraint - arguably, it's corrupting the packet.
> > 
> > I think it would be better change the bpf_skb_proto_6_to_4() to
> > properly handle FRAGLIST GSO packets.
> > 
> > If traversing the segments become too costly, you can try replacing
> > GRO_FRAGLIST with GRO_UDP_FWD.
> 
> Yeah, I don't know...
> 
> I've considered that we could perhaps fix the 6to4 helper, and 4to6 helper...
> but then I think every *other* helper / code path that plays games
> with the packet header needs fixing as well,
> ie. everything dealing with encap/decap, vlan, etc..
> 
> At that point it seems to me like it's worth fixing here rather than
> in all those other places.
> 
> In general it seems gro fraglist as implemented is just a bad idea...
> Packets (and things we treat like packets) really should only have 1 header.
> GRO fraglist - as implemented - violates this pretty fundamental assumption.
> As such it seems to be on the gro fraglist implementation to deal with it.
> That to me seems to mean it should be fixed here, and not elsewhere.

@Steffen: IIRC GRO_FRAGLIST was originally added to support some
forwarding scenarios. Now we have GRO_UDP_FWD which should be quite
comparable. I'm wondering if the latter feature addresses your use
case, too.

If so, could we consider deprecating (and in a longer run, drop) the
GRO_FRAGLIST feature? 

Thanks!

Paolo