Message-ID: <CACGkMEvL0_6a1u0riEbQV-oVem_vfnTy48X2H3RcXF_MgL-zZg@mail.gmail.com>
Date: Tue, 29 Jul 2025 10:40:52 +0800
From: Jason Wang <jasowang@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.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 Mon, Jul 28, 2025 at 11:51 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> 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.

Thanks a lot for the clarification. It's clear to me now.

>

Powered by blists - more mailing lists