Message-ID: <CAF=yD-+Mzk0ibfgByXqiS_y=FoKqLVtATKQF4PPpUL4Pk8hosw@mail.gmail.com>
Date: Sun, 27 Jul 2025 23:50:40 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Wang <jasowang@...hat.com>
Cc: Wang Liang <wangliang74@...wei.com>, mst@...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:21 PM Jason Wang <jasowang@...hat.com> wrote:
>
> On Mon, Jul 28, 2025 at 1:16 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Willem de Bruijn wrote:
> > > Wang Liang wrote:
> > > >
> > > > 在 2025/7/24 21:29, Willem de Bruijn 写道:
> > > > > Wang Liang wrote:
> > > > >> When sending a packet with virtio_net_hdr to tun device, if the gso_type
> > > > >> in virtio_net_hdr is SKB_GSO_UDP and the gso_size is less than udphdr
> > > > >> size, below crash may happen.
> > > > >>
> > > > > gso_size is the size of the segment payload, excluding the transport
> > > > > header.
> > > > >
> > > > > This is probably not the right approach.
> > > > >
> > > > > Not sure how a GSO skb can be built that is shorter than even the
> > > > > transport header. Maybe an skb_dump of the GSO skb can be elucidating.
> > > > >>                          return -EINVAL;
> > > > >>
> > > > >>                  /* Too small packets are not really GSO ones. */
> > > > >> --
> > > > >> 2.34.1
> > > > >>
> > > >
> > > > Thanks for your review!
> > >
> > > Thanks for the dump and repro.
> > >
> > > I can indeed reproduce, only with the UDP_ENCAP_ESPINUDP setsockopt.
> > >
> > > > Here is the skb_dump result:
> > > >
> > > >      skb len=4 headroom=98 headlen=4 tailroom=282
> > > >      mac=(64,14) mac_len=14 net=(78,20) trans=98
> > > >      shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
> > > >      csum(0x8c start=140 offset=0 ip_summed=1 complete_sw=0 valid=1 level=0)
> > >
> > > So this is as expected not the original GSO skb, but a segment,
> > > after udp_rcv_segment from udp_queue_rcv_skb.
> > >
> > > It is a packet with skb->data pointing to the transport header, and
> > > only 4B length. So this is an illegal UDP packet with length shorter
> > > than sizeof(struct udphdr).
> > >
> > > The packet does not enter xfrm4_gro_udp_encap_rcv, so we can exclude
> > > that.
> > >
> > > It does enter __xfrm4_udp_encap_rcv, which will return 1 because the
> > > pskb_may_pull will fail. There is a negative integer overflow just
> > > before that:
> > >
> > >         len = skb->len - sizeof(struct udphdr);
> > >         if (!pskb_may_pull(skb, sizeof(struct udphdr) + min(len, 8)))
> > >                 return 1;
> > >
> > > This is true for all the segments btw, not just the last one. On
> > > return of 1 here, the packet does not enter encap_rcv but gets
> > > passed to the socket as a normal UDP packet:
> > >
> > >       /* If it's a keepalive packet, then just eat it.
> > >        * If it's an encapsulated packet, then pass it to the
> > >        * IPsec xfrm input.
> > >        * Returns 0 if skb passed to xfrm or was dropped.
> > >        * Returns >0 if skb should be passed to UDP.
> > >        * Returns <0 if skb should be resubmitted (-ret is protocol)
> > >        */
> > >       int xfrm4_udp_encap_rcv(struct sock *sk, struct sk_buff *skb)
> > >
> > > 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?
> >
>
> Just to make sure I understand this. Did you mean to disable UFO for
> guest -> host path? If yes, it seems can break some appllication.

No, I mean inside the special segmentation path inside UDP receive.

I know that we have to keep UFO segmentation around because existing
guests may generate those packets and these features cannot be
re-negotiated once enabled, even on migration. But no new kernel
generates UFO packets.

Segmentation inside UDP receive was added when UDP_GRO was added, in
case packets accidentally add up at a local socket receive path that
does not support large packets.

Since GRO never builds UFO packets, such packets should not arrive at
such sockets to begin with.

Evidently we forgot about looping virtio_net_hdr packets. They were
never intended to be supported in this new path, nor clearly have they
ever worked. We just need to mitigate them without crashing.

Powered by blists - more mailing lists