netdev - Re: [PATCH net] net: check the minimum value of gso size in virtio_net_hdr_to

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: <CAF=yD-JUgwSgYhYkOcJUkiuTkDLee6mfR0abikokpZfveCCp4g@mail.gmail.com>
Date: Mon, 28 Jul 2025 09:45:57 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Wang Liang <wangliang74@...wei.com>
Cc: mst@...hat.com, jasowang@...hat.com, xuanzhuo@...ux.alibaba.com, 
	eperezma@...hat.com, pabeni@...hat.com, davem@...emloft.net, 
	willemb@...gle.com, atenart@...nel.org, yuehaibing@...wei.com, 
	zhangchangzhong@...wei.com, netdev@...r.kernel.org, 
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	steffen.klassert@...unet.com, tobias@...ongswan.org
Subject: Re: [PATCH net] net: check the minimum value of gso size in virtio_net_hdr_to_skb()

On Sun, Jul 27, 2025 at 11:36 PM Wang Liang <wangliang74@...wei.com> wrote:
>
>
> 在 2025/7/28 1:15, Willem de Bruijn 写道:
> > Willem de Bruijn wrote:
> >> But so the real bug, an skb with 4B in the UDP layer happens before
> >> that.
> >>
> >> An skb_dump in udp_queue_rcv_skb of the GSO skb shows
> >>
> >> [  174.151409] skb len=190 headroom=64 headlen=190 tailroom=66
> >> [  174.151409] mac=(64,14) mac_len=14 net=(78,20) trans=98
> >> [  174.151409] shinfo(txflags=0 nr_frags=0 gso(size=4 type=65538 segs=0))
> >> [  174.151409] csum(0x8c start=140 offset=0 ip_summed=3 complete_sw=0 valid=1 level=0)
> >> [  174.151409] hash(0x0 sw=0 l4=0) proto=0x0800 pkttype=2 iif=8
> >> [  174.151409] priority=0x0 mark=0x0 alloc_cpu=1 vlan_all=0x0
> >> [  174.151409] encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
> >> [  174.152101] dev name=tun0 feat=0x00002000000048c1
> >>
> >> And of segs[0] after segmentation
> >>
> >> [  103.081442] skb len=38 headroom=64 headlen=38 tailroom=218
> >> [  103.081442] mac=(64,14) mac_len=14 net=(78,20) trans=98
> >> [  103.081442] shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
> >> [  103.081442] csum(0x8c start=140 offset=0 ip_summed=1 complete_sw=0 valid=1 level=0)
> >> [  103.081442] hash(0x0 sw=0 l4=0) proto=0x0800 pkttype=2 iif=8
> >> [  103.081442] priority=0x0 mark=0x0 alloc_cpu=0 vlan_all=0x0
> >> [  103.081442] encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
> >>
> >> So here translen is already 38 - (98-64) == 38 - 34 == 4.
> >>
> >> So the bug happens in segmentation.
> >>
> >> [ongoing ..]
> > Oh of course, this is udp fragmentation offload (UFO):
> > VIRTIO_NET_HDR_GSO_UDP.
> >
> > So only the first packet has an UDP header, and that explains why the
> > other packets are only 4B.
> >
> > They are not UDP packets, but they have already entered the UDP stack
> > due to this being GSO applied in udp_queue_rcv_skb.
> >
> > That was never intended to be used for UFO. Only for GRO, which does
> > not build such packets.
> >
> > Maybe we should just drop UFO (SKB_GSO_UDP) packets in this code path?
>
>
> Thank you for your analysis, it is totally right!
>
> After segment in udp_queue_rcv_skb(), these segs are not UDP packets, which
> leads the crash.
>
> The packet with VIRTIO_NET_HDR_GSO_UDP type from tun device perhaps should
> not enter udp_rcv_segment().
>
> How about not do segment and pass the packet to udp_queue_rcv_one_skb()
> directly, like this:
>
>    diff --git a/include/linux/udp.h b/include/linux/udp.h
>    index 4e1a672af4c5..0c27e5194657 100644
>    --- a/include/linux/udp.h
>    +++ b/include/linux/udp.h
>    @@ -186,6 +186,9 @@ static inline bool udp_unexpected_gso(struct sock
> *sk, struct sk_buff *skb)
>            if (!skb_is_gso(skb))
>                    return false;
>
>    +       if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
>    +               return false;
>    +
>            if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 &&
>                !udp_test_bit(ACCEPT_L4, sk))
>                    return true;

That seemingly is what would have happened before introducing
udp_unexpected_gso. The same for SKB_GSO_UDP_L4 packets actually.

It is not in any way an intended use case. And given the longstanding
breakage clearly not actually used.

UDP sockets do not expect GSO packets, so queuing them might confuse them.

Specifically for UFO, it can be argued that they are the same as large
packets that were build by the IP defrag layer.

Still to avoid having to start maintaining an unintentional code path,
including having to likely fix more reports over time and with that
likely add complexity to the real UDP hot path as well, I think we
should instead drop these.