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]

Message-ID: <CANP3RGe8ko=18F2cr0_hVMKw99nhTyOCf4Rd_=SMiwBtQ7AmrQ@mail.gmail.com>
Date:   Tue, 8 Feb 2022 04:57:14 -0800
From:   Maciej Żenczykowski <maze@...gle.com>
To:     Paolo Abeni <pabeni@...hat.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

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.

(btw. wrt. this commit itself, it seems like the diff should be a signed int)